Synchronize aggregate test runner threads

3 files changed, 55 insertions, 42 deletions

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<TestRunner> wrapped;
-    private final AtomicInteger current = new AtomicInteger(-1);
+    private volatile int current = -1;
+    private final Object monitor = new Object();
 
     private AggregateTestRunner(List<TestRunner> testRunners) {
         this.wrapped = testRunners;
@@ -28,7 +27,7 @@ public class AggregateTestRunner implements TestRunner {
     @Override
     public Collection<LogRecord> getLog(long after) {
         ArrayList<LogRecord> 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 <T> List<T> merged(List<T> first, List<T> 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);