-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix message detection error in ManualTime.expectNoMessageFor #28773
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
|
Test PASSed. |
|
|
||
| assertThrows[AssertionError] { | ||
| manualTime.expectNoMessageFor(10.millis, probe) | ||
| } |
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.
This is tricky because we are dealing with two timelines, the manual timer time, which the duration is about, but then also actual time where the actual actor is running and interacting with the timer.
I'm not quite convinced this change is the right thing to do. @raboof wdyt?
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.
The test looks good.
It's tricky: ideally the change wouldn't be needed, but there's indeed a race between effects that are triggered by the timePasses but scheduled asynchronously (but immediately) so they can't be observed immediately.
In #24243 (comment) we discussed also using a custom/wrapped dispatcher to detect when there's at least no more work scheduled there. Until that time it's probably OK to 'fudge it a bit' and allow for some time to let the dispatchers clear.
The default timeout of expectNoMessage is rather long (3 seconds IIRC?), which rather goes against the idea of tests with a manually controlled clock. Could we make it shorter? And perhaps add a comment pointing to this issue and/or #24243 (comment) so we remember we might want a more elegant solution in the future?
|
@raboof Could you take a new look at this one. To me it seems strange to work around the problem like this. Maybe it's simply not something we can solve now? |
References #28772
It seems that
TestProbefails to poll a message from its queue withreceiveOne_internalmethod below, even though there is actually a message inTestProbemailbox.I found that enqueuing into TestProbe's queue did not make it before messaging polling started.
which leads to message detection result as there is no message during the period.
I changed
expectNoMessageForto get it to call each probe'sexpectNoMessagewith default timeout to successfully get a message.