Refactor JUnitRunner for testability

3 files changed, 276 insertions, 129 deletions

diff --git a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java
index 9bbba2d6c2a..c01c9b571e0 100644
--- a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java
+++ b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java
@@ -4,27 +4,21 @@ package com.yahoo.vespa.testrunner;
 import ai.vespa.cloud.Environment;
 import ai.vespa.cloud.SystemInfo;
 import ai.vespa.cloud.Zone;
-import ai.vespa.hosted.api.TestDescriptor;
 import ai.vespa.hosted.cd.InconclusiveTestException;
 import ai.vespa.hosted.cd.internal.TestRuntimeProvider;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.component.annotation.Inject;
-import com.yahoo.io.IOUtils;
 import com.yahoo.jdisc.application.OsgiFramework;
 import com.yahoo.vespa.defaults.Defaults;
 import org.junit.jupiter.engine.JupiterTestEngine;
 import org.junit.platform.engine.discovery.DiscoverySelectors;
-import org.junit.platform.launcher.Launcher;
+import org.junit.platform.launcher.LauncherDiscoveryRequest;
+import org.junit.platform.launcher.TestExecutionListener;
 import org.junit.platform.launcher.core.LauncherConfig;
 import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder;
 import org.junit.platform.launcher.core.LauncherFactory;
 import org.junit.platform.launcher.listeners.SummaryGeneratingListener;
-import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleContext;
 
-import java.io.IOException;
-import java.net.URL;
-import java.nio.charset.Charset;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
@@ -32,10 +26,11 @@ import java.util.SortedMap;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.ExecutionException;
+import java.util.function.BiConsumer;
+import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
-import java.util.stream.Stream;
 
 import static java.util.stream.Collectors.toList;
 
