Skip to content

Restructure test_watch_requires_lock_to_run to avoid flakes#6469

Merged
mrocklin merged 3 commits intodask:mainfrom
hendrikmakait:fix-flaky-watch-block-test
May 31, 2022
Merged

Restructure test_watch_requires_lock_to_run to avoid flakes#6469
mrocklin merged 3 commits intodask:mainfrom
hendrikmakait:fix-flaky-watch-block-test

Conversation

@hendrikmakait
Copy link
Copy Markdown
Member

distributed/tests/test_profile.py fails occasionally (e.g., see #6444 (comment)). This test restructures the test to avoid timing-based flakes.

Closes #xxxx

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

log = watch(interval="10ms", cycle="50ms", stop=stop_profile)
log = watch(interval="10ms", cycle="50ms", stop=stop_profiling)

start = time() # wait until thread starts up
Copy link
Copy Markdown
Member

@graingert graingert May 27, 2022

Choose a reason for hiding this comment

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

how about using an event to wait for the profiler thread to start, and grabbing the profile_thread in a nonlocal?

stop_profiling_called = threading.Event()
profile_thread = None

def stop_profiling():
    nonlocal profile_thread
    profile_thread = threading.current_thread()
    stop_profiling_called.set()

    return time() > start + 0.500

stop_profiling_called.wait(2)
sleep(0.5)
assert len(log) == 0
release_lock = True

profile_thread.join(2)
thread.join(2)

Copy link
Copy Markdown
Member Author

@hendrikmakait hendrikmakait May 27, 2022

Choose a reason for hiding this comment

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

Instead of

start = time() # wait until thread starts up
while threading.active_count() < start_threads + 2:
assert time() < start + 2
sleep(0.01)
you mean? [The edit made it clearer]. Good idea, I just stole that logic from test_watch, so I'll adjust both of them for more reliable behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, adjusted the structure to be more event-based.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 27, 2022

Unit Test Results

       15 files  ±    0         15 suites  ±0   5h 43m 1s ⏱️ - 50m 6s
  2 821 tests +    2    2 736 ✔️ +    9    82 💤 +  1  3  - 8 
20 595 runs   - 301  19 675 ✔️  - 270  915 💤  - 24  5  - 7 

For more details on these failures, see this check.

Results for commit 06500d4. ± Comparison against base commit 1346671.

♻️ This comment has been updated with latest results.

@hendrikmakait
Copy link
Copy Markdown
Member Author

Failures are unrelated.

@mrocklin
Copy link
Copy Markdown
Member

@graingert do you approve (I'm not reviewing this closely)

@mrocklin mrocklin merged commit c2b28cf into dask:main May 31, 2022
crusaderky pushed a commit to crusaderky/distributed that referenced this pull request Jun 1, 2022
)

distributed/tests/test_profile.py fails occasionally (e.g., see dask#6444 (comment)). This test restructures the test to avoid timing-based flakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants