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
Add a pythonpath
setting to allow paths to be added to sys.path
.
#9134
Conversation
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.
Thanks @okken, looking good! I left some comments.
Also:
We need to document this feature somewhere.
I think the plugin can use a cleanup hook. While the vast majority of users use pytest thru the pytest
command, where the the session only executes once and then the interpreter exits, it is also possible to use pytest.main()
. Without cleanup, if someone calls pytest.main()
then goes on to do other stuff, their sys.path will be polluted.
There are various approaches to doing the cleanup I can think of:
- Take a sys.path snapshot before, just restore it after.
- Remove the
pythonpath
values indiscriminately. - Remove the
pythonpath
values if they match their expected positions.
1 and 3 are safer, but have a problem that if say some library imported by pytest modifies sys.path at import time, it will get lost (1) or not cleanup (3).
2 however can mess with pre-existing sys.path.
Not sure what is best, basically what we want is to remove what we added, and only that, maybe there's some trick to achieve it properly.
Pushed changes based on review feedback. |
I've "resolved" some conversations after making changes. Let me know if that's NOT the correct procedure. :) |
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.
A couple more comments.
Feel free to resolve my comments as you fix them, the PR is small enough to keep everything in my head :)
Co-authored-by: Ran Benita <ran@unusedvar.com>
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.
The code looks good to me; nice and clean!
To verify, I tested it now on my aforementioned project which currently uses pytest-pythonpath. I found one issue, see line comments. With that fixed, it's working as intended.
As for the docs, just need to add a confval
to doc/en/reference/reference.rst
(and make the changelog entry reference it) and that would be enough as far as I'm concerned.
Co-authored-by: Ran Benita <ran@unusedvar.com>
…14_pythonpath_setting
@bluetech @RonnyPfannschmidt |
@bluetech @RonnyPfannschmidt |
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.
Docs LGTM, with a few suggestions.
Co-authored-by: Ran Benita <ran@unusedvar.com>
Thanks @bluetech for the documentation assistance, as well as all the other help with this PR. |
@bluetech Please let me know if there's anything else that needs done for this PR. |
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.
@bluetech Please let me know if there's anything else that needs done for this PR.
I took the liberty of adding a cleanup test and pushing it to your branch, PTAL and feel free to make any changes to it. With it, and the small suggestion below, it's good to go from my side.
Since it's a new feature, I'd like the other maintainers to approve as well. Particularly @nicoddemus -- this might have some interaction with the import modes which touch sys.path as well, though I think it should be OK.
Also cc the author of pytest-pythonpath, @ericpalakovichcarr: this PR adds the equivalent of pytest-pythonpath into pytest itself, as a core plugin. We'd love to hear if you have any thoughts or comments on this! |
assert before is not None | ||
assert after is not None | ||
assert any("I_SHALL_BE_REMOVED" in entry for entry in before) | ||
assert not any("I_SHALL_BE_REMOVED" in entry for entry in after) |
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.
Cool tests. Thanks @bluetech
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.
LGTM!
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.
Particularly @nicoddemus -- this might have some interaction with the import modes which touch sys.path as well, though I think it should be OK.
Thanks for the heads up. Indeed it is fine. 👍
Thanks @okken!
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.
lovely, well done 👍
Thanks @okken! |
Obsoleted by pytest-dev/pytest#9134, which was released as part of pytest 7.0.0. It has pytest<7 pinned, and we only have the single pytest version, so it can be removed.
Obsoleted by pytest-dev/pytest#9134, which was released as part of pytest 7.0.0. It has pytest<7 pinned, and we only have the single pytest version, so it can be removed.
Obsoleted by pytest-dev/pytest#9134, which was released as part of pytest 7.0.0. It has pytest<7 pinned, and we only have the single pytest version, so it can be removed.
Obsoleted by pytest-dev/pytest#9134, which was released as part of pytest 7.0.0. It has pytest<7 pinned, and we only have the single pytest version, so it can be removed.
I was just going in circles for a while looking for this exact feature, lol! Everyone on the internet recommends pytest for Python testing, but nobody mentions how to actually get it working when your source and tests aren't next to each other. There are countless google hits for people suggesting "sys.path.append" code snippits. It's the wild west out there. Maybe I should make a feature request for this. I think it would be good to add some mention in the basic documentation. For example, the files: When there's mention of 1) starting a new project, or 2) a project directory hierarchy, there should be mention of setting pythonpath in pyproject.toml to include your source path. It should be assumed that normies like me are reading this documentation and just want to get their first unit test on their first program to run. |
Good point @LuminairPrime, created #9833 for this. Perhaps that's something you would like to contribute? 😁 |
https://build.opensuse.org/request/show/972472 by user mcalabkova + dimstar_suse from upstream README: **NOTE:** This plugin is obsolete as of pytest 7.0.0. Thanks to [this PR](pytest-dev/pytest#9134) from [Brian Okken](https://github.com/okken), you can now modify the PYTHONPATH using the `pythonpath` configuration option. See documentation here: https://docs.pytest.org/en/7.0.x/reference/reference.html#confval-pythonpath ... and as we didn't fork pytest 6, there is no package to provide pytest <7
Closes #9114
Closes #8964
I made a lot of assumptions here, so please comment if necessary.
Question:
Implementation is based on: