Skip to content

Commit 28f4fad

Browse files
snazysnazy
authored
Apprunner: Work around potential flakiness in GH (apache#52)
There's no real "wait" in the futures, but it's still possible that the actual futures run late and cause a test failure.
1 parent e122607 commit 28f4fad

4 files changed

Lines changed: 57 additions & 49 deletions

File tree

‎apprunner/common/build.gradle.kts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,8 @@
1919

2020
plugins { id("polaris-apprunner-java") }
2121

22-
dependencies { compileOnly(libs.jakarta.annotation.api) }
22+
dependencies {
23+
compileOnly(libs.jakarta.annotation.api)
24+
25+
testImplementation(libs.junit.pioneer)
26+
}

‎apprunner/common/src/test/java/org/apache/polaris/apprunner/common/TestListenUrlWaiter.java‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
*/
1919
package org.apache.polaris.apprunner.common;
2020

21+
import static org.assertj.core.api.Assertions.assertThat;
22+
2123
import java.util.concurrent.ExecutionException;
2224
import java.util.concurrent.ExecutorService;
2325
import java.util.concurrent.Executors;
@@ -30,9 +32,9 @@
3032
import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension;
3133
import org.junit.jupiter.api.AfterAll;
3234
import org.junit.jupiter.api.BeforeAll;
33-
import org.junit.jupiter.api.RepeatedTest;
3435
import org.junit.jupiter.api.Test;
3536
import org.junit.jupiter.api.extension.ExtendWith;
37+
import org.junitpioneer.jupiter.RetryingTest;
3638

3739
@ExtendWith(SoftAssertionsExtension.class)
3840
class TestListenUrlWaiter {
@@ -99,28 +101,30 @@ void ioHandling() {
99101
.containsExactly("https://localhost.in.some.space:4242", null);
100102
}
101103

102-
@RepeatedTest(20) // repeat, risk of flakiness
104+
@RetryingTest(maxAttempts = 50, minSuccess = 20)
105+
// repeat, risk of flakiness
103106
void timeout() {
104107
var clock = new AtomicLong();
105108
var timeout = 10_000L; // long timeout, for slow CI
106109

107110
var line = new AtomicReference<>();
108111
var waiter = new ListenUrlWaiter(clock::get, timeout, line::set);
109112

110-
soft.assertThat(waiter.isTimeout()).isFalse();
113+
assertThat(waiter.isTimeout()).isFalse();
111114

112115
clock.set(TimeUnit.MILLISECONDS.toNanos(timeout + 1));
113116

114-
soft.assertThat(waiter.isTimeout()).isTrue();
117+
assertThat(waiter.isTimeout()).isTrue();
115118

116-
soft.assertThat(executor.submit(waiter::getListenUrls))
119+
assertThat(executor.submit(waiter::getListenUrls))
117120
.failsWithin(5, TimeUnit.SECONDS)
118121
.withThrowableOfType(ExecutionException.class)
119122
.withRootCauseExactlyInstanceOf(TimeoutException.class)
120123
.withMessageEndingWith(ListenUrlWaiter.TIMEOUT_MESSAGE + ListenUrlWaiter.NOTHING_RECEIVED);
121124
}
122125

123-
@RepeatedTest(20) // repeat, risk of flakiness
126+
@RetryingTest(maxAttempts = 50, minSuccess = 20)
127+
// repeat, risk of flakiness
124128
void noTimeout() throws Exception {
125129
var clock = new AtomicLong();
126130
var timeout = 10_000L; // long timeout, for slow CI
@@ -137,15 +141,15 @@ void noTimeout() throws Exception {
137141
var listenLine =
138142
"2021-05-28 12:12:25,753 INFO [io.quarkus] (main) nessie-quarkus 0.6.2-SNAPSHOT on JVM (powered by Quarkus 1.13.4.Final) started in 1.444s. Listening on: http://4.2.4.2:4242. Management interface listening on http://4.2.4.2:2424.";
139143
waiter.accept(listenLine);
140-
soft.assertThat(line.getAndSet(null)).isEqualTo(listenLine);
141-
soft.assertThat(waiter.getListenUrls())
144+
assertThat(line.getAndSet(null)).isEqualTo(listenLine);
145+
assertThat(waiter.getListenUrls())
142146
.containsExactly("http://4.2.4.2:4242", "http://4.2.4.2:2424");
143-
soft.assertThat(waiter.isTimeout()).isFalse();
147+
assertThat(waiter.isTimeout()).isFalse();
144148

145149
// Clock post the timeout-boundary (so a timeout-check would trigger)
146150
clock.set(TimeUnit.MILLISECONDS.toNanos(timeout + 1));
147-
soft.assertThat(waiter.getListenUrls())
151+
assertThat(waiter.getListenUrls())
148152
.containsExactly("http://4.2.4.2:4242", "http://4.2.4.2:2424");
149-
soft.assertThat(waiter.isTimeout()).isFalse();
153+
assertThat(waiter.isTimeout()).isFalse();
150154
}
151155
}

‎apprunner/common/src/test/java/org/apache/polaris/apprunner/common/TestProcessHandler.java‎

Lines changed: 36 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@
1818
*/
1919
package org.apache.polaris.apprunner.common;
2020

21+
import static java.util.concurrent.TimeUnit.MINUTES;
2122
import static java.util.concurrent.TimeUnit.SECONDS;
23+
import static org.assertj.core.api.Assertions.assertThat;
24+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2225

2326
import java.io.InputStream;
2427
import java.io.OutputStream;
@@ -39,9 +42,9 @@
3942
import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension;
4043
import org.junit.jupiter.api.AfterAll;
4144
import org.junit.jupiter.api.BeforeAll;
42-
import org.junit.jupiter.api.RepeatedTest;
4345
import org.junit.jupiter.api.Test;
4446
import org.junit.jupiter.api.extension.ExtendWith;
47+
import org.junitpioneer.jupiter.RetryingTest;
4548

4649
@ExtendWith(SoftAssertionsExtension.class)
4750
class TestProcessHandler {
@@ -58,7 +61,7 @@ static void createExecutor() {
5861
@AfterAll
5962
static void stopExecutor() throws Exception {
6063
executor.shutdown();
61-
executor.awaitTermination(10, SECONDS);
64+
executor.awaitTermination(5, MINUTES);
6265
}
6366

6467
@Test
@@ -81,7 +84,7 @@ void doubleStart() {
8184
.hasMessage("Process already started");
8285
}
8386

84-
@RepeatedTest(20)
87+
@RetryingTest(maxAttempts = 50, minSuccess = 20)
8588
// repeat, risk of flakiness
8689
void processWithNoOutput() {
8790
var phMock = new ProcessHandlerMock();
@@ -91,17 +94,16 @@ void processWithNoOutput() {
9194
var futureListenUrl = executor.submit(phMock.ph::getListenUrls);
9295

9396
while (phMock.clock.get() < TimeUnit.MILLISECONDS.toNanos(phMock.timeToUrl)) {
94-
soft.assertThat(futureListenUrl).isNotDone();
95-
soft.assertAll();
97+
assertThat(futureListenUrl).isNotDone();
9698
phMock.clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10));
9799
}
98100
// should be exactly at (but not "past") the time to wait for the listen-url now
99101

100102
// bump the clock "past" the listen-url-timeout
101103
phMock.clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10));
102104

103-
soft.assertThat(futureListenUrl)
104-
.failsWithin(5, SECONDS)
105+
assertThat(futureListenUrl)
106+
.failsWithin(5, MINUTES)
105107
.withThrowableOfType(ExecutionException.class) // EE from ForkJoinPool/executor (test code)
106108
.withRootCauseInstanceOf(
107109
TimeoutException.class) // TE from ProcessHandler/ListenUrlWaiter.getListenUrl
@@ -110,10 +112,10 @@ void processWithNoOutput() {
110112
// Need to wait for the watchdog to finish, before we can do any further assertion
111113
phMock.ph.watchdogExitGrace();
112114

113-
soft.assertThat(phMock.ph.isAlive()).isFalse();
115+
assertThat(phMock.ph.isAlive()).isFalse();
114116
}
115117

116-
@RepeatedTest(20)
118+
@RetryingTest(maxAttempts = 50, minSuccess = 20)
117119
// repeat, risk of flakiness
118120
void processExitsEarly() {
119121
var phMock = new ProcessHandlerMock();
@@ -122,14 +124,14 @@ void processExitsEarly() {
122124

123125
var futureListenUrl = executor.submit(phMock.ph::getListenUrls);
124126

125-
soft.assertThat(phMock.ph.isAlive()).isTrue();
126-
soft.assertThatThrownBy(() -> phMock.ph.getExitCode())
127+
assertThat(phMock.ph.isAlive()).isTrue();
128+
assertThatThrownBy(() -> phMock.ph.getExitCode())
127129
.isInstanceOf(IllegalThreadStateException.class);
128130

129131
phMock.exitCode.set(88);
130132

131-
soft.assertThat(futureListenUrl)
132-
.failsWithin(5, SECONDS)
133+
assertThat(futureListenUrl)
134+
.failsWithin(5, MINUTES)
133135
.withThrowableOfType(ExecutionException.class) // EE from ForkJoinPool/executor (test code)
134136
.withMessageEndingWith(
135137
ListenUrlWaiter.TIMEOUT_MESSAGE
@@ -139,11 +141,11 @@ void processExitsEarly() {
139141
// Need to wait for the watchdog to finish, before we can do any further assertion
140142
phMock.ph.watchdogExitGrace();
141143

142-
soft.assertThat(phMock.ph.isAlive()).isFalse();
143-
soft.assertThat(phMock.ph.getExitCode()).isEqualTo(88);
144+
assertThat(phMock.ph.isAlive()).isFalse();
145+
assertThat(phMock.ph.getExitCode()).isEqualTo(88);
144146
}
145147

146-
@RepeatedTest(20)
148+
@RetryingTest(maxAttempts = 50, minSuccess = 20)
147149
// repeat, risk of flakiness
148150
void processLotsOfIoNoListen() throws Exception {
149151
var phMock = new ProcessHandlerMock();
@@ -158,27 +160,25 @@ void processLotsOfIoNoListen() throws Exception {
158160
for (var c : message) {
159161
phMock.stdout.add((byte) c);
160162
}
161-
soft.assertThat(futureListenUrl).isNotDone();
162-
soft.assertAll();
163+
assertThat(futureListenUrl).isNotDone();
163164
phMock.clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10));
164165
}
165166
// should be exactly at (but not "past") the time to wait for the listen-url now
166167

167-
soft.assertThat(phMock.ph.remainingWaitTimeNanos()).isEqualTo(0);
168-
soft.assertThat(phMock.ph.isAlive()).isTrue();
168+
assertThat(phMock.ph.remainingWaitTimeNanos()).isEqualTo(0);
169+
assertThat(phMock.ph.isAlive()).isTrue();
169170

170171
var timeoutFail = System.currentTimeMillis() + SECONDS.toMillis(10);
171172
while (!phMock.stdout.isEmpty()) {
172-
soft.assertThat(System.currentTimeMillis() < timeoutFail).isTrue();
173-
soft.assertAll();
173+
assertThat(System.currentTimeMillis() < timeoutFail).isTrue();
174174
Thread.sleep(1L);
175175
}
176176

177177
// bump the clock "past" the listen-url-timeout
178178
phMock.clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10));
179179

180-
soft.assertThat(futureListenUrl)
181-
.failsWithin(5, SECONDS)
180+
assertThat(futureListenUrl)
181+
.failsWithin(5, MINUTES)
182182
.withThrowableOfType(ExecutionException.class) // EE from ForkJoinPool/executor (test code)
183183
.withRootCauseInstanceOf(
184184
TimeoutException.class) // TE from ProcessHandler/ListenUrlWaiter.getListenUrl
@@ -189,13 +189,13 @@ void processLotsOfIoNoListen() throws Exception {
189189
// Need to wait for the watchdog to finish, before we can do any further assertion
190190
phMock.ph.watchdogExitGrace();
191191

192-
soft.assertThat(phMock.ph.isAlive()).isFalse();
193-
soft.assertThat(phMock.ph.getExitCode()).isGreaterThanOrEqualTo(0);
192+
assertThat(phMock.ph.isAlive()).isFalse();
193+
assertThat(phMock.ph.getExitCode()).isGreaterThanOrEqualTo(0);
194194

195-
soft.assertThat(phMock.stdoutLines).hasSize((int) (phMock.timeToUrl / 10));
195+
assertThat(phMock.stdoutLines).hasSize((int) (phMock.timeToUrl / 10));
196196
}
197197

198-
@RepeatedTest(20)
198+
@RetryingTest(maxAttempts = 50, minSuccess = 20)
199199
// repeat, risk of flakiness
200200
void processLotsOfIoProperListenUrl() {
201201
var phMock = new ProcessHandlerMock();
@@ -208,8 +208,7 @@ void processLotsOfIoProperListenUrl() {
208208
for (var c : "Hello world\n".toCharArray()) {
209209
phMock.stdout.add((byte) c);
210210
}
211-
soft.assertThat(futureListenUrl).isNotDone();
212-
soft.assertAll();
211+
assertThat(futureListenUrl).isNotDone();
213212
phMock.clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10));
214213
}
215214
// should be exactly at (but not "past") the time to wait for the listen-url now
@@ -221,21 +220,21 @@ void processLotsOfIoProperListenUrl() {
221220
// bump the clock "past" the listen-url-timeout
222221
phMock.clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10));
223222

