From a63453fdb073b22ab234edf133761a52386724d6 Mon Sep 17 00:00:00 2001 From: jonmv Date: Wed, 11 May 2022 13:21:37 +0200 Subject: Synchronize aggregate test runner threads --- .../vespa/testrunner/AggregateTestRunner.java | 85 ++++++++++++---------- .../vespa/testrunner/AggregateTestRunnerTest.java | 9 ++- .../vespa/testrunner/TestRunnerHandlerTest.java | 3 +- 3 files changed, 55 insertions(+), 42 deletions(-) (limited to 'vespa-osgi-testrunner/src') diff --git a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/AggregateTestRunner.java b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/AggregateTestRunner.java index 6e3393b2761..5c54349335b 100644 --- a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/AggregateTestRunner.java +++ b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/AggregateTestRunner.java @@ -4,9 +4,7 @@ package com.yahoo.vespa.testrunner; import java.util.ArrayList; import java.util.Collection; import java.util.List; -import java.util.Objects; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicInteger; import java.util.logging.LogRecord; /** @@ -15,7 +13,8 @@ import java.util.logging.LogRecord; public class AggregateTestRunner implements TestRunner { private final List wrapped; - private final AtomicInteger current = new AtomicInteger(-1); + private volatile int current = -1; + private final Object monitor = new Object(); private AggregateTestRunner(List testRunners) { this.wrapped = testRunners; @@ -28,7 +27,7 @@ public class AggregateTestRunner implements TestRunner { @Override public Collection getLog(long after) { ArrayList records = new ArrayList<>(); - for (int i = 0; i <= current.get() && i < wrapped.size(); i++) + for (int i = 0; i <= current && i < wrapped.size(); i++) records.addAll(wrapped.get(i).getLog(after)); return records; @@ -36,57 +35,67 @@ public class AggregateTestRunner implements TestRunner { @Override public Status getStatus() { - if (current.get() == -1) - return Status.NOT_STARTED; - - Status status = Status.NO_TESTS; - for (int i = 0; i <= current.get(); i++) { - if (i == wrapped.size()) - return status; - - Status next = wrapped.get(i).getStatus(); - status = status.ordinal() < next.ordinal() ? status : next; + synchronized (monitor) { + if (current == -1) + return Status.NOT_STARTED; + + Status status = Status.NO_TESTS; + for (int i = 0; i <= current; i++) { + if (i == wrapped.size()) + return status; + + Status next = wrapped.get(i).getStatus(); + status = status.ordinal() < next.ordinal() ? status : next; + } + return Status.RUNNING; } - return Status.RUNNING; } @Override public CompletableFuture test(Suite suite, byte[] config) { - if (0 <= current.get() && current.get() < wrapped.size()) - throw new IllegalStateException("Tests already running, should not attempt to start now"); - - current.set(-1); - CompletableFuture aggregate = new CompletableFuture<>(); - CompletableFuture vessel = CompletableFuture.completedFuture(null); - runNext(suite, config, vessel, aggregate); - return aggregate; + synchronized (monitor) { + if (0 <= current && current < wrapped.size()) + throw new IllegalStateException("Tests already running, should not attempt to start now"); + + current = -1; + CompletableFuture aggregate = new CompletableFuture<>(); + CompletableFuture vessel = CompletableFuture.completedFuture(null); + runNext(suite, config, vessel, aggregate); + return aggregate; + } } private void runNext(Suite suite, byte[] config, CompletableFuture vessel, CompletableFuture aggregate) { vessel.whenComplete((__, ___) -> { - int next = current.incrementAndGet(); - if (next == wrapped.size()) - aggregate.complete(null); - else - runNext(suite, config, wrapped.get(next).test(suite, config), aggregate); + synchronized (monitor) { + if (++current < wrapped.size()) + runNext(suite, config, wrapped.get(current).test(suite, config), aggregate); + else + aggregate.complete(null); + } }); } @Override public TestReport getReport() { - return wrapped.stream().map(TestRunner::getReport).filter(Objects::nonNull) - .reduce(AggregateTestRunner::merge).orElse(null); + TestReport report = null; + for (int i = 0; i < current && i < wrapped.size(); i++) + report = merge(report, wrapped.get(i).getReport()); + + return report; } static TestReport merge(TestReport first, TestReport second) { - return TestReport.builder() - .withAbortedCount(first.abortedCount + second.abortedCount) - .withFailedCount(first.failedCount + second.failedCount) - .withIgnoredCount(first.ignoredCount + second.ignoredCount) - .withSuccessCount(first.successCount + second.successCount) - .withFailures(merged(first.failures, second.failures)) - .withLogs(merged(first.logLines, second.logLines)) - .build(); + return first == null ? second + : second == null ? first + : TestReport.builder() + .withAbortedCount(first.abortedCount + second.abortedCount) + .withFailedCount(first.failedCount + second.failedCount) + .withIgnoredCount(first.ignoredCount + second.ignoredCount) + .withSuccessCount(first.successCount + second.successCount) + .withFailures(merged(first.failures, second.failures)) + .withLogs(merged(first.logLines, second.logLines)) + .build(); } static List merged(List first, List second) { diff --git a/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/AggregateTestRunnerTest.java b/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/AggregateTestRunnerTest.java index 14e139b3fbf..8bd72d35737 100644 --- a/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/AggregateTestRunnerTest.java +++ b/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/AggregateTestRunnerTest.java @@ -136,8 +136,10 @@ class AggregateTestRunnerTest { .build(); first.report = report; assertSame(report, runner.getReport()); - second.report = report; + assertSame(report, runner.getReport()); + + second.future.complete(null); TestReport merged = runner.getReport(); assertEquals(List.of(record1, record1), merged.logLines); assertEquals(List.of(failure, failure), merged.failures); @@ -145,6 +147,7 @@ class AggregateTestRunnerTest { assertEquals(8, merged.ignoredCount); assertEquals(4, merged.failedCount); assertEquals(2, merged.abortedCount); + } @Test @@ -171,9 +174,9 @@ class AggregateTestRunnerTest { catch (Exception e) { TestReport.trimStackTraces(e, "org.junit.platform.launcher.core.SessionPerRequestLauncher"); assertEquals("java.lang.RuntimeException: java.lang.RuntimeException: inner\n" + - "\tat com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:168)\n" + + "\tat com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:171)\n" + "Caused by: java.lang.RuntimeException: inner\n" + - "\tat com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:165)\n", + "\tat com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:168)\n", ExceptionUtils.getStackTraceAsString(e)); } } diff --git a/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/TestRunnerHandlerTest.java b/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/TestRunnerHandlerTest.java index 49dd2b20797..7b75f1cd4bd 100644 --- a/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/TestRunnerHandlerTest.java +++ b/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/TestRunnerHandlerTest.java @@ -6,6 +6,7 @@ import com.yahoo.component.provider.ComponentRegistry; import com.yahoo.container.jdisc.HttpRequest; import com.yahoo.container.jdisc.HttpResponse; import com.yahoo.test.json.JsonTestHelper; +import com.yahoo.vespa.testrunner.TestRunner.Suite; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -23,7 +24,6 @@ import java.util.stream.Collectors; import static com.yahoo.jdisc.http.HttpRequest.Method.GET; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -57,6 +57,7 @@ class TestRunnerHandlerTest { @Test public void createsCorrectTestReport() throws IOException { + aggregateRunner.test(Suite.SYSTEM_TEST, new byte[0]); HttpResponse response = testRunnerHandler.handle(HttpRequest.createTestRequest("http://localhost:1234/tester/v1/report", GET)); ByteArrayOutputStream out = new ByteArrayOutputStream(); response.render(out); -- cgit v1.2.3