-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Stabilize BuildRequest Engine tests #9215
Stabilize BuildRequest Engine tests #9215
Conversation
As I understand it, this stabilizes the tests by waiting for 5 seconds instead of 250 ms but only if the engine status isn't already what's expected, and it has two wait handles: one for an exception and one for expected exit? From a stability perspective, is it meaningfully different than just increasing the sleep time to 5 seconds? |
If the engine status isn't the expected one yet, the signal set OnStatusChanged or OnEngineException is the exact one to stop waiting. If any of the two wait handles receives a signal before waiting timeouts (more than 5 seconds), it stops waiting at once. |
Uh - my eyes and my brain hurt looking on that test class logic :-) Those synchronization events shared between tests 😭 - those can trip baddly once any of the test scenarios goes unexpected path... Anyway - let's forget about that now and lets focus on a happy path now for simplification (that's what we want to stabilize). |
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 would like to see the explicit Reset()
calls to disapper (unless you have explanation why they are needed) - as they are only confusing.
Other than that - it looks good
a1d9648
to
c4700a4
Compare
c4700a4
to
ff550b6
Compare
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.
Thank you!
Fixes #9100 #9277
Context
In BuildRequestEngine_Tests the tests rely on
Thread.Sleep(250)
to wait for the engine status changes. It may cause the tests unstable.In addition, because the status changed AutoResetEvent is not reset after the status change at some places,
WaitForEvent(_engineStatusChangedEvent, "EngineStatusChanged")
insideVerifyEngineStatus(BuildRequestEngineStatus expectedStatus)
couldn't work correctly to wait for the expected status.Changes Made
Instead of
Thread.Sleep(250)
, rely onVerifyEngineStatus(BuildRequestEngineStatus expectedStatus)
to wait. And after the operation that triggers the status change event, make sure the following in order.Testing
N/A
Notes