-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix tests on Windows #5096
Fix tests on Windows #5096
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
waitForHello(bboxWithMinimumDuration); | ||
|
||
assertThat(bboxWithMinimumDuration.isRunning()).isTrue(); | ||
|
||
waitForHello(bboxWithMinimumDuration); |
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 think it was wrong previously: once waitForHello
returns, sleep && echo
would complete and the container should stop. So it is wrong to expect isRunning()
to return true
here.
It worked fine before on Linux probably because the check was faster than containers' cleanup but I got a flaky behaviour on Windows
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 think you are right. Do you remember your original idea here @rnorth?
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 wasn't the original author, but it looks to me as though the original version was indeed prone to a race condition. It's quite surprising that it's taken so long for this to be detected.
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.
Very happy to further polish our Windows support (even if in this case, this is not about actual implementation code, but just making our own tests more portable).
waitForHello(bboxWithMinimumDuration); | ||
|
||
assertThat(bboxWithMinimumDuration.isRunning()).isTrue(); | ||
|
||
waitForHello(bboxWithMinimumDuration); |
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 think you are right. Do you remember your original idea here @rnorth?
No description provided.