@@ -47,8 +42,9 @@ public class JunitRunner extends AbstractComponent implements TestRunner {
     private static final Logger logger = Logger.getLogger(JunitRunner.class.getName());
 
     private final SortedMap<Long, LogRecord> logRecords = new ConcurrentSkipListMap<>();
-    private final BundleContext bundleContext;
     private final TestRuntimeProvider testRuntimeProvider;
+    private final Function<Suite, List<Class<?>>> classLoader;
+    private final BiConsumer<LauncherDiscoveryRequest, TestExecutionListener[]> testExecutor;
     private volatile CompletableFuture<TestReport> execution;
 
     @Inject
@@ -56,45 +52,21 @@ public class JunitRunner extends AbstractComponent implements TestRunner {
                        JunitTestRunnerConfig config,
                        TestRuntimeProvider testRuntimeProvider,
                        SystemInfo systemInfo) {
-        this.testRuntimeProvider = testRuntimeProvider;
-        this.bundleContext = getUnrestrictedBundleContext(osgiFramework);
-        uglyHackSetCredentialsRootSystemProperty(config, systemInfo.zone());
-    }
-
-    // Hack to retrieve bundle context that allows access to other bundles
-    private static BundleContext getUnrestrictedBundleContext(OsgiFramework framework) {
-        try {
-            BundleContext restrictedBundleContext = framework.bundleContext();
-            var field = restrictedBundleContext.getClass().getDeclaredField("wrapped");
-            field.setAccessible(true);
-            return  (BundleContext) field.get(restrictedBundleContext);
-        } catch (ReflectiveOperationException e) {
-            throw new RuntimeException(e);
-        }
-    }
+        this(testRuntimeProvider,
+             new TestBundleLoader(osgiFramework)::loadTestClasses,
+             (discoveryRequest, listeners) -> LauncherFactory.create(LauncherConfig.builder()
+                                                                                   .addTestEngines(new JupiterTestEngine())
+                                                                                   .build()).execute(discoveryRequest, listeners));
 
-    // TODO(bjorncs|tokle) Propagate credentials root without system property. Ideally move knowledge about path to test-runtime implementations
-    private static void uglyHackSetCredentialsRootSystemProperty(JunitTestRunnerConfig config, Zone zone) {
-        Optional<String> credentialsRoot;
-        if (config.useAthenzCredentials()) {
-            credentialsRoot = Optional.of(Defaults.getDefaults().underVespaHome("var/vespa/sia"));
-        } else if (zone.environment() != Environment.prod){
-            // Only set credentials in non-prod zones where not available
-            credentialsRoot = Optional.of(config.artifactsPath().toString());
-        } else {
-            credentialsRoot = Optional.empty();
-        }
-        credentialsRoot.ifPresent(root -> System.setProperty("vespa.test.credentials.root", root));
+        uglyHackSetCredentialsRootSystemProperty(config, systemInfo.zone());
     }
 
-    private static TestDescriptor.TestCategory toCategory(TestRunner.Suite testProfile) {
-        switch(testProfile) {
-            case SYSTEM_TEST: return TestDescriptor.TestCategory.systemtest;
-            case STAGING_SETUP_TEST: return TestDescriptor.TestCategory.stagingsetuptest;
-            case STAGING_TEST: return TestDescriptor.TestCategory.stagingtest;
-            case PRODUCTION_TEST: return TestDescriptor.TestCategory.productiontest;
-            default: throw new RuntimeException("Unknown test profile: " + testProfile.name());
-        }
+    JunitRunner(TestRuntimeProvider testRuntimeProvider,
+                       Function<Suite, List<Class<?>>> classLoader,
+                       BiConsumer<LauncherDiscoveryRequest, TestExecutionListener[]> testExecutor) {
+        this.classLoader = classLoader;
+        this.testExecutor = testExecutor;
+        this.testRuntimeProvider = testRuntimeProvider;
     }
 
     @Override
@@ -104,19 +76,8 @@ public class JunitRunner extends AbstractComponent implements TestRunner {
         }
         try {
             logRecords.clear();
-            Optional<Bundle> testBundle = findTestBundle();
-            if (testBundle.isEmpty()) {
-                execution = CompletableFuture.completedFuture(null);
-                return execution;
-            }
-
             testRuntimeProvider.initialize(testConfig);
-            Optional<TestDescriptor> testDescriptor = loadTestDescriptor(testBundle.get());
-            if (testDescriptor.isEmpty()) {
-                throw new RuntimeException("Could not find test descriptor");
-            }
-            execution = CompletableFuture.supplyAsync(() -> launchJunit(loadClasses(testBundle.get(), testDescriptor.get(), toCategory(suite)),
-                                                                        suite == Suite.PRODUCTION_TEST));
+            execution = CompletableFuture.supplyAsync(() -> launchJunit(suite));
         } catch (Exception e) {
             execution = CompletableFuture.completedFuture(createReportWithFailedInitialization(e));
         }
@@ -135,84 +96,45 @@ public class JunitRunner extends AbstractComponent implements TestRunner {
                                        .build();
     }
 
-    private Optional<Bundle> findTestBundle() {
-        return Stream.of(bundleContext.getBundles())
-                     .filter(this::isTestBundle)
-                     .findAny();
-    }
-
-    private boolean isTestBundle(Bundle bundle) {
-        var testBundleHeader = bundle.getHeaders().get("X-JDisc-Test-Bundle-Version");
-        return testBundleHeader != null && ! testBundleHeader.isBlank();
-    }
-
-    private Optional<TestDescriptor> loadTestDescriptor(Bundle bundle) {
-        URL resource = bundle.getEntry(TestDescriptor.DEFAULT_FILENAME);
-        TestDescriptor testDescriptor;
-        try {
-            var jsonDescriptor = IOUtils.readAll(resource.openStream(), Charset.defaultCharset()).trim();
-            testDescriptor = TestDescriptor.fromJsonString(jsonDescriptor);
-            logger.info("Test classes in bundle: " + testDescriptor);
-            return Optional.of(testDescriptor);
-        } catch (IOException e) {
-            return Optional.empty();
-        }
-    }
-
-    private List<Class<?>> loadClasses(Bundle bundle, TestDescriptor testDescriptor, TestDescriptor.TestCategory testCategory) {
-        List<Class<?>> testClasses = testDescriptor.getConfiguredTests(testCategory).stream()
-                .map(className -> loadClass(bundle, className))
-                .collect(toList());
-
-        StringBuffer buffer = new StringBuffer();
-        testClasses.forEach(cl -> buffer.append("\t").append(cl.toString()).append(" / ").append(cl.getClassLoader().toString()).append("\n"));
-        logger.info("Loaded testClasses: \n" + buffer);
-        return testClasses;
-    }
 
-    private Class<?> loadClass(Bundle bundle, String className) {
-        try {
-            return bundle.loadClass(className);
-        } catch (ClassNotFoundException e) {
-            throw new RuntimeException("Could not find class: " + className + " in bundle " + bundle.getSymbolicName(), e);
-        }
-    }
+    private TestReport launchJunit(Suite suite) {
+        List<Class<?>> testClasses = classLoader.apply(suite);
+        if (testClasses == null)
+            return  null;
 
-    private TestReport launchJunit(List<Class<?>> testClasses, boolean isProductionTest) {
-        var logListener = new VespaJunitLogListener(record -> logRecords.put(record.getSequenceNumber(), record));
-        var summaryListener = new SummaryGeneratingListener();
+        VespaJunitLogListener logListener = new VespaJunitLogListener(record -> logRecords.put(record.getSequenceNumber(), record));
+        SummaryGeneratingListener summaryListener = new SummaryGeneratingListener();
+        LauncherDiscoveryRequest discoveryRequest = LauncherDiscoveryRequestBuilder.request()
+                                                                                   .selectors(testClasses.stream()
+                                                                                                         .map(DiscoverySelectors::selectClass)
+                                                                                                         .collect(toList()))
+                                                                                   .build();
 
-        Launcher launcher = LauncherFactory.create(LauncherConfig.builder().addTestEngines(new JupiterTestEngine()).build());
-        launcher.registerTestExecutionListeners(logListener, summaryListener);
-
-        launcher.execute(LauncherDiscoveryRequestBuilder.request()
-                                                        .selectors(testClasses.stream()
-                                                                              .map(DiscoverySelectors::selectClass)
-                                                                              .collect(toList()))
-                                                        .build());
+        testExecutor.accept(discoveryRequest, new TestExecutionListener[] { logListener, summaryListener });
 
         var report = summaryListener.getSummary();
         var failures = report.getFailures().stream()
-                .map(failure -> {
-                    TestReport.trimStackTraces(failure.getException(), JunitRunner.class.getName());
-                    return new TestReport.Failure(VespaJunitLogListener.toString(failure.getTestIdentifier().getUniqueIdObject()),
-                                                  failure.getException());
-                })
-                .collect(toList());
-        long inconclusive = isProductionTest ? failures.stream()
-                                                       .filter(failure -> failure.exception() instanceof InconclusiveTestException)
-                                                       .count()
-                                             : 0;
-
+                             .map(failure -> {
+                                 TestReport.trimStackTraces(failure.getException(), JunitRunner.class.getName());
+                                 return new TestReport.Failure(VespaJunitLogListener.toString(failure.getTestIdentifier().getUniqueIdObject()),
+                                                               failure.getException());
+                             })
+                             .collect(toList());
+
+        // TODO: move to aggregator.
+        long inconclusive = suite == Suite.PRODUCTION_TEST ? failures.stream()
+                                                                     .filter(failure -> failure.exception() instanceof InconclusiveTestException)
+                                                                     .count()
+                                                           : 0;
         return TestReport.builder()
-                .withSuccessCount(report.getTestsSucceededCount())
-                .withAbortedCount(report.getTestsAbortedCount())
-                .withIgnoredCount(report.getTestsSkippedCount())
-                .withFailedCount(report.getTestsFailedCount() - inconclusive)
-                .withInconclusiveCount(inconclusive)
-                .withFailures(failures)
-                .withLogs(logRecords.values())
-                .build();
+                         .withSuccessCount(report.getTestsSucceededCount())
+                         .withAbortedCount(report.getTestsAbortedCount())
+                         .withIgnoredCount(report.getTestsSkippedCount())
+                         .withFailedCount(report.getTestsFailedCount() - inconclusive)
+                         .withInconclusiveCount(inconclusive)
+                         .withFailures(failures)
+                         .withLogs(logRecords.values())
+                         .build();
     }
 
     @Override
@@ -248,4 +170,18 @@ public class JunitRunner extends AbstractComponent implements TestRunner {
         }
     }
 
+    // TODO(bjorncs|tokle) Propagate credentials root without system property. Ideally move knowledge about path to test-runtime implementations
+    private static void uglyHackSetCredentialsRootSystemProperty(JunitTestRunnerConfig config, Zone zone) {
+        Optional<String> credentialsRoot;
+        if (config.useAthenzCredentials()) {
+            credentialsRoot = Optional.of(Defaults.getDefaults().underVespaHome("var/vespa/sia"));
+        } else if (zone.environment() != Environment.prod){
+            // Only set credentials in non-prod zones where not available
+            credentialsRoot = Optional.of(config.artifactsPath().toString());
+        } else {
+            credentialsRoot = Optional.empty();
+        }
+        credentialsRoot.ifPresent(root -> System.setProperty("vespa.test.credentials.root", root));
+    }
+
 }
diff --git a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestBundleLoader.java b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestBundleLoader.java
new file mode 100644
index 00000000000..3c7c83e3eda
--- /dev/null
+++ b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestBundleLoader.java
@@ -0,0 +1,111 @@
+package com.yahoo.vespa.testrunner;
+
+import ai.vespa.hosted.api.TestDescriptor;
+import com.yahoo.io.IOUtils;
+import com.yahoo.jdisc.application.OsgiFramework;
+import com.yahoo.vespa.testrunner.TestRunner.Suite;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.BundleContext;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.List;
+import java.util.Optional;
+import java.util.logging.Logger;
+import java.util.stream.Stream;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * @author mortent
+ */
+class TestBundleLoader {
+
+    private static final Logger logger = Logger.getLogger(TestBundleLoader.class.getName());
+
+    private final BundleContext bundleContext;
+
+    public TestBundleLoader(OsgiFramework osgi) {
+        this.bundleContext = getUnrestrictedBundleContext(osgi);
+    }
+
+    List<Class<?>> loadTestClasses(Suite suite) {
+        Optional<Bundle> testBundle = findTestBundle();
+        if (testBundle.isEmpty())
+            return null;
+
+        Optional<TestDescriptor> testDescriptor = loadTestDescriptor(testBundle.get());
+        if (testDescriptor.isEmpty())
+            throw new RuntimeException("Could not find test descriptor");
+
+        return loadClasses(testBundle.get(), testDescriptor.get(), toCategory(suite));
+    }
+
+    private Optional<Bundle> findTestBundle() {
+        return Stream.of(bundleContext.getBundles()).filter(this::isTestBundle).findAny();
+    }
+
+    private boolean isTestBundle(Bundle bundle) {
+        String testBundleHeader = bundle.getHeaders().get("X-JDisc-Test-Bundle-Version");
+        return testBundleHeader != null && ! testBundleHeader.isBlank();
+    }
+
+    private static Optional<TestDescriptor> loadTestDescriptor(Bundle bundle) {
+        URL resource = bundle.getEntry(TestDescriptor.DEFAULT_FILENAME);
+        TestDescriptor testDescriptor;
+        try {
+            var jsonDescriptor = IOUtils.readAll(resource.openStream(), UTF_8).trim();
+            testDescriptor = TestDescriptor.fromJsonString(jsonDescriptor);
+            logger.info("Test classes in bundle: " + testDescriptor);
+            return Optional.of(testDescriptor);
+        }
+        catch (IOException e) {
+            return Optional.empty();
+        }
+    }
+
+    private List<Class<?>> loadClasses(Bundle bundle, TestDescriptor testDescriptor, TestDescriptor.TestCategory testCategory) {
+        List<Class<?>> testClasses = testDescriptor.getConfiguredTests(testCategory).stream()
+                                                   .map(className -> loadClass(bundle, className))
+                                                   .collect(toList());
+
+        StringBuffer buffer = new StringBuffer();
+        testClasses.forEach(cl -> buffer.append("\t").append(cl.toString()).append(" / ").append(cl.getClassLoader().toString()).append("\n"));
+        logger.info("Loaded testClasses: \n" + buffer);
+        return testClasses;
+    }
+
+    private Class<?> loadClass(Bundle bundle, String className) {
+        try {
+            return bundle.loadClass(className);
+        }
+        catch (ClassNotFoundException e) {
+            throw new RuntimeException("Could not find class: " + className + " in bundle " + bundle.getSymbolicName(), e);
+        }
+    }
+
+    private static TestDescriptor.TestCategory toCategory(TestRunner.Suite testProfile) {
+        switch(testProfile) {
+            case SYSTEM_TEST: return TestDescriptor.TestCategory.systemtest;
+            case STAGING_SETUP_TEST: return TestDescriptor.TestCategory.stagingsetuptest;
+            case STAGING_TEST: return TestDescriptor.TestCategory.stagingtest;
+            case PRODUCTION_TEST: return TestDescriptor.TestCategory.productiontest;
+            default: throw new RuntimeException("Unknown test profile: " + testProfile.name());
+        }
+    }
+
+    // Hack to retrieve bundle context that allows access to other bundles
+    private static BundleContext getUnrestrictedBundleContext(OsgiFramework framework) {
+        try {
+            BundleContext restrictedBundleContext = framework.bundleContext();
+            var field = restrictedBundleContext.getClass().getDeclaredField("wrapped");
+            field.setAccessible(true);
+            return (BundleContext) field.get(restrictedBundleContext);
+        }
+        catch (ReflectiveOperationException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+}
diff --git a/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/JunitRunnerTest.java b/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/JunitRunnerTest.java
new file mode 100644
index 00000000000..42be19b0f76
--- /dev/null
+++ b/vespa-osgi-testrunner/src/test/java/com/yahoo/vespa/testrunner/JunitRunnerTest.java
@@ -0,0 +1,100 @@
+package com.yahoo.vespa.testrunner;
+
+import com.yahoo.vespa.testrunner.TestRunner.Suite;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.engine.JupiterTestEngine;
+import org.junit.platform.engine.EngineExecutionListener;
+import org.junit.platform.engine.TestDescriptor;
+import org.junit.platform.engine.TestEngine;
+import org.junit.platform.engine.TestExecutionResult;
+import org.junit.platform.engine.reporting.ReportEntry;
+import org.junit.platform.launcher.LauncherDiscoveryRequest;
+import org.junit.platform.launcher.TestExecutionListener;
+import org.junit.platform.launcher.TestIdentifier;
+import org.junit.platform.launcher.TestPlan;
+import org.junit.platform.launcher.core.EngineDiscoveryOrchestrator;
+import org.junit.platform.launcher.core.EngineExecutionOrchestrator;
+import org.junit.platform.launcher.core.LauncherDiscoveryResult;
+
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicReference;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.Phase.EXECUTION;
+
+/**
+ * @author jonmv
+ */
+class JunitRunnerTest {
+
+    @Test
+    void test() throws ExecutionException, InterruptedException {
+        AtomicReference<byte[]> testRuntime = new AtomicReference<>();
+        JunitRunner runner = new JunitRunner(testRuntime::set,
+                                            __ -> List.of(HtmlLoggerTest.class),
+                                            this::execute);
+
+        runner.test(Suite.SYSTEM_TEST, new byte[0]).get();
+        assertEquals(1, runner.getReport().successCount);
+        assertEquals(0, runner.getReport().failedCount);
+    }
+
+
+    // For some inane reason, the JUnit test framework makes it impossible to simply launch a new instance of itself
+    // from inside a unit test (run by itself) in the standard way, so all this kludge is necessary to work around that.
+    void execute(LauncherDiscoveryRequest discoveryRequest, TestExecutionListener... listeners) {
+        TestEngine testEngine = new JupiterTestEngine();
+        LauncherDiscoveryResult discoveryResult = new EngineDiscoveryOrchestrator(Set.of(testEngine), Set.of()).discover(discoveryRequest, EXECUTION);
+        TestDescriptor engineTestDescriptor = discoveryResult.getEngineTestDescriptor(testEngine);
+        TestPlan plan = TestPlan.from(List.of(engineTestDescriptor), discoveryRequest.getConfigurationParameters());
+        for (TestExecutionListener listener : listeners) listener.testPlanExecutionStarted(plan);
+        new EngineExecutionOrchestrator().execute(discoveryResult, new ExecutionListenerAdapter(plan, listeners));
+        for (TestExecutionListener listener : listeners) listener.testPlanExecutionFinished(plan);
+    }
+
+    static class ExecutionListenerAdapter implements EngineExecutionListener {
+
+        private final TestPlan plan;
+        private final List<TestExecutionListener> listeners;
+
+        public ExecutionListenerAdapter(TestPlan plan, TestExecutionListener... listeners) {
+            this.plan = plan;
+            this.listeners = List.of(listeners);
+        }
+
+        private TestIdentifier getTestIdentifier(TestDescriptor testDescriptor) {
+            return plan.getTestIdentifier(testDescriptor.getUniqueId().toString());
+        }
+
+        @Override public void dynamicTestRegistered(TestDescriptor testDescriptor) {
+            TestIdentifier id = TestIdentifier.from(testDescriptor);
+            plan.addInternal(id);
+            for (TestExecutionListener listener : listeners)
+                listener.dynamicTestRegistered(id);
+        }
+
+        @Override public void executionSkipped(TestDescriptor testDescriptor, String reason) {
+            for (TestExecutionListener listener : listeners)
+                listener.executionSkipped(getTestIdentifier(testDescriptor), reason);
+        }
+
+        @Override public void executionStarted(TestDescriptor testDescriptor) {
+            for (TestExecutionListener listener : listeners)
+                listener.executionStarted(getTestIdentifier(testDescriptor));
+        }
+
+        @Override public void executionFinished(TestDescriptor testDescriptor, TestExecutionResult testExecutionResult) {
+            for (TestExecutionListener listener : listeners)
+                listener.executionFinished(getTestIdentifier(testDescriptor), testExecutionResult);
+        }
+
+        @Override public void reportingEntryPublished(TestDescriptor testDescriptor, ReportEntry entry) {
+            for (TestExecutionListener listener : listeners)
+                listener.reportingEntryPublished(getTestIdentifier(testDescriptor), entry);
+        }
+
+    }
+
+}