-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix science build
to handle nested file names.
#7
Conversation
Previously a flat name like "science.pyz" would work without a `--path` mapping if the file was in the cwd, but a name like "dist/science.pyz" would not even if it the dile was at that path relative to the cwd. Fixes a-scie#2
@sureshjoshi you've got an invite to collaborate in this repo, but even if you don't want to accept that, perhaps you want to review the test. The fix is exactly what you suggested with a new test that fails without the fix. |
science build
to handle nest file names.science build
to handle nested file names.
tests/test_exe.py
Outdated
@@ -142,3 +145,50 @@ def test_dogfood(tmp_path: Path, science_exe: Path, config: Path, science_pyz: P | |||
"Expected the bootstrap science executable to be able to build itself and produce a " | |||
"byte-wise identical science executable." | |||
) | |||
|
|||
|
|||
def test_issue_2(tmp_path: Path, science_exe: Path, config: Path, science_pyz: Path) -> None: |
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.
This is a matter of taste, so feel free to ignore, but I prefer the test name to indicate what we're testing for (test_nested_filenames
or whatever) with a comment linking to the issue, rather than the test name being opaque to the reader and forcing them to scramble for the issue to figure out what the test is trying to do.
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.
That makes sense.
I like what I have here best:
https://github.com/pantsbuild/scie-pants/blob/42ba03064ef5f76de4cba3c9fe2be5cace63d995/package/src/test.rs#L30C4-L32
https://github.com/pantsbuild/scie-pants/blob/42ba03064ef5f76de4cba3c9fe2be5cace63d995/package/src/test.rs#L656-L659C7
Which renders a clickable link in most modern terminals when you run the tests:
https://github.com/pantsbuild/scie-pants/actions/runs/4973791786/jobs/8899860210#step:10:528
I did some pytest mucking to get something similar.
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.
Previously a flat name like "science.pyz" would work without a
--path
mapping if the file was in the cwd, but a name like "dist/science.pyz"
would not even if it the file was at that path relative to the cwd.
Fixes #2