224-
soft.assertThat(futureListenUrl)
225-
.succeedsWithin(5, SECONDS)
223+
assertThat(futureListenUrl)
224+
.succeedsWithin(5, MINUTES)
226225
.isEqualTo(Arrays.asList("http://0.0.0.0:4242", null));
227226

228-
soft.assertThat(phMock.ph.isAlive()).isTrue();
229-
soft.assertThatThrownBy(() -> phMock.ph.getExitCode())
227+
assertThat(phMock.ph.isAlive()).isTrue();
228+
assertThatThrownBy(() -> phMock.ph.getExitCode())
230229
.isInstanceOf(IllegalThreadStateException.class);
231230

232231
// The .stop() waits until the watchdog has finished its work
233232
phMock.ph.stop();
234233

235-
soft.assertThat(phMock.ph.isAlive()).isFalse();
236-
soft.assertThat(phMock.ph.getExitCode()).isGreaterThanOrEqualTo(0);
234+
assertThat(phMock.ph.isAlive()).isFalse();
235+
assertThat(phMock.ph.getExitCode()).isGreaterThanOrEqualTo(0);
237236

238-
soft.assertThat(phMock.stdoutLines).hasSize((int) (phMock.timeToUrl / 10 / 2) + 1);
237+
assertThat(phMock.stdoutLines).hasSize((int) (phMock.timeToUrl / 10 / 2) + 1);
239238
}
240239

241240
static final class ProcessHandlerMock {

‎apprunner/gradle/libs.versions.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ maven-core = { module = "org.apache.maven:maven-core", version = "3.9.9" }
2727
maven-plugin-annotations = { module = "org.apache.maven.plugin-tools:maven-plugin-annotations", version = "3.15.1" }
2828
jakarta-annotation-api = { module = "jakarta.annotation:jakarta.annotation-api", version = "3.0.0" }
2929
junit-bom = { module = "org.junit:junit-bom", version = "5.14.1" }
30+
junit-pioneer = { module = "org.junit-pioneer:junit-pioneer", version = "2.3.0" }
3031
soebes-itf-assertj = { module = "com.soebes.itf.jupiter.extension:itf-assertj", version.ref = "soebes-itf" }
3132
soebes-itf-jupiter-extension = { module = "com.soebes.itf.jupiter.extension:itf-jupiter-extension", version.ref = "soebes-itf" }
3233

0 commit comments

Comments
 (0)