Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork674
feat(WIP): allow pytest deps to come from target resolve#22835
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
feat(WIP): allow pytest deps to come from target resolve#22835
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5035b26 to903daaaCompare| pytest_req_strings=pytest.requirementsifpytest.requirementselsepytest.default_requirements | ||
| pytest_requirements=PexRequirements( | ||
| pytest_req_strings, | ||
| from_superset=Resolve(target_resolve,use_entire_lockfile=False), | ||
| description_of_origin="pytest requirements from target resolve", | ||
| ) | ||
| pytest_pex_get=create_pex( | ||
| PexRequest( | ||
| output_filename="pytest_from_target_resolve.pex", | ||
| internal_only=True, | ||
| requirements=pytest_requirements, | ||
| interpreter_constraints=interpreter_constraints, | ||
| ) | ||
| ) |
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.
author's note: maybe this logic would be better close toPythonToolBase.pex_requirements
| defpex_requirements( |
Kept it here initially because it feels like this behavior is only desirable for pytest specifically not all python tools due to it's nature as both a library and a tool
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.
We can always move it later, and for now this keeps the blast radius more contained.
benjyw left a comment
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 looks good, thanks for the contribution!
How have you tested it? It would be good to add some testing inpytest_runner_test.py.
Uh oh!
There was an error while loading.Please reload this page.
Context
Currently, the pytest runner creates an environment composed by a mix of the
python_testdependencies and the pytest dependencies that are needed for the test to properly run. In cases which a third-party dependency is contained both the python_test dependency as well as pytest with potentially different versions (since they come from independent resolves), this can lead to inconsistencies between production and testing environment, which is undesirable.Related issues and threads
Potential solution (work in progress)
One way to avoid that is to make sure pytest dependencies come from the same resolve as the one used by the python_test target. In cases where the monorepo has a single resolve, this can already be solved with the
pytest.install_from_resolvesetting. However, in cases where the monorepo contains more than one resolve, there's currently no way to solve that.This PR introduces
pytest.install_from_target_resolve: BoolOptionsetting that, when set true, ignores the pytest resolve configuration and expects test dependencies to come from the target resolve. This ensures all dependencies are aligned across different execution environments.Potential problems
Since we are ignoring pytest dependencies, we need to make sure they are included on the list of dependencies before building the venv, otherwise this won't work.this should be addressed on903daaa