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
test: don't fail test if scheduled task runs more often than expected #10746
test: don't fail test if scheduled task runs more often than expected #10746
Conversation
If the test thread was blocked for a while, for example due to garbage collection, verifying that the task ran exactly 5 times within 2 seconds would fail because it ran more often than that. With this change we just verify that the task ran at least 5 times within 2 seconds.
@@ -288,7 +288,7 @@ public void shouldScheduleOnFixedRate() { | |||
scheduleService.runAtFixedRate(Duration.ofMillis(10), mockedTask); | |||
|
|||
// then | |||
verify(mockedTask, TIMEOUT.times(5)).execute(any()); | |||
verify(mockedTask, TIMEOUT.atLeast(5)).execute(any()); |
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.
It doesn't look like this test is testing really what it wants. To make it deterministic, one way would be to use ControlledActorScheduler
. Then we can write:
scheduleService.runAtFixedRate(Duration.ofSeconds(10), mockedTask);
actorScheduler.runUntilDone();
// verify it ran at the first interval
clock.addTime(Duration.ofSeconds(10));
actorScheduler.runUntilDone();
verify(mockedTask, times(1)).execute(any());
// verify it was rescheduled for the next interval
clock.addTime(Duration.ofSeconds(10));
actorScheduler.runUntilDone();
verify(mockedTask, times(2)).execute(any());
What do you think?
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.
I interpreted the test more as a "see if runAtFixedRate
works at all" test and not so much as a test that verifies specific timings.
I think it makes sense to test that runAtFixedRate
runs jobs at a rate that is close to the requested rate but that'd be a separate test IMO.
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.
Ok. Then this test is enough. I don't think another test is required. I assume time based scheduling is already tested for actor scheduler.
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.
Thanks 🎉
bors merge |
Build succeeded: |
Successfully created backport PR #10750 for |
If the test thread was blocked for a while, for example due to garbage collection, verifying that the task ran exactly 5 times within 2 seconds would fail because it ran more often than that.
With this change we just verify that the task ran at least 5 times within 2 seconds.
closes #10745