Skip to content
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

bpo-40055: distutils tests now disable docutils import #19139

Closed
wants to merge 1 commit into from
Closed

bpo-40055: distutils tests now disable docutils import #19139

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 24, 2020

Prevent docutils from being imported since "import docutils" can have
side effects on tests. For example, it imports pkg_resources which
alters warnings filters.

https://bugs.python.org/issue40055

Prevent docutils from being imported since "import docutils" can have
side effects on tests. For example, it imports pkg_resources which
alters warnings filters.
@vstinner
Copy link
Member Author

@merwok @pganssle: Would you mind to review this change? Are you ok to not behaving differently if docutil is available or not? (always make it not available)

@merwok
Copy link
Member

merwok commented Mar 27, 2020

It is a bit sad to lose test coverage for a feature.
But if it has to be done, it should be done by deleting the tests IMO.

@vstinner
Copy link
Member Author

I'm waiting for @pganssle who works on setuptools and so pkg_resources.

@pganssle
Copy link
Member

I think @jaraco knows more about pkg_resources than me, TBH.

I'm not entirely sure I understand the problem here, but avoiding imports of docutils seems like the wrong thing to do here. From the bug report it looks like the problem is that if you import docutils it imports pkg_resources, which configures the warning levels for warnings related to pkg_resources. I do not see why this is a conceptual problem for CPython's tests.

I get that it means that importing docutils has side effects, but importing literally anything has at minimum the side effect of populating the sys.modules cache, so I don't see why other modifications to globals is a problem. It seems to me like the real issue is that whatever is generating the warning about the warnings filters being adjusted is just a bit over-sensitive, and we should probably fix this by trying to fix that warning's mechanism to be more aligned with whatever bad effects it's trying to solve.

@vstinner
Copy link
Member Author

I do not see why this is a conceptual problem for CPython's tests.

regrtest (our test runner) requires that tests have zero side effects. Here the problem are warnings filters.

If a test changes warnings filters: they must be saved and then restored.

I get that it means that importing docutils has side effects, but importing literally anything has at minimum the side effect of populating the sys.modules cache,

Modifying sys.modules is fine.

we should probably fix this by trying to fix that warning's mechanism to be more aligned with whatever bad effects it's trying to solve.

I'm also fine with only fixing (save/restore?) warnings filters.

@merwok
Copy link
Member

merwok commented Mar 30, 2020

+1 to add setup/teardown to save and restore warning filters then.
(I could look into it maybe at the end of the week, or review a PR on any day)

@pganssle
Copy link
Member

I would also prefer to either clear the warnings filters or clear the warnings filters and eject pkg_resources from sys.modules, but I see all of these solutions as somewhat brittle hacks around a somewhat sloppy definition of "no side-effects".

I don't see the change to the warnings filters as terribly different from modifying sys.modules, since this is evidently part of the package initialization of pkg_resources. Entering pkg_resources into sys.modules in one test will prevent the package from being executed again if you run import pkg_resources, which means that later tests using pkg_resources will suffer from the side-effect of the cache having been populated and the filters having been cleared.

My guess is that the principle behind tests not having side effects is that we want tests or test modules to be independent of one another. If some random test is manipulating filters that other tests may use, that would be a violation of that principle, but this is just pkg_resources settings its own defaults for the warnings filters related to its own warnings - if anything, we should be making sure that once pkg_resources is in the sys.modules cache, anything that changes those warnings filters doesn't restore them to the pre-import stage.

I feel like the right long-term solution would be to move to a testing scenario where every test module is run as separate processes with their own namespaces rather than trying to make sure everything plays nicely with process-wide globals, but that feels like a much bigger undertaking than is warranted for this minor issue, so I say let's go with something where we clear the warnings filters and maybe eject pkg_resources from sys.modules (which seems like the right thing to do, but also I don't think these warnings filters matter at all for our purposes so 🤷‍♂️).

@vstinner
Copy link
Member Author

I wrote PR #20095 to save/restore warnings filters, rather than preventing test_distutils to import docutils.

I feel like the right long-term solution would be to move to a testing scenario where every test module is run as separate processes

Each test file is run in a separated process by all Python CIs. But regrtest is quite strict: it requires that tests don't modify "globals" (see Lib/test/libregrtest/save_env.py). It's a watchdog to capture various kinds of bugs.

@vstinner vstinner closed this May 14, 2020
@vstinner vstinner deleted the distutils_no_docutils branch May 14, 2020 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants