-
Notifications
You must be signed in to change notification settings - Fork 2k
Issue #13543 fix flakey TestStopMojo #13642
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
Issue #13543 fix flakey TestStopMojo #13642
Conversation
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.
LGTM.
I just added some variant using Optional
but that's really a question of style :)
So it's really up to you to change to this or not
...tty-ee9-maven-plugin/src/test/java/org/eclipse/jetty/ee9/maven/plugin/TestJettyStopMojo.java
Outdated
Show resolved
Hide resolved
...tty-ee9-maven-plugin/src/test/java/org/eclipse/jetty/ee9/maven/plugin/TestJettyStopMojo.java
Outdated
Show resolved
Hide resolved
...tty-ee9-maven-plugin/src/test/java/org/eclipse/jetty/ee9/maven/plugin/TestJettyStopMojo.java
Outdated
Show resolved
Hide resolved
...tty-ee9-maven-plugin/src/test/java/org/eclipse/jetty/ee9/maven/plugin/TestJettyStopMojo.java
Outdated
Show resolved
Hide resolved
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.
sadly we have to duplicate this
Done, ready for re-review |
@olamy can you rereview pls? |
...y-ee10-maven-plugin/src/test/java/org/eclipse/jetty/ee10/maven/plugin/TestJettyStopMojo.java
Outdated
Show resolved
Hide resolved
Awaitility.await().until(shutdownService::isListening); | ||
|
||
//wait forever until our shutdown service stops this process | ||
while (true) |
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.
why is it different in 10/11 and here?
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.
Ooops, forgot to do this in ee10
and ee11
. Fixed now
Ready for re-re-review :) |
Speculative fix for #13543.
@olamy if you like the approach I've done for
ee9
I'll do it for the other environments too.