21873 flaky tests #21988
21873 flaky tests #21988
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
wilkinsona
left a comment
wilkinsona
left a comment
There was a problem hiding this comment.
Thanks very much for the pull request. There are a couple of places where a call to server.stop() has been removed. I think they need to be kept. Could you please take a look and, if you agree, update your proposal to add them back in?
| LiveReloadWebSocketHandler handler = connect(); | ||
| this.server.triggerReload(); | ||
| Thread.sleep(200); | ||
| this.server.stop(); |
There was a problem hiding this comment.
I think we need to keep this call to stop().
There was a problem hiding this comment.
The test class already has an @AfterEach which calls this.server.stop();.
I assumed the original author had written those 2 test cases first and only after that decided to have a general @AfterEach method (and maybe forgot to take the individual stop()s out).
I made sure that the tests run fine without them.
Do you think stop() should be kept nevertheless?
There was a problem hiding this comment.
Sorry, I'd missed that when reviewing the diffs in isolation. I agree there's no need to keep the stop() calls. To keep this change clean, I think it makes sense to remove the stop() calls separately. No need for you to do anything though. We can take care of it prior to merging this.
| handler.sendMessage(new PingMessage()); | ||
| Thread.sleep(200); | ||
| assertThat(handler.getPongCount()).isEqualTo(1); | ||
| this.server.stop(); |
There was a problem hiding this comment.
I think we need to keep this call to stop().
| LiveReloadWebSocketHandler handler = connect(); | ||
| this.server.triggerReload(); | ||
| Thread.sleep(200); | ||
| this.server.stop(); |
There was a problem hiding this comment.
Sorry, I'd missed that when reviewing the diffs in isolation. I agree there's no need to keep the stop() calls. To keep this change clean, I think it makes sense to remove the stop() calls separately. No need for you to do anything though. We can take care of it prior to merging this.
wilkinsona
added
type: task
|
Thanks very much, @tszmytka. |
3 participants
This substitutes
Thread.sleep()with calls to appropriateAwaitilitymethods as described in #21873