Rework stack trace trimming, using declared test class names

6 files changed, 67 insertions, 47 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 21890bab569..6fd3400d41d 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
@@ -22,6 +22,7 @@ import java.time.Clock;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.SortedMap;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentSkipListMap;
@@ -32,6 +33,8 @@ import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
 
+import static java.util.stream.Collectors.toSet;
+
 
 /**
  * @author mortent
@@ -115,9 +118,11 @@ public class JunitRunner extends AbstractComponent implements TestRunner {
         List<Class<?>> testClasses = classLoader.apply(suite);
         if (testClasses == null)
             return  null;
+        Set<String> testClassNames = testClasses.stream().map(Class::getName).collect(toSet());
 
         testRuntimeProvider.initialize(testConfig);
         TestReportGeneratingListener testReportListener = new TestReportGeneratingListener(suite,
+                                                                                           testClassNames,
                                                                                            record -> logRecords.put(record.getSequenceNumber(), record),
                                                                                            stdoutTee,
                                                                                            stderrTee,
diff --git a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReport.java b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReport.java
index a2ac86309d9..e967ddd8abe 100644
--- a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReport.java
+++ b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReport.java
@@ -32,28 +32,30 @@ public class TestReport {
 
     private final Object monitor = new Object();
     private final Set<TestIdentifier> complete = new HashSet<>();
+    private final Set<String> testClassNames;
     private final Clock clock;
     private final ContainerNode root;
     private final Suite suite;
     private NamedNode current;
     private TestPlan plan;
 
-    private TestReport(Clock clock, Suite suite, ContainerNode root) {
+    private TestReport(Clock clock, Suite suite, Set<String> testClassNames, ContainerNode root) {
         this.clock = clock;
         this.root = root;
+        this.testClassNames = Set.copyOf(testClassNames);
         this.current = root;
         this.suite = suite;
     }
 
-    TestReport(Clock clock, Suite suite) {
-        this(clock, suite, new ContainerNode(null, null, toString(suite), clock.instant()));
+    TestReport(Clock clock, Suite suite, Set<String> testClassNames) {
+        this(clock, suite, testClassNames, new ContainerNode(null, null, toString(suite), clock.instant()));
     }
 
     static TestReport createFailed(Clock clock, Suite suite, Throwable thrown) {
         if (thrown instanceof OutOfMemoryError) throw (Error) thrown;
-        TestReport failed = new TestReport(clock, suite);
+        TestReport failed = new TestReport(clock, suite, Set.of());
         failed.complete();
-        failed.root().children.add(new FailureNode(failed.root(), clock.instant(), thrown, suite));
+        failed.root().children.add(new FailureNode(failed.root(), clock.instant(), thrown, suite, Set.of()));
         return failed;
     }
 
@@ -127,7 +129,7 @@ public class TestReport {
         synchronized (monitor) {
             Status status = Status.successful;
             if (thrown != null) {
-                FailureNode failure = new FailureNode(current, clock.instant(), thrown, suite);
+                FailureNode failure = new FailureNode(current, clock.instant(), thrown, suite, testClassNames);
                 current.children.add(failure);
                 status = failure.status();
             }
@@ -138,7 +140,7 @@ public class TestReport {
 
     void log(LogRecord record) {
         synchronized (monitor) {
-            if (record.getThrown() != null) trimStackTraces(record.getThrown(), JunitRunner.class.getName());
+            if (record.getThrown() != null) trimStackTraces(record.getThrown(), testClassNames);
             if ( ! (current.children.peekLast() instanceof OutputNode))
                 current.children.add(new OutputNode(current));
 
@@ -158,7 +160,9 @@ public class TestReport {
                 ContainerNode newRoot = new ContainerNode(null, null, root.name(), root.start());
                 newRoot.children.addAll(root.children);
                 newRoot.children.addAll(other.root.children);
-                TestReport merged = new TestReport(clock, suite, newRoot);
+                Set<String> testClassNames = new HashSet<>(this.testClassNames);
+                testClassNames.addAll(other.testClassNames);
+                TestReport merged = new TestReport(clock, suite, testClassNames, newRoot);
                 merged.complete();
                 return merged;
             }
@@ -277,9 +281,9 @@ public class TestReport {
         private final Throwable thrown;
         private final Suite suite;
 
-        public FailureNode(NamedNode parent, Instant now, Throwable thrown, Suite suite) {
+        public FailureNode(NamedNode parent, Instant now, Throwable thrown, Suite suite, Set<String> testClassNames) {
             super(parent, null, thrown.toString(), now);
-            trimStackTraces(thrown, JunitRunner.class.getName());
+            trimStackTraces(thrown, testClassNames);
             this.thrown = thrown;
             this.suite = suite;
 
@@ -327,26 +331,20 @@ public class TestReport {
 
     /**
      * Recursively trims stack traces for the given throwable and its causes/suppressed.
-     * This is based on the assumption that the relevant stack is anything above the first native
-     * reflection invocation, above any frame in the given root class.
+     * This is based on the assumption that the relevant stack is anything from the first
+     * test bundle class frame, and upwards; the exception is for dynamic tests, where a
+     * specific dynamic test factory method is right below the first user frame.
      */
