numpy icon indicating copy to clipboard operation
numpy copied to clipboard

DEV: stop monkeypatching and use of `autouse=True` fixtures by default in test suite

Open rgommers opened this issue 8 months ago • 2 comments

tl;dr there is way too much messing around with tests, warnings, fixtures, environment variables, etc. going on in numpy/conftest.py, we should stop doing that.

I started looking at this because in the process of trying to use pytest-run-parallel, I found that every single test in our test suite uses the monkeypatch fixture. This is enabled here:

https://github.com/numpy/numpy/blob/7d2e4418e17eeaed1045e37335bacf0f01343f4d/numpy/conftest.py#L150-L152

The use of PYTHONHASHSEED=0 was originally introduced for gh-2939, which turned out to be a SciPy rather than an f2py bug. It seems like a bad idea to set such env vars to non-default values in general - even in that original issue, the hash randomization actually found a bug, so disabling it should have been a temporary workaround. There are multiple ways this can affect other tools and test scenarios, e.g.:

  • pytest-leaks doesn't work with it, and we document here that to use pytest-leaks effectively, this fixture and a doctest fixture should be disabled by hand in conftest.py`
  • pytest-run-paralllel is simply rendered ineffective; it disables running any test in parallel when the thread-unsafe monkeypatch fixture is used
  • hypothesis: it seems like turning off hash randomization is a bad default for it, at least that's what I deduce from https://github.com/numpy/numpy/pull/14440#discussion_r322400004. The custom hypothesis profiles we have should be working around this already, but it still illustrates the problem.

There are other similar issues with the other fixtures (e.g., the warnings module is also thread-unsafe currently). We should clean this up and have saner defaults, then enable fixtures only when they are actually needed. E.g., all the mess we need for doctesting needs to only be turned on if doctests are actually requested.

The last thing to look at is the FPU checks. There are still two different ways of handling that, the comments on the oldest way say that it's to deal with tests that use yield. We still have a whole bunch of those, so that isn't easy to clean up - but it'd be nice to do that at some point. It looks like this is also a case where this check should be enabled in CI rather than disabled by default though, since there is only a single issues containing the string "FPU precision mode changed" (gh-23545), so this pretty invasive check catches very little.

rgommers avatar May 30 '25 08:05 rgommers

True Reality...! ⬇ The monkey patching and auto fixtures used in NumPy's tests rely on global application, which leads to test brittleness and prevents tests from running in parallel. The solution is to stop globally using patches. We should instead use explicit, local fixtures that localise what we change. It will be an important step for cleaner tests, faster test runs, and more resilient tests to move to the decomposition of a large test setup, optional patching, and incremental refactoring. Bottom line: Instead of global, heavy-handed changes, use narrow, deliberate, modular test setups.

bandirevanth avatar May 30 '25 10:05 bandirevanth

Solution/Workaround

  1. Stop Using Autouse Fixtures for Environment Patching
  2. Scope Patching to Individual Tests or Test Classes
  3. Isolate Test Effects with Context Managers
  4. Enhance Thread Safety & Streamline Test Configuration
  5. Validate Parallel Test Runs Early and Often -
    Run tests with pytest -n (pytest-xdist) or pytest-run-parallel frequently.

bandirevanth avatar May 30 '25 10:05 bandirevanth

@bwhitt7 I think a lot of this is covered now by your recent work. Would you be able to update this issue with the current state, and see if/when it can be closed?

rgommers avatar Sep 10 '25 11:09 rgommers