-
-
Notifications
You must be signed in to change notification settings - Fork 750
Simplify test_worker_heartbeat_after_cancel #5067
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
Simplify test_worker_heartbeat_after_cancel #5067
Conversation
| nthreads=[("127.0.0.1", 1) for _ in range(10)], | ||
| # typical runtime just 2-3s but on CI this may increase significantly | ||
| timeout=60, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dacorator can now fit on 1 line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, keeping an additional line reduces diffs if the decorator is ever changed again. I personally also consider this to be easier readable.
The point of having an auto formatter like black running is to not have to argue about formatting. Since black is fine with this, I'll keep it as is
distributed/tests/test_scheduler.py
Outdated
| while not s.tasks: | ||
| await asyncio.sleep(0.001) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant with the following sleep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the sleep after the cancel seems unnecessary; why don't you simply start bombarding the workers with heartbeat requests straight away?
while s.tasks:
await asyncio.sleep(0.001)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while sum(w.executing_count for w in workers) < len(workers) / 2:Why don't you wait for all workers fullstop? (remove the / 2). slowinc should reliably fill the whole cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
9107e8e to
48e4577
Compare
crusaderky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved conditionally to getting green tests
|
github actions seems to be down? |
I believe this just looks funny because there are still tasks of a previous commit to be cancelled It's coming in while typing... |
|
Looking at all actions, this workflow was marked with a clock opposed to a yellow / red / green dot. The checks summary above doesn't seem to pick this up |

Follow up to #5057
Closes #5056
I don't really understand why I wrote the test so complicated yesterday. The issue is very easily reproducible this way. For some reason this was not successful, yesterday 🤷♂️