Repository navigation
BUG: avoid test pollution in astropy.units - #17750
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
neutrinoceros
changed the title
astropy.units
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Great that you found the solution, though this leaves me a little confused -- and it would be good to understand why before merging...
| def test_scipy_registration(): | ||
| """Check that scipy gets loaded upon first use.""" | ||
| assert sps.erf not in qh.UFUNC_HELPERS | ||
| if sps.erf in qh.UFUNC_HELPERS: |
There was a problem hiding this comment.
Maybe add a comment?
# Generally, scipy will not be loaded here, but in a double run it might.
I also think it may make sense to test instead a fresh instance of UfuncHelpers, but perhaps for another PR.
There was a problem hiding this comment.
note that TestScipySpecialUfuncs (the class immediately following this test) also relies on this test being run first, so I can't simply replace the test.
There was a problem hiding this comment.
(I added your comment)
| """ | ||
| with self._lock: | ||
| module_info = self.modules.pop(module) | ||
| module_info = self.modules[module] |
There was a problem hiding this comment.
I'm super confused why this would be necessary. What test fails if you keep it? It should not be possible to get here more than once...
There was a problem hiding this comment.
test_scipy_registered is the test that fails without this. See the reprod invoke in the PR description.
There was a problem hiding this comment.
And yes, we don't get here more than once, but that test explicitly checks for a state that is changed by this line.
There was a problem hiding this comment.
I think I'd prefer to change the tests then, skipping that with @pytest.mark.skipif(HAS_SCIPY)
There was a problem hiding this comment.
This makes sense too, but I don't get what you don't like about my less destructive import_module ?
There was a problem hiding this comment.
The advantage of what I have here is that, if ever something when wrong with checking membership, we will know, rather than everything becoming very slow as for every call the whole module gets loaded back in.
But, more generally, I prefer not to change code to fix the double tests if it is arguably the test that was written sloppily. We cannot know whether somewhere in the wild code relies on the current behaviour.
There was a problem hiding this comment.
fair enough. Adopted !
3d95f04 to
322a27d
Compare
322a27d to
9f9d92a
Compare
|
I am afraid to ask but I should: Does this mean we need to add yet another job to CI to smoke these out automatically in the future? |
…750-on-v7.0.x Backport PR #17750 on branch v7.0.x (BUG: avoid test pollution in `astropy.units`)
|
I think the idea is to make the double test depend at least also on |
|
Like this? |
Description
Partially address #17745
This fixes two issues:
UfuncHelpers.import_modulewas needlessly destructive (removing keys fromself.modules), which led madetest_scipy_registeredfail if run after scipy.special was imported.test_scipy_registrationrelies on being run before any scipy.special method is called (which implicitly triggers registration), so it cannot possibly succeed when run at a later point. I don't see a good solution here and I would like to avoid mocking global state too much, so I hope that simply skipping the test on a second run is acceptable.reprod (requires scipy)