-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-46425: fix direct invocation of test_importlib
#30682
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
Conversation
So I'm of two minds on this. One, this was actually done on purpose as the expectation is to run the tests via But none of this is also important, so I don't object to this PR either. 🙂 I'll let @corona10 or someone else handle reviewing and merging. |
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.
I'm not quite sure what the objective here is. Is it only so you can run any of the files directly as a script? For something as extensive as the importlib test suite, what's the advantage of running individual modules as scripts? Is this something someone needs or is your goal only consistency? (We generally discourage blanket changes motivated by a desire for consistency, due to the costs of churn in a large project like this.)
Almost always, you would want to run all of the importlib tests:
./python -m test test_importlib
./python -m unittest test.test_importlib
- (rarely)
./python -m test.test_importlib
If you are actually working on importlib and want to run a specific subset of the tests then you'd usually use "-m", for example:
./python -m unittest test.test_importlib.builtin.test_loader
- (specific test)
./python -m unittest test.test_importlib.builtin.test_loader.LoaderTests.test_module
(which you can't do as cleanly when invoking with a script)
So I'd be interested in hearing the use case for running one of the individual importlib test files as a script. Also, you should definitely get @brettcannon's feedback since he wrote nearly all these tests and likely has a clear opinion on the matter.
In case there is a good enough use case for run-as-script then the change mostly LGTM. I've left some comments noting files that should be left alone.
Thanks, Brett. I think that the main thing here is that most modules are working fine and only some are broken when invoked directly as |
That's fair, but I want to be clear that nothing is "broken". There is no general rule that sub-tests need to function as separate execution targets. As I have said, I don't object to the change specifically, but I'm also not going to be the one who merges it either. 😁 |
For me, on Windows, with fresh 3.11, |
Yeah, this word might a bit harsh. Let's not use it 🙂 Let's say I am newcomer to CPython's source code (which I +- am). And I want to work on some feature/bugfix in cpython/Lib/test/test_importlib/builtin/test_finder.py Lines 97 to 98 in cfadcc3
But, if I try to run it directly (which this module declare to support) - tests will fail. I think, it can be improved! And an it is easy to do: change imports, add |
This change causes problems within the backports (importlib_metadata, importlib_resources). This code is kept in sync with those projects and thus (a) relies on the relative imports to find the fixtures in a relative location and (b) would never expect the modules to be invoked as a script. As primary maintainer of importlib.metadata and importlib.resources, I'd prefer to keep the code portable and simple (encapsulated). It's conceivable that the backport could maintain a forked version of these files that doesn't include these behaviors, but that adds a cognitive and maintenance burden to the effort. In my effort, the lack of |
@jaraco thanks! I will send a new PR with restored changes in |
Thanks sobolevn. I missed your message prior to preparing and submitting #30799. Since it's not clear the demarcation of the tests, I figured it would be better if I helped out with that part. I went through and flagged what I think are the relevant files. I'm going to use that PR to validate that I got the correct files as well (by performing a dry-run sync into the backports). |
Multiple tests in
test_importlib
were not suited to direct invocation. Why?unittest.main()
callIn several places relative imports were used in
setUp()
. It was also problematic:Now, all
test_importlib
modules pass the local check 🎉Size of the PR
This PR is quite big. If desireable, I can split it into multiple parts.
CC @corona10 as my mentor.
https://bugs.python.org/issue46425