[Data][Test] make test_hanging_detector_detects_issues deterministic #59060
Conversation
Signed-off-by: machichima <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the test_hanging_detector_detects_issues test deterministic by mocking the operator and avoiding time.sleep. The changes are well-implemented and achieve the goal of creating a more reliable test. I've suggested a small refactoring to the test to make it even more robust by mocking time.perf_counter, which avoids depending on the internal state of the detector.
Signed-off-by: machichima <[email protected]>
|
Hi @bveeramani , |
ray-gardener
bot
added
train
machichima
changed the title
owenowenisme
added
data
|
Thank you @machichima! I'm on vacation until the 7th. I'll take a look as soon as I'm back! |
omatthew98
left a comment
omatthew98
left a comment
There was a problem hiding this comment.
Overall lgtm besides the redefinition of the fixture, once you fix that I will approve / get this merged. Thanks!
python/ray/data/tests/conftest.py
Outdated
| DataContext._set_current(copy) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") |
There was a problem hiding this comment.
This fixture should already exist in python/ray/tests/conftest.py, let's use that fixture rather than redefining here.
There was a problem hiding this comment.
Removed. Thank you!
Signed-off-by: machichima <[email protected]>
omatthew98
left a comment
omatthew98
left a comment
There was a problem hiding this comment.
Thanks, I will flag to the team for merge!
5 participants
Description
Follow-up on #58852
Implementing the third step described in: #58630 (comment)
Mocking the operator to make
test_hanging_detector_detects_issuestest deterministic, prevent it depending on timing variation.Related issues
Closes #58562
Additional information