Deprecate `cleanup` fixture; replace with `gonna_run_dask` fixture

[gjoseph92]

Addresses https://github.com/dask/distributed/pull/6822#discussion_r940276267, where some tests were using the cleanup fixture for its behavior setting default config values (such as disabling the profiler).

I didn't like that muddying of concerns. cleanup should just assert that things aren't leaked. If you want config values, that should be separate IMO.

cleanup seemed to be currently used like "throw this in whenever you're going to launch a dask cluster, can't hurt". I just don't like the naming of that, so this adds a new gonna_run_dask fixture for convenience, which combines cleanup + config_for_cluster_tests (which is exactly what cleanup used to be be before #6822).

This deprecates the cleanup fixture right now and raises an error pointing you to gonna_run_dask. Maybe that's a bad idea? I just didn't see many/any standalone uses of cleanup that actually seemed necessary (either in things that weren't making threads/processes, or already were using loop, which runs cleanup itself).

This would break things for any downstream projects using cleanup directly.

cc @graingert

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

[gjoseph92]

I'm really unsure whether or not I like this. gonna_run_dask generally seems like a good idea, but IDK if cleanup should be deprecated. It's also not clear to me whether anywhere you'd use gonna_run_dask you should just be using other things like loop, cluster, gen_cluster, etc.

The way in which fixtures are used seems very tangled and inconsistent. Maybe we should just put config_for_cluster_tests back inside cleanup and not touch anything, and not try to impose any logical structure.

[github-actions[bot]]

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 31m 10s :stopwatch: + 3m 20s   2 992 tests ±0    2 904 :heavy_check_mark: +2       88 :zzz: ±0  0 :x:  - 2  22 189 runs  ±0  21 139 :heavy_check_mark: ±0  1 050 :zzz: +2  0 :x:  - 2 

Results for commit 63864860. ± Comparison against base commit bf537608.

:recycle: This comment has been updated with latest results.

[fjetter]

The "what fixtures should be deprecated" conversation probably belongs into https://github.com/dask/distributed/issues/6806

I suggest to get started on that one before introducing any deprecations really. We can define the fixtures we want to be public there and then deprecate basically all of utils_test in one go in favor of a public testing module and a private one. The private one can then be iterated on quickly to find a good match for our needs

[graingert]

I don't like the name "gonna_run_dask", however once "cleanup" had this new name it was obvious to me which tests didn't need it, so clearly the new name is better, and I'm happy to go with it if nothing else is suggested, which puts me at a begrudging +1

the problems I have with the name are:

• it seems a bit informal, which is ok,
• it's technically wrong because really it's nothing to do with dask it's specifically about "distributed"

some paints for your bikeshed:

@pytest.fixture
def distributes():
    with _check_clean(), config_for_cluster_tests():
        yield

[ian-r-rose]

I kind of like the informality of gonna_run_dask (though I take your point that gonna_run_distributed is a bit more accurate). Having the more conversational tone is one way of signposting that this is, in some sense, not the usual path for tests. Reminds me a bit of React's dangerouslySetInnerHTML.