-
Notifications
You must be signed in to change notification settings - Fork 3
Make the test suite use this repo's compiled helpers #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| """Test configuration for MAGeCK2. | ||
|
|
||
| The suite shells out to the bundled C++ helpers by bare name (``RRA``, | ||
| ``mageckGSEA``), so it exercises whichever build happens to come first on PATH. | ||
| That is the correct binary after ``pip install .`` -- setup.py compiles the | ||
| helpers 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 suite | ||
| then silently tests v1's binaries and reports failures against fixes that are | ||
| present in this checkout, which is confusing and points nowhere useful. | ||
|
|
||
| Put this repository's own freshly built helpers first on PATH when they exist, | ||
| so the tests always describe this checkout. | ||
|
|
||
| Note this override applies in CI too: ``pip install .`` runs ``make`` in the | ||
| source tree, so the build outputs are present there alongside the copies | ||
| ``data_files`` installs. The installed copies would then never be exercised, and | ||
| a regression in helper installation could pass CI even though an installed | ||
| MAGeCK2 cannot find its helpers. To keep that coverage, the PATH as it stood | ||
| before this hook ran is published as ``MAGECK2_INSTALLED_PATH``, and the tests | ||
| asserting the helpers are installed look there rather than at the build tree. | ||
| """ | ||
|
|
||
| import os | ||
| from pathlib import Path | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parent.parent | ||
|
|
||
| # (binary name, directory the Makefile writes it to) | ||
| HELPER_BIN_DIRS = [ | ||
| ("RRA", REPO_ROOT / "rra" / "bin"), | ||
| ("mageckGSEA", REPO_ROOT / "gsea" / "bin"), | ||
| ] | ||
|
|
||
|
|
||
| #: The PATH before this hook ran -- i.e. where an *installed* MAGeCK2 finds its | ||
| #: helpers. Published so tests can assert on the installation rather than on the | ||
| #: build tree that gets prepended below. | ||
| INSTALLED_PATH_ENV = "MAGECK2_INSTALLED_PATH" | ||
|
|
||
|
|
||
| def pytest_configure(config): | ||
| """Prepend repo-built helper directories to PATH, most specific first.""" | ||
| os.environ.setdefault(INSTALLED_PATH_ENV, os.environ.get("PATH", "")) | ||
|
|
||
| prefix = [] | ||
| for name, bindir in HELPER_BIN_DIRS: | ||
| if (bindir / name).is_file(): | ||
| prefix.append(str(bindir)) | ||
|
|
||
| if prefix: | ||
| os.environ["PATH"] = os.pathsep.join(prefix + [os.environ.get("PATH", "")]) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CI workflow,
pip install .[test]runsBuildWithHelpers, whosemakecalls leave both executables in these source-tree directories; this hook therefore always prepends those build outputs beforepytest -v. Consequently the smoke tests no longer exercise the copies installed throughdata_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 installationPATH.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: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_filesregression — installed helpers removed, source tree intact, venv built the way CI builds one: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.
conftestnow publishes the pre-override PATH asMAGECK2_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.