Skip to content

Commit 780b637

Browse files
BrandonArpBrandonArp
authored
fix flakey tests (#729)
* additional debugging for flaky tests * remove some mock usage, fix import * fix some date math
1 parent 13020da commit 780b637

File tree

10 files changed

+164
-39
lines changed

10 files changed

+164
-39
lines changed

‎app/com/arpnetworking/metrics/portal/reports/impl/chrome/BaseScreenshotRenderer.java‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -120,16 +120,34 @@ public ImmutableList<Problem> validateRender(final S source, final F format) {
120120
});
121121

122122
result.whenComplete((x, e) -> {
123-
LOGGER.debug()
124-
.setMessage("rendering completed")
125-
.addData("source", source)
126-
.addData("format", format)
127-
.addData("timeRange", timeRange)
128-
.addData("result", x)
129-
.setThrowable(e)
130-
.log();
131-
navigate.cancel(true);
132-
dts.close();
123+
try {
124+
if (e != null) {
125+
LOGGER.error()
126+
.setMessage("rendering failed")
127+
.addData("source", source)
128+
.addData("format", format)
129+
.addData("timeRange", timeRange)
130+
.setThrowable(e)
131+
.log();
132+
} else {
133+
LOGGER.debug()
134+
.setMessage("rendering completed")
135+
.addData("source", source)
136+
.addData("format", format)
137+
.addData("timeRange", timeRange)
138+
.addData("result", x)
139+
.log();
140+
}
141+
navigate.cancel(true);
142+
} finally {
143+
dts.close();
144+
LOGGER.debug()
145+
.setMessage("devtools service closed")
146+
.addData("source", source)
147+
.addData("format", format)
148+
.addData("timeRange", timeRange)
149+
.log();
150+
}
133151
});
134152

135153
TIMEOUT_SERVICE.schedule(() -> result.cancel(true), timeout.toNanos(), TimeUnit.NANOSECONDS);

‎app/com/arpnetworking/metrics/portal/reports/impl/chrome/HtmlScreenshotRenderer.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package com.arpnetworking.metrics.portal.reports.impl.chrome;
1818

