Fix MultiEnvTestFilter to pass inner test classes (#5775)

Previously the filter required multi-version test extensions to
recognise all test classes in their `accepts` method. This proved
difficult for inner test classes that generally do not carry the
`@ExtendWith` annotation.

Moreover, the old filtering logic was overzealous in requiring
extensions to explicitly "accept" test classes, which would generally
be annotated with the extension anyway.

This change removes the `accepts` method from `MultiEnvTestExtension`
and simplifies the filtering logic to be based only on the presence
of appropriate extension annotations.

MultiEnvExtensionRegistry is enhanced to follow class nesting chains
to find extension annotations.

clients/client-testextension/src/main/java/org/projectnessie/client/ext/MultiVersionApiTest.java

@@ -46,11 +46,6 @@ public List<String> allEnvironmentIds(ConfigurationParameters configuration) {
     return Arrays.stream(NessieApiVersion.values()).map(Enum::name).collect(Collectors.toList());
   }
 
-  @Override
-  public boolean accepts(Class<?> testClass) {
-    return AnnotationUtils.findAnnotation(testClass, NessieApiVersions.class).isPresent();
-  }
-
   static NessieApiVersion apiVersion(ExtensionContext context) {
     return UniqueId.parse(context.getUniqueId()).getSegments().stream()
         .filter(s -> API_VERSION_SEGMENT_TYPE.equals(s.getType()))

compatibility/common/src/main/java/org/projectnessie/tools/compatibility/internal/AbstractMultiVersionExtension.java

@@ -73,8 +73,7 @@ public List<String> allEnvironmentIds(ConfigurationParameters configuration) {
     }
   }
 
-  @Override
-  public boolean accepts(Class<?> testClass) {
+  private void checkExtensions(Class<?> testClass) {
     long count = multiVersionExtensionsForTestClass(Stream.of(testClass)).count();
     if (count > 1) {
       // Sanity check, it's illegal to have multiple nessie-compatibility extensions on one test
@@ -84,8 +83,6 @@ public boolean accepts(Class<?> testClass) {
               "Test class %s contains more than one Nessie multi-version extension",
               testClass.getName()));
     }
-
-    return count == 1;
   }
 
   @Override
@@ -158,6 +155,8 @@ && parseVersion(versionCondition.maxVersion()).isLessThan(version)) {
   }
 
   Version populateNessieVersionAnnotatedFields(ExtensionContext context, Object instance) {
+    checkExtensions(context.getRequiredTestClass());
+
     Optional<Version> nessieVersion = nessieVersionFromContext(context);
     if (!nessieVersion.isPresent()) {
       return null;

compatibility/common/src/test/java/org/projectnessie/tools/compatibility/internal/TestNessieCompatibilityExtensions.java

@@ -16,15 +16,17 @@
 package org.projectnessie.tools.compatibility.internal;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
 
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Stream;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.platform.testkit.engine.EngineExecutionResults;
 import org.junit.platform.testkit.engine.EngineTestKit;
 import org.projectnessie.client.api.NessieApiV1;
 import org.projectnessie.junit.engine.MultiEnvTestEngine;
@@ -58,17 +60,26 @@ void tooManyExtensions() {
                 TooManyExtensions3.class,
                 TooManyExtensions4.class))
         .allSatisfy(
-            c ->
-                assertThatThrownBy(
-                        () ->
-                            EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
-                                .configurationParameter("nessie.versions", "0.18.0,0.19.0")
-                                .selectors(selectClass(c))
-                                .filters(new MultiEnvTestFilter())
-                                .execute())
-                    .isInstanceOf(IllegalStateException.class)
-                    .hasMessageEndingWith(
-                        " contains more than one Nessie multi-version extension"));
+            c -> {
+              EngineExecutionResults result =
+                  EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
+                      .configurationParameter("nessie.versions", "0.18.0,0.19.0")
+                      .selectors(selectClass(c))
+                      .filters(new MultiEnvTestFilter())
+                      .execute();
+              assertThat(
+                      result.allEvents().executions().failed().stream()
+                          .map(e -> e.getTerminationInfo().getExecutionResult().getThrowable())
+                          .filter(Optional::isPresent)
+                          .map(Optional::get))
+                  .isNotEmpty()
+                  .allSatisfy(
+                      e ->
+                          assertThat(e)
+                              .isInstanceOf(IllegalStateException.class)
+                              .hasMessageEndingWith(
+                                  " contains more than one Nessie multi-version extension"));
+            });
   }
 
   @Test
@@ -100,6 +111,19 @@ void olderServers() {
     assertThat(OldServersSample.uris).allSatisfy(uri -> assertThat(uri.getPath()).isEqualTo("/"));
   }
 
+  @Test
+  void nestedTests() {
+    EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
+        .configurationParameter("nessie.versions", "0.18.0,current")
+        .selectors(selectClass(OuterSample.class))
+        .selectors(selectClass(OuterSample.Inner.class))
+        .filters(new MultiEnvTestFilter())
+        .execute();
+    assertThat(OuterSample.outerVersions)
+        .containsExactly(Version.parseVersion("0.18.0"), Version.CURRENT);
+    assertThat(OuterSample.innerVersions).containsExactlyElementsOf(OuterSample.outerVersions);
+  }
+
   @Test
   void upgrade() {
     EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
@@ -204,6 +228,29 @@ void never() {
     }
   }
 
