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 regression of pytest.pex
leaking into coverage data
#11110
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# We pass `--ignore-errors` because Pants dynamically injects missing `__init__.py` | ||
# files and this will cause Coverage to fail. | ||
argv=(report_type.report_name, "--ignore-errors"), | ||
argv=(report_type.report_name,), |
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.
Removing --ignore-errors
without the above fix causes the integration tests to fail, so I think that the integration tests are already a good enough regression test for this edge case.
if "test_runner.pex/*" not in omit_elements: | ||
omit_elements.append("test_runner.pex/*") | ||
if "pytest.pex/*" not in omit_elements: | ||
omit_elements.append("pytest.pex/*") |
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.
Should this rule be getting this filename from a constant somewhere?
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.
Maybe, we would factor up this line:
pants/src/python/pants/backend/python/goals/pytest_runner.py
Lines 120 to 123 in b9c7c4d
pytest_pex_request = Get( | |
Pex, | |
PexRequest( | |
output_filename="pytest.pex", |
Wdyt? Probably worth it to make explicit the dependency.
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.
Yea... I stopped short of suggesting a subsystem dependency... just a constant should be fine?
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.
Oh, this results in a circular dependency. Going to stick with this, since the tests will catch this error in the future.
This happened when we removed `test_runner.pex`. We didn't catch it because we set `--ignore-errors`, which should be safe to remove now that we no longer magically inject `__init__.py` files like we did before. [ci skip-rust] [ci skip-build-wheels]
…11112) [ci skip-rust] [ci skip-build-wheels]
This happened when we removed
test_runner.pex
. We didn't catch it because we set--ignore-errors
, which should be safe to remove now that we no longer magically inject__init__.py
files like we did before.[ci skip-rust]
[ci skip-build-wheels]