Make the test suite use this repo's compiled helpers - #38
Conversation
The suite shells out to the bundled C++ helpers by bare name (RRA, mageckGSEA), so it exercises whichever build comes first on PATH. That is the right binary after `pip install .`, which compiles them via BuildWithHelpers and installs them alongside the package -- but not during ordinary local development. An editable install never runs that build step, and a MAGeCK v1 package in the same environment ships commands with the *same names*. The result is that the suite silently tests v1's binaries. Locally that surfaced as four mageckGSEA failures asserting fixes that are present in this checkout: the absent-gene and degenerate-pathway tests got v1's answers, and the -H test failed on a usage error because v1 has no --skip_header flag at all. Nothing in the output points at the binary being the wrong one. Add a conftest that puts rra/bin and gsea/bin first on PATH when the helpers have been built there, so the tests always describe this checkout. When they have not been built it changes nothing and the installed binaries are used, so CI behaviour is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a82118e57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if prefix: | ||
| os.environ["PATH"] = os.pathsep.join(prefix + [os.environ.get("PATH", "")]) |
There was a problem hiding this comment.
In the CI workflow, pip install .[test] runs BuildWithHelpers, whose make calls leave both executables in these source-tree directories; this hook therefore always prepends those build outputs before pytest -v. Consequently the smoke tests no longer exercise the copies installed through data_files, so a regression in helper installation, destination, or permissions can pass CI even though an installed MAGeCK2 cannot find or execute its helpers. Limit this override to the editable/local-development case or retain a CI test that uses the unmodified installation PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6aba9ff. You were right on both the mechanism and the consequence.
Verified the mechanism first — pip install .[test] into a fresh venv leaves the binaries in both places:
source tree: rra/bin/RRA, gsea/bin/mageckGSEA (make output)
venv: .venv/bin/RRA, .venv/bin/mageckGSEA (data_files copy)
So the fallback my PR description claimed CI relies on never fires. That description was wrong; I've corrected it.
Then measured the coverage loss by simulating a data_files regression — installed helpers removed, source tree intact, venv built the way CI builds one:
with the conftest as submitted -> 49 passed (a broken release ships green)
with no conftest at all -> 8 failed
after this fix -> 2 failed
The 49-passed row is exactly your point, and it would have masked a regression of the bug 0.2.0 fixed.
The fix keeps both coverages rather than dropping the override. conftest now publishes the pre-override PATH as MAGECK2_INSTALLED_PATH, and the two tests that assert the helpers are installed — test_rra_binary_on_path, test_mageckgsea_binary_on_path — resolve against it. The rest of the suite still tests the checkout's own build, which is the point of the override.
I preferred this to limiting the override to editable installs: "am I editable?" is awkward to detect reliably, and the installation contract deserves an explicit assertion rather than being implied by whichever binary happened to be found.
A side benefit — this reports the failure better than the pre-conftest baseline did. A broken install used to surface as eight GSEA assertion failures naming nothing relevant; it now says compiled RRA binary not installed onto PATH.
Verified across all three scenarios: good install 49 pass, broken install 2 fail (named), local editable install with v1 helpers in the environment 49 pass.
The PATH override applies in CI as well as locally: pip install . runs
make in the source tree, so the build outputs sit in rra/bin and
gsea/bin alongside the copies data_files installs. Prepending them meant
the installed copies were never exercised, and a regression in helper
installation could pass CI even though an installed MAGeCK2 could not
find its helpers -- the exact failure 0.2.0 fixed.
Measured on a venv built the way CI builds one, with the installed
helpers removed and the source tree left intact:
before this commit 49 passed (broken install ships green)
after 2 failed test_rra_binary_on_path,
test_mageckgsea_binary_on_path
conftest now publishes the pre-override PATH as MAGECK2_INSTALLED_PATH,
and the two tests that assert the helpers are installed look there. The
rest of the suite still tests this checkout's own build, which is the
point of the override.
This also reports the failure better than before the conftest existed: a
broken install used to surface as eight GSEA assertion failures that
named nothing relevant, and now names the missing binary directly.
Reported by Codex review on #38.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
The suite invokes the bundled C++ helpers by bare name (
RRA,mageckGSEA), so it exercises whichever build comes first onPATH. That is the right binary afterpip install ., which compiles them and installs them alongside the package — but not during ordinary local development. An editable install never runs that build step, and a MAGeCK v1 package in the same environment ships commands with the same names.The result is that the suite silently tests v1's binaries and reports failures against fixes that are present in this checkout. Nothing in the output points at the binary being the wrong one.
Measured
Same environment, same
PATH, only the conftest differing:The four are all
mageckGSEAtests —test_pathway_gsea_end_to_end,test_gsea_skips_genes_absent_from_rank_file,test_gsea_degenerate_pathway_is_not_maximally_significant,test_gsea_skip_header_matches_headerless. They assert fixes this repo has and v1 does not; the-Hone fails on a usage error, because v1 has no--skip_headerflag at all.The change
pytest_configureprependsrra/binandgsea/bintoPATH, but only for helpers that actually exist there.Verified the fallback explicitly: with both
bindirectories renamed away, the suite still collects and runs, falling back to the installed binaries. No crash, no collection error.Preserving installed-helper coverage
The override applies in CI too, not just locally —
pip install .runsmakein the source tree, so the build outputs sit inrra/binandgsea/binalongside the copiesdata_filesinstalls. (An earlier revision of this description claimed CI takes the fallback path; that was wrong, and Codex caught it.)Left alone, that would mean the installed copies are never exercised, so a regression in helper installation could pass CI even though an installed MAGeCK2 cannot find its helpers — the exact failure 0.2.0 fixed. Measured, with the installed helpers removed and the source tree intact:
So
conftestpublishes the pre-override PATH asMAGECK2_INSTALLED_PATH, and the two tests asserting the helpers are installed resolve against it. The rest of the suite still tests this checkout's build. This also names the failure better than the pre-conftest baseline: a broken install used to surface as eight GSEA assertion failures naming nothing relevant, and now sayscompiled RRA binary not installed onto PATH.Why now
This is a release blocker for 0.3.0 in practice: anyone verifying the tag from a fresh clone under an editable install sees four failures that have nothing to do with the code. I have hit it in every session on this repo.
The commit is unchanged from the one authored on Aug 4; it was branched off an older
mainand never pushed, so this is a cherry-pick onto currentmainrather than a merge of the stale branch — merging that branch as-is would have deleted ~800 lines of tests added since.