Skip to content

21873 flaky tests #21988

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

21873 flaky tests #21988

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea811a4
Use Awaitility instead of Thread.sleep
tszmytka Jun 16, 2020
153390d
Merge branch 'master' into 21873-flaky-tests
tszmytka Jun 16, 2020
701b63f
Format properly
tszmytka Jun 17, 2020
a0792cc
Format properly
tszmytka Jun 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions ...ols/src/test/java/org/springframework/boot/devtools/livereload/LiveReloadServerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.springframework.web.socket.handler.TextWebSocketHandler;

import static org.assertj.core.api.Assertions.assertThat;
import static org.awaitility.Awaitility.await;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
Expand Down Expand Up @@ -88,19 +89,16 @@ void servesLivereloadJs() throws Exception {
void triggerReload() throws Exception {
LiveReloadWebSocketHandler handler = connect();
this.server.triggerReload();
Thread.sleep(200);
this.server.stop();
Copy link
Member

@wilkinsona wilkinsona Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep this call to stop().

Copy link
Contributor Author

@tszmytka tszmytka Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@wilkinsona wilkinsona Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assertThat(handler.getMessages().get(0)).contains("http://livereload.com/protocols/official-7");
assertThat(handler.getMessages().get(1)).contains("command\":\"reload\"");
await().atMost(Duration.ofSeconds(1)).until(handler::getMessages,
(msgs) -> msgs.get(0).contains("http://livereload.com/protocols/official-7")
&& msgs.get(1).contains("command\":\"reload\""));
}

@Test
void pingPong() throws Exception {
LiveReloadWebSocketHandler handler = connect();
handler.sendMessage(new PingMessage());
Thread.sleep(200);
assertThat(handler.getPongCount()).isEqualTo(1);
this.server.stop();
Copy link
Member

@wilkinsona wilkinsona Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep this call to stop().

Copy link
Contributor Author

@tszmytka tszmytka Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same situation as above.

await().atMost(Duration.ofSeconds(1)).until(handler::getPongCount, is(1));
}

@Test
Expand All @@ -119,8 +117,7 @@ private void awaitClosedException() throws InterruptedException {
void serverClose() throws Exception {
LiveReloadWebSocketHandler handler = connect();
this.server.stop();
Thread.sleep(200);
assertThat(handler.getCloseStatus().getCode()).isEqualTo(1006);
await().atMost(Duration.ofSeconds(1)).until(() -> handler.getCloseStatus().getCode(), is(1006));
}

private LiveReloadWebSocketHandler connect() throws Exception {
Expand Down