+  @ExtendWith(OlderNessieServersExtension.class)
+  static class OuterSample {
+    static final List<Version> outerVersions = new ArrayList<>();
+    static final List<Version> innerVersions = new ArrayList<>();
+
+    @NessieVersion Version outerVersion;
+
+    @Test
+    void outer() {
+      outerVersions.add(outerVersion);
+    }
+
+    @Nested
+    class Inner {
+      @NessieVersion Version innerVersion;
+
+      @Test
+      void inner() {
+        innerVersions.add(innerVersion);
+      }
+    }
+  }
+
   @ExtendWith(NessieUpgradesExtension.class)
   static class UpgradeSample {
     @NessieAPI NessieApiV1 api;

testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvExtensionRegistry.java

@@ -16,6 +16,8 @@
 package org.projectnessie.junit.engine;
 
 import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.engine.config.DefaultJupiterConfiguration;
@@ -49,9 +51,15 @@ public Stream<MultiEnvTestExtension> stream() {
   }
 
   public Stream<? extends MultiEnvTestExtension> stream(Class<?> testClass) {
+    Set<ExtendWith> annotations = new HashSet<>();
+    // Find annotations following the class nesting chain
+    for (Class<?> cl = testClass; cl != null; cl = cl.getDeclaringClass()) {
+      annotations.addAll(AnnotationUtils.findRepeatableAnnotations(cl, ExtendWith.class));
+    }
+
     //noinspection unchecked
     return (Stream<? extends MultiEnvTestExtension>)
-        AnnotationUtils.findRepeatableAnnotations(testClass, ExtendWith.class).stream()
+        annotations.stream()
             .flatMap(e -> Arrays.stream(e.value()))
             .filter(MultiEnvTestExtension.class::isAssignableFrom)
             .flatMap(registry::stream);

testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestExtension.java

@@ -37,10 +37,4 @@ public interface MultiEnvTestExtension extends Extension {
    * invocations of this method to ensure a stable test case creation order.
    */
   List<String> allEnvironmentIds(ConfigurationParameters configuration);
-
-  /**
-   * Checks whether this extension recognizes the given test class as a multi-environment test
-   * class.
-   */
-  boolean accepts(Class<?> testClass);
 }

testing/multi-env-test-engine/src/main/java/org/projectnessie/junit/engine/MultiEnvTestFilter.java

@@ -53,20 +53,19 @@ private FilterResult filter(Class<?> testClass, UniqueId id) {
     MultiEnvExtensionRegistry registry = registry();
 
     if (id.getEngineId().map("junit-jupiter"::equals).orElse(false)) {
-      if (registry.stream(testClass).anyMatch(ext -> ext.accepts(testClass))) {
+      if (registry.stream(testClass).findAny().isPresent()) {
         return FilterResult.excluded("Excluding multi-env test from Jupiter Engine: " + id);
       } else {
         return FilterResult.included(null);
       }
     } else {
-      // check acceptance only for extensions that own the test
+      // check whether any of the extensions declared by the test recognize the version segment
       boolean matched =
           registry.stream(testClass)
-              .filter(
+              .anyMatch(
                   ext ->
                       id.getSegments().stream()
-                          .anyMatch(s -> ext.segmentType().equals(s.getType())))
-              .anyMatch(ext -> ext.accepts(testClass));
+                          .anyMatch(s -> ext.segmentType().equals(s.getType())));
 
       if (matched) {
         return FilterResult.included(null);

testing/multi-env-test-engine/src/test/java/org/projectnessie/tools/compatibility/engine/TestMultiEnvTestEngine.java

@@ -22,6 +22,7 @@
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor;
@@ -66,14 +67,15 @@ void multiEnv() {
     Set<UniqueId> ids =
         EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
             .selectors(selectClass(MultiEnvAcceptedTest.class))
+            .selectors(selectClass(MultiEnvAcceptedTest.Inner.class))
             .filters(new MultiEnvTestFilter())
             .execute()
             .testEvents()
             .list()
             .stream()
             .map(e -> e.getTestDescriptor().getUniqueId())
             .collect(Collectors.toSet());
-    assertThat(ids).hasSize(2);
+    assertThat(ids).hasSize(4); // 2 from the outer class, 2 from the inner
     assertThat(ids)
         .allSatisfy(id -> assertThat(id.getEngineId()).hasValue(MultiEnvTestEngine.ENGINE_ID));
     assertThat(
@@ -83,26 +85,22 @@ void multiEnv() {
                         id.getSegments().stream()
                             .filter(s -> "test-segment".equals(s.getType()))
                             .map(UniqueId.Segment::getValue)))
-        .containsExactlyInAnyOrder("TE1", "TE2");
-  }
-
-  @Test
-  void multiEnvUnmatched() {
-    assertThat(
-            EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID)
-                .selectors(selectClass(MultiEnvFakeTest.class))
-                .filters(new MultiEnvTestFilter())
-                .execute()
-                .testEvents()
-                .list())
-        .isEmpty(); // annotated, but unmatched tests are excluded
+        .containsExactlyInAnyOrder("TE1", "TE2", "TE1", "TE2");
   }
 
   public static class PlainTest {
     @Test
     void test() {
       // nop
     }
+
+    @Nested
+    class Inner {
+      @Test
+      void test() {
+        // nop
+      }
+    }
   }
 
   @ExtendWith(TestExtension.class)
@@ -111,13 +109,13 @@ public static class MultiEnvAcceptedTest {
     void test() {
       // nop
     }
-  }
 
-  @ExtendWith(TestExtension.class)
-  public static class MultiEnvFakeTest {
-    @Test
-    void test() {
-      // nop
+    @Nested
+    class Inner {
+      @Test
+      void test() {
+        // nop
+      }
     }
   }
 
@@ -131,10 +129,5 @@ public String segmentType() {
     public List<String> allEnvironmentIds(ConfigurationParameters configuration) {
       return Arrays.asList("TE1", "TE2");
     }
-
-    @Override
-    public boolean accepts(Class<?> testClass) {
-      return MultiEnvAcceptedTest.class.equals(testClass);
-    }
   }
 }