Skip to content

Automatically configure all tests using autouse fixture#6802

Closed
hendrikmakait wants to merge 4 commits intodask:mainfrom
hendrikmakait:autouse-fixture
Closed

Automatically configure all tests using autouse fixture#6802
hendrikmakait wants to merge 4 commits intodask:mainfrom
hendrikmakait:autouse-fixture

Conversation

@hendrikmakait
Copy link
Copy Markdown
Member

@hendrikmakait hendrikmakait commented Jul 27, 2022 •
edited
Loading

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
Copy Markdown
Contributor

github-actions Bot commented Jul 27, 2022

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
Copy Markdown
Member Author

hendrikmakait commented Jul 27, 2022

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.

jrbourbeau
jrbourbeau reviewed Jul 27, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

hendrikmakait commented Jul 27, 2022

@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
Copy Markdown
Member Author

hendrikmakait commented Jul 27, 2022 •
edited
Loading

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.

@hendrikmakait hendrikmakait mentioned this pull request Jul 27, 2022
Open
@graingert
Copy link
Copy Markdown
Member

graingert commented Aug 2, 2022

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

@hendrikmakait
Copy link
Copy Markdown
Member Author

hendrikmakait commented Aug 3, 2022

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

Reviewers

@jrbourbeau jrbourbeau jrbourbeau left review comments

@graingert graingert Awaiting requested review from graingert

@gjoseph92 gjoseph92 Awaiting requested review from gjoseph92

Assignees

@hendrikmakait hendrikmakait

Labels

tests Unit tests and/or continuous integration

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@hendrikmakait @graingert @jrbourbeau