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

Automatically configure all tests using autouse fixture #6802

Closed
wants to merge 4 commits into from

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Jul 27, 2022

Closes #6498

This PR effectively turns _reconfigure into an autouse fixture that resets the configuration for each test. As a caveat, in some tests _reconfigure() is now run twice, but this should not cause any issues.

  • Tests added / passed
  • Passes pre-commit run --all-files

@hendrikmakait hendrikmakait self-assigned this Jul 27, 2022
@hendrikmakait hendrikmakait added the tests Unit tests and/or continuous integration label Jul 27, 2022
@hendrikmakait hendrikmakait marked this pull request as ready for review July 27, 2022 15:05
@github-actions
Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 44m 31s ⏱️ + 32m 51s
  2 977 tests ±0    2 885 ✔️ +1       88 💤 +1  4  - 1 
22 071 runs   - 1  21 036 ✔️ +2  1 031 💤 +2  4  - 4 

For more details on these failures, see this check.

Results for commit 20dcf92. ± Comparison against base commit 236945a.

@hendrikmakait
Copy link
Member Author

The flake by distributed/deploy/tests/test_slow_adaptive.py::test_startup seems to be related to #6796. From what I can tell, we already called _reconfigure for that tesT, so this PR should not have changed anything.
Other failures: #6751, #6752, #6753.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hendrikmakait. It'd be great to have tests automatically configured with a pytest.fixture. I gave this a shot over in #5242 which we ended up reverting in #5300 because it impacted other projects (e.g. like dask/dask) that use testing utilities like gen_cluster. I've not thought too deeply about the implementation here, but wanted to make sure whatever changes we make still allow downstream projects to use gen_cluster, etc.

@hendrikmakait
Copy link
Member Author

@jrbourbeau: The way this is currently implemented, nothing changes for existing fixtures, we simply apply _reconfigure to all tests. For tests that use fixtures that already call _reconfigure, this means that they get wrapped in another _reconfigure() context manager. Since this simply resets the configuration to some static defaults, this should not change the behavior of those tests.

@hendrikmakait
Copy link
Member Author

hendrikmakait commented Jul 27, 2022

As a caveat, if you from distributed.test_utils import *, this will run the autoconfigure fixture and might change the behavior of your tests. Then again, this is bad practice to begin with, so I'd not consider this a problem.

@graingert
Copy link
Member

@hendrikmakait I also opened a PR to detect the profiler in tests in general: #6819

@hendrikmakait
Copy link
Member Author

As described in #6498, this has become superfluous for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Unit tests and/or continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introducing an autouse fixture to (re)set config options for all tests.
3 participants