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.

Sign up for GitHub

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

Jump to bottom

use the loop fixture in even more tests #6674

Merged
merged 1 commit into from
Jul 5, 2022
Merged

use the loop fixture in even more tests #6674

merged 1 commit into from
Jul 5, 2022

Conversation

graingert
Copy link
Member

Refs #6163
follow up to #6642
adjustments for tests added since the first round, and addition
of explicit loop=None for cases where the LoopRunner itself
is being tested

Closes #xxxx

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

@graingert
use the loop fixture in even more tests
48150a2 
Refs dask#6163
follow up to dask#6642
adjustments for tests added since the first round, and addition
of explicit loop=None for cases where the LoopRunner itself
is being tested
@graingert graingert marked this pull request as ready for review July 5, 2022 14:39
crusaderky
crusaderky reviewed Jul 5, 2022
View reviewed changes
distributed/tests/test_client.py
@@ -6854,6 +6854,7 @@ def f(x):
processes=False,
dashboard_address=":0",
worker_dashboard_address=":0",
loop=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it better to use the loop_in_thread fixture here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but cloudpickle would attempt to pickle it

Copy link
Member Author

Choose a reason for hiding this comment

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

I could port the fixture to a context manager to use it here

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO it would be overkill

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 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 24m 39s ⏱️ - 7m 44s
  2 917 tests ±0    2 832 ✔️ +2    80 💤 +1  4 ❌  - 1  1 🔥  - 2 
21 598 runs   - 1  20 656 ✔️ +1  935 💤 +2  6 ❌  - 2  1 🔥  - 2 

For more details on these failures and errors, see this check.

Results for commit 48150a2. ± Comparison against base commit 1a28011.

@crusaderky crusaderky merged commit 75f4635 into dask:main Jul 5, 2022
@graingert graingert deleted the use-the-loop-fixture-in-even-more-tests branch July 5, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crusaderky crusaderky crusaderky left review comments

@gjoseph92 gjoseph92 Awaiting requested review from gjoseph92

@hendrikmakait hendrikmakait Awaiting requested review from hendrikmakait

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@graingert @crusaderky