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.
dacorator can now fit on 1 line
There was a problem hiding this comment.
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.
Redundant with the following sleep
There was a problem hiding this comment.
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.
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.
9107e8e to
48e4577
Compare
crusaderky
left a comment
There was a problem hiding this comment.
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 |
|
This is incredibly satisfying ❤️
|
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 🤷♂️