1919
import com.arpnetworking.metrics.portal.reports.RenderedReport;
20+
import com.arpnetworking.steno.Logger;
21+
import com.arpnetworking.steno.LoggerFactory;
2022
import jakarta.inject.Inject;
2123
import models.internal.TimeRange;
2224
import models.internal.impl.HtmlReportFormat;
@@ -32,6 +34,7 @@
3234
* @author Spencer Pearson (spencerpearson at dropbox dot com)
3335
*/
3436
public final class HtmlScreenshotRenderer extends BaseScreenshotRenderer<WebPageReportSource, HtmlReportFormat> {
37+
private static final Logger LOGGER = LoggerFactory.getLogger(HtmlScreenshotRenderer.class);
3538

3639
@Override
3740
protected boolean getIgnoreCertificateErrors(final WebPageReportSource source) {
@@ -51,6 +54,13 @@ protected URI getUri(final WebPageReportSource source) {
5154
final TimeRange timeRange,
5255
final B builder
5356
) {
57+
LOGGER.debug()
58+
.setMessage("Rendering page content to HTML")
59+
.addData("source", source)
60+
.addData("format", format)
61+
.addData("timeRange", timeRange)
62+
.addData("builder", builder)
63+
.log();
5464
return devToolsService.evaluate("document.documentElement.outerHTML").thenApply(
5565
jsObject -> builder.setBytes(((String) jsObject).getBytes(StandardCharsets.UTF_8))
5666
);

‎app/com/arpnetworking/metrics/portal/reports/impl/chrome/PdfScreenshotRenderer.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package com.arpnetworking.metrics.portal.reports.impl.chrome;
1818

1919
import com.arpnetworking.metrics.portal.reports.RenderedReport;
20+
import com.arpnetworking.steno.Logger;
21+
import com.arpnetworking.steno.LoggerFactory;
2022
import jakarta.inject.Inject;
2123
import models.internal.TimeRange;
2224
import models.internal.impl.PdfReportFormat;
@@ -31,6 +33,7 @@
3133
* @author Spencer Pearson (spencerpearson at dropbox dot com)
3234
*/
3335
public final class PdfScreenshotRenderer extends BaseScreenshotRenderer<WebPageReportSource, PdfReportFormat> {
36+
private static final Logger LOGGER = LoggerFactory.getLogger(HtmlScreenshotRenderer.class);
3437

3538
@Override
3639
protected boolean getIgnoreCertificateErrors(final WebPageReportSource source) {
@@ -50,6 +53,13 @@ protected URI getUri(final WebPageReportSource source) {
5053
final TimeRange timeRange,
5154
final B builder
5255
) {
56+
LOGGER.debug()
57+
.setMessage("Rendering page content to HTML")
58+
.addData("source", source)
59+
.addData("format", format)
60+
.addData("timeRange", timeRange)
61+
.addData("builder", builder)
62+
.log();
5363
return devToolsService.printToPdf(format.getWidthInches(), format.getHeightInches()).thenApply(builder::setBytes);
5464
}
5565

‎app/com/arpnetworking/metrics/portal/reports/impl/chrome/grafana/BaseGrafanaScreenshotRenderer.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,12 @@ public URI getUri(final GrafanaReportPanelReportSource source) {
8080
final B builder
8181
) {
8282
final CompletableFuture<B> result = new CompletableFuture<>();
83+
LOGGER.debug()
84+
.setMessage("waiting for reportrendered event")
85+
.addData("source", source)
86+
.addData("format", format)
87+
.addData("timeRange", timeRange)
88+
.log();
8389

8490
final CompletableFuture<?> successFuture = devToolsService.nowOrOnEvent(
8591
"reportrendered",

‎app/com/arpnetworking/metrics/portal/scheduling/impl/PeriodicSchedule.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ private PeriodicSchedule(final Builder builder) {
6464
@Override
6565
protected Optional<Instant> unboundedNextRun(final Optional<Instant> lastRun) {
6666
final Instant untruncatedNextRun = lastRun
67-
.map(run -> run.plus(_periodCount, _period))
67+
.map(run -> _period.addTo(run, _periodCount))
6868
.orElseGet(this::getRunAtAndAfter);
6969

7070
try {

‎test/java/com/arpnetworking/metrics/portal/reports/impl/chrome/BaseScreenshotRendererTest.java‎

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
import com.arpnetworking.metrics.portal.TestBeanFactory;
1919
import com.arpnetworking.metrics.portal.reports.RenderedReport;
2020
import com.arpnetworking.metrics.portal.reports.impl.testing.MockRenderedReportBuilder;
21+
import com.arpnetworking.steno.Logger;
22+
import com.arpnetworking.steno.LoggerFactory;
2123
import com.google.common.collect.ImmutableList;
2224
import com.google.common.collect.ImmutableMap;
2325
import com.google.common.collect.ImmutableSet;
@@ -36,6 +38,9 @@
3638
import java.net.URI;
3739
import java.time.Duration;
3840
import java.util.concurrent.CompletableFuture;
41+
import java.util.concurrent.ExecutionException;
42+
import java.util.concurrent.TimeUnit;
43+
import java.util.concurrent.TimeoutException;
3944

4045
/**
4146
* Tests for {@link BaseScreenshotRenderer}.
@@ -49,6 +54,7 @@ public class BaseScreenshotRendererTest extends BaseChromeTestSuite {
4954
@Mock
5055
private DevToolsService _dts;
5156

57+
private static final Logger LOGGER = LoggerFactory.getLogger(BaseScreenshotRendererTest.class);
5258
private static final String SECRET_TOKEN = "SECRET TOKEN";
5359
private static final PerOriginConfigs ORIGIN_CONFIGS = new PerOriginConfigs.Builder()
5460
.setByOrigin(ImmutableMap.of("http://allowed.com", new OriginConfig.Builder()
@@ -83,14 +89,15 @@ public void tearDown() {
8389
private MockRenderer createMockRenderer() {
8490
return createMockRenderer(new CompletableFuture<>());
8591
}
92+
@SuppressWarnings("unchecked")
8693
private MockRenderer createMockRenderer(final CompletableFuture<?> complete) {
87-
return new MockRenderer(_factory, complete);
94+
return new MockRenderer(_factory, (CompletableFuture<MockRenderedReportBuilder>) complete);
8895
}
8996

9097
@Test
9198
public void testClosesDevToolsWhenComplete() {
9299
final MockRenderer renderer = createMockRenderer();
93-
renderer.getComplete().complete(null); // make the render complete immediately
100+
renderer.getComplete().complete(new MockRenderedReportBuilder()); // make the render complete immediately
94101
renderer.render(
95102
TestBeanFactory.createWebPageReportSourceBuilder().build(),
96103
new HtmlReportFormat.Builder().build(),
@@ -103,30 +110,31 @@ public void testClosesDevToolsWhenComplete() {
103110

104111
@Test
105112
public void testClosesDevToolsOnTimeoutWhileRendering() {
106-
final MockRenderer renderer = createMockRenderer(Mockito.mock(CompletableFuture.class));
113+
final MockRenderer renderer = createMockRenderer(new CompletableFuture<>());
107114
renderer.render(
108115
TestBeanFactory.createWebPageReportSourceBuilder().build(),
109116
new HtmlReportFormat.Builder().build(),
110117
DEFAULT_TIME_RANGE,
111118
Mockito.mock(MockRenderedReportBuilder.class),
112119
Duration.ofMillis(500)
113120
);
114-
Mockito.verify(renderer.getComplete(), Mockito.timeout(500)).thenApply(Mockito.any());
115121
Mockito.verify(_dts, Mockito.timeout(1000).atLeastOnce()).close();
116122
}
117123

118124
@Test
119125
public void testClosesDevToolsOnCancelWhileRendering() {
120-
final MockRenderer renderer = createMockRenderer(Mockito.mock(CompletableFuture.class));
126+
final MockRenderer renderer = createMockRenderer(new CompletableFuture<>());
121127
final CompletableFuture<?> future = renderer.render(
122128
TestBeanFactory.createWebPageReportSourceBuilder().build(),
123129
new HtmlReportFormat.Builder().build(),
124130
DEFAULT_TIME_RANGE,
125131
Mockito.mock(MockRenderedReportBuilder.class),
126132
Duration.ofDays(1)
127133
);
128-
Mockito.verify(renderer.getComplete(), Mockito.timeout(500)).thenApply(Mockito.any());
129-
future.cancel(true);
134+
try {
135+
future.cancel(true);
136+
renderer.getComplete().get(500, TimeUnit.MILLISECONDS);
137+
} catch (final TimeoutException | ExecutionException | InterruptedException ignored) { }
130138
Mockito.verify(_dts, Mockito.timeout(1000).atLeastOnce()).close();
131139
}
132140

@@ -200,21 +208,35 @@ protected URI getUri(final WebPageReportSource source) {
200208
final TimeRange timeRange,
201209
final B builder
202210
) {
203-
return _complete.thenApply(anything -> builder.setBytes(new byte[0]));
211+
LOGGER.debug()
212+
.setMessage("Rendering in mock renderer")
213+
.addData("source", source)
214+
.addData("format", format)
215+
.addData("timeRange", timeRange)
216+
.addData("builder", builder)
217+
.addData("complete", _complete)
218+
.log();
219+
final CompletableFuture<B> result = _complete.thenApply(anything -> builder.setBytes(new byte[0]));
220+
LOGGER.debug()
221+
.setMessage("Rendering in mock renderer callback complete")
222+
.addData("result", result)
223+
.log();
224+
225+
return result;
204226
}
205227

206-
CompletableFuture<?> getComplete() {
228+
CompletableFuture<MockRenderedReportBuilder> getComplete() {
207229
return _complete;
208230
}
209231

210232
MockRenderer(
211233
final DevToolsFactory factory,
212-
final CompletableFuture<?> complete
234+
final CompletableFuture<MockRenderedReportBuilder> complete
213235
) {
214236
super(factory);
215237
_complete = complete;
216238
}
217239

218-
private final CompletableFuture<?> _complete;
240+
private final CompletableFuture<MockRenderedReportBuilder> _complete;
219241
}
220242
}

‎test/java/com/arpnetworking/metrics/portal/reports/impl/chrome/HtmlScreenshotRendererTest.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@
2222
import models.internal.impl.HtmlReportFormat;
2323
import models.internal.impl.WebPageReportSource;
2424
import org.junit.Test;
25-
import org.mockito.ArgumentCaptor;
26-
import org.mockito.Mockito;
2725

2826
import java.net.URI;
2927
import java.nio.charset.StandardCharsets;
@@ -47,7 +45,7 @@ public class HtmlScreenshotRendererTest extends BaseChromeTestSuite {
4745

4846
@Test(timeout = 60000)
4947
public void testRendering() throws Exception {
50-
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);
48+
final MockRenderedReportBuilder builder = new MockRenderedReportBuilder();
5149

5250
_wireMock.givenThat(
5351
get(urlEqualTo("/"))
@@ -72,15 +70,13 @@ public void testRendering() throws Exception {
7270

7371
stage.get();
7472

75-
final ArgumentCaptor<byte[]> bytes = ArgumentCaptor.forClass(byte[].class);
76-
Mockito.verify(builder).setBytes(bytes.capture());
77-
final String response = Strings.stringFromBytes(bytes.getValue(), StandardCharsets.UTF_8);
73+
final String response = Strings.stringFromBytes(builder.getBytes(), StandardCharsets.UTF_8);
7874
assertTrue(response.contains("here are some bytes"));
7975
}
8076

8177
@Test(timeout = 60000)
8278
public void testObeysOriginConfig() throws Exception {
83-
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);
79+
final MockRenderedReportBuilder builder = new MockRenderedReportBuilder();
8480

8581
_wireMock.givenThat(
8682
get(urlEqualTo("/allowed-page"))
@@ -107,7 +103,7 @@ public void testObeysOriginConfig() throws Exception {
107103
format,
108104
DEFAULT_TIME_RANGE,
109105
builder,
110-
DEFAULT_TIMEOUT.multipliedBy(1000)
106+
DEFAULT_TIMEOUT
111107
);
112108

113109
stage.get();

‎test/java/com/arpnetworking/metrics/portal/reports/impl/chrome/grafana/HtmlScreenshotRendererTest.java‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@
2525
import org.hamcrest.Matchers;
2626
import org.junit.Assert;
2727
import org.junit.Test;
28-
import org.mockito.ArgumentCaptor;
29-
import org.mockito.Mockito;
3028

3129
import java.net.URI;
3230
import java.nio.charset.StandardCharsets;
@@ -50,7 +48,7 @@
5048
public class HtmlScreenshotRendererTest extends BaseChromeTestSuite {
5149

5250
private void runTestWithRenderDelay(final Duration renderDelay) throws Exception {
53-
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);
51+
final MockRenderedReportBuilder builder = new MockRenderedReportBuilder();
5452

5553
_wireMock.givenThat(
5654
get(urlEqualTo("/"))
@@ -79,9 +77,7 @@ private void runTestWithRenderDelay(final Duration renderDelay) throws Exception
7977

8078
stage.get();
8179

82-
final ArgumentCaptor<byte[]> bytes = ArgumentCaptor.forClass(byte[].class);
83-
Mockito.verify(builder).setBytes(bytes.capture());
84-
final String response = Strings.stringFromBytes(bytes.getValue(), StandardCharsets.UTF_8);
80+
final String response = Strings.stringFromBytes(builder.getBytes(), StandardCharsets.UTF_8);
8581
assertEquals(response, "content we care about");
8682
}
8783

@@ -97,7 +93,7 @@ public void testDelayedRendering() throws Exception {
9793

9894
@Test(timeout = 60000)
9995
public void testDelayedRenderingFailure() throws Exception {
100-
final MockRenderedReportBuilder builder = Mockito.mock(MockRenderedReportBuilder.class);
96+
final MockRenderedReportBuilder builder = new MockRenderedReportBuilder();
10197

10298
_wireMock.givenThat(
10399
get(urlEqualTo("/"))

0 commit comments

Comments
 (0)