-    static void trimStackTraces(Throwable thrown, String testFrameworkRootClass) {
+    static void trimStackTraces(Throwable thrown, Set<String> testClassNames) {
         if (thrown == null)
             return;
 
         StackTraceElement[] stack = thrown.getStackTrace();
-        int i = 0;
-        int firstReflectFrame = -1;
-        int cutoff = 0;
-        boolean rootedInTestFramework = false;
+        int i = -1;
+        int cutoff = stack.length;
         while (++i < stack.length) {
-            rootedInTestFramework |= testFrameworkRootClass.equals(stack[i].getClassName());
-            if (firstReflectFrame == -1 && stack[i].getClassName().startsWith("jdk.internal.reflect."))
-                firstReflectFrame = i; // jdk.internal.reflect class invokes the first user test frame, on both jdk 17 and 21.
-            if (rootedInTestFramework && firstReflectFrame > 0) {
-                cutoff = firstReflectFrame;
-                break;
-            }
+            for (String name : testClassNames) if (stack[i].getClassName().startsWith(name))
+                cutoff = i + 1;
             boolean isDynamicTestInvocation = "org.junit.jupiter.engine.descriptor.DynamicTestTestDescriptor".equals(stack[i].getClassName());
             if (isDynamicTestInvocation) {
                 cutoff = i;
@@ -356,9 +354,9 @@ public class TestReport {
         thrown.setStackTrace(copyOf(stack, cutoff));
 
         for (Throwable suppressed : thrown.getSuppressed())
-            trimStackTraces(suppressed, testFrameworkRootClass);
+            trimStackTraces(suppressed, testClassNames);
 
-        trimStackTraces(thrown.getCause(), testFrameworkRootClass);
+        trimStackTraces(thrown.getCause(), testClassNames);
     }
 
     private static String toString(Suite suite) {
diff --git a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReportGeneratingListener.java b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReportGeneratingListener.java
index 5bc9fda6835..487cc8c501a 100644
--- a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReportGeneratingListener.java
+++ b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/TestReportGeneratingListener.java
@@ -18,6 +18,7 @@ import java.time.Clock;
 import java.time.ZoneOffset;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Consumer;
 import java.util.logging.Handler;
 import java.util.logging.Level;
@@ -41,8 +42,8 @@ class TestReportGeneratingListener implements TestExecutionListener {
     private final Handler handler;              // Captures logging from test code.
     private final Clock clock;
 
-    TestReportGeneratingListener(Suite suite, Consumer<LogRecord> logger, TeeStream stdoutTee, TeeStream stderrTee, Clock clock) {
-        this.report = new TestReport(clock, suite);
+    TestReportGeneratingListener(Suite suite, Set<String> testClassNames, Consumer<LogRecord> logger, TeeStream stdoutTee, TeeStream stderrTee, Clock clock) {
+        this.report = new TestReport(clock, suite, testClassNames);
         this.logger = logger;
         this.stdoutTee = stdoutTee;
         this.stderrTee = stderrTee;
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 2690c89e1c1..0c55645eb33 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
@@ -12,6 +12,7 @@ import java.time.Clock;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
@@ -148,11 +149,6 @@ class AggregateTestRunnerTest {
     }
 
     @Test
-    void testReportStatus() {
-        assertEquals(FAILURE, JunitRunner.testRunnerStatus(TestReport.createFailed(Clock.systemUTC(), Suite.SYSTEM_TEST, new RuntimeException("hello"))));
-    }
-
-    @Test
     void testStackTrimming() {
         try {
             try {
@@ -163,17 +159,22 @@ class AggregateTestRunnerTest {
             }
         }
         catch (Exception e) {
-            TestReport.trimStackTraces(e, "org.junit.platform.commons.util.ReflectionUtils");
+            TestReport.trimStackTraces(e, Set.of(AggregateTestRunnerTest.class.getName()));
             assertEquals("""
-                            java.lang.RuntimeException: java.lang.RuntimeException: inner
-                            \tat com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:162)
-                            Caused by: java.lang.RuntimeException: inner
-                            \tat com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:159)
-                            """,
+                         java.lang.RuntimeException: java.lang.RuntimeException: inner
+                         	at com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:158)
+                         Caused by: java.lang.RuntimeException: inner
+                         	at com.yahoo.vespa.testrunner.AggregateTestRunnerTest.testStackTrimming(AggregateTestRunnerTest.java:155)
+                         """,
                          ExceptionUtils.getStackTraceAsString(e));
         }
     }
 
+    @Test
+    void testReportStatus() {
+        assertEquals(FAILURE, JunitRunner.testRunnerStatus(TestReport.createFailed(Clock.systemUTC(), Suite.SYSTEM_TEST, new RuntimeException("hello"))));
+    }
+
     static class MockTestRunner implements TestRunner {
 
         final List<LogRecord> log = new ArrayList<>();
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
index 12700040f60..6878579b950 100644
--- 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
@@ -28,13 +28,18 @@ import java.io.BufferedReader;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.lang.annotation.Annotation;
+import java.net.URL;
+import java.net.URLClassLoader;
 import java.time.Clock;
 import java.time.Instant;
 import java.time.ZoneId;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.EnumMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.UnaryOperator;
 import java.util.stream.Stream;
 
 import static ai.vespa.hosted.cd.internal.TestRuntimeProvider.testRuntime;
@@ -97,17 +102,20 @@ class JunitRunnerTest {
     }
 
     static TestRunner test(Suite suite, byte[] testConfig, Class<?>... testClasses) {
-        JunitRunner runner = new JunitRunner(clock,
-                                             config -> { assertSame(testConfig, config); testRuntime.set(new MockTestRuntime()); },
-                                             __ -> List.of(testClasses),
-                                             JunitRunnerTest::execute);
         try {
+            JunitRunner runner = new JunitRunner(clock,
+                                                 config -> {
+                                                     assertSame(testConfig, config);
+                                                     testRuntime.set(new MockTestRuntime());
+                                                 },
+                                                 __ -> List.of(testClasses),
+                                                 JunitRunnerTest::execute);
             runner.test(suite, testConfig).get();
+            return runner;
         }
         catch (Exception e) {
-            fail(e);
+            throw new AssertionError(e);
         }
-        return runner;
     }
 
 
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 6d6fbbf2cf1..9edd9a23c68 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
@@ -19,10 +19,15 @@ import org.junit.jupiter.api.Test;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.net.URLClassLoader;
 import java.time.Clock;
 import java.time.Instant;
 import java.time.ZoneId;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
@@ -47,16 +52,18 @@ class TestRunnerHandlerTest {
     private TestRunner aggregateRunner;
 
     @BeforeEach
-    void setup() {
+    void setup() throws Exception {
         TestReport moreTestsReport = JunitRunnerTest.test(Suite.PRODUCTION_TEST,
                                                           new byte[0],
                                                           FailingTestAndBothAftersTest.class,
                                                           WrongBeforeAllTest.class,
                                                           FailingExtensionTest.class)
                                                     .getReport();
+        Exception cnfe = new ClassNotFoundException("School's out all summer!");
+        cnfe.setStackTrace(Arrays.copyOf(cnfe.getStackTrace(), 1));
         TestReport failedReport = TestReport.createFailed(Clock.fixed(testInstant, ZoneId.of("UTC")),
                                                           Suite.PRODUCTION_TEST,
-                                                          new ClassNotFoundException("School's out all summer!"));
+                                                          cnfe);
         aggregateRunner = AggregateTestRunner.of(List.of(new MockRunner(TestRunner.Status.SUCCESS,
                                                                         AggregateTestRunnerTest.report.mergedWith(moreTestsReport)
                                                                                                       .mergedWith(failedReport))));