[SUREFIRE-2190] Fix module dependencies for compile only dependencies (…

…#668)

* [SUREFIRE-2190] Fix module dependencies for compile only dependencies

Include all modules on the module path to the root modules. This fixes
test code that uses dependencies that are declared as optional
(`require static`) for the main artifact to access these dependencies.

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfiguration.java

@@ -194,18 +194,16 @@ File createArgsFile(
                             .append(NL);
                 }
 
-                args.append("--add-modules").append(NL).append(moduleName).append(NL);
-
                 args.append("--add-reads")
                         .append(NL)
                         .append(moduleName)
                         .append('=')
                         .append("ALL-UNNAMED")
                         .append(NL);
-            } else {
-                args.append("--add-modules").append(NL).append(moduleName).append(NL);
             }
 
+            args.append("--add-modules").append(NL).append("ALL-MODULE-PATH").append(NL);
+
             for (String[] entries : providerJpmsArguments) {
                 for (String entry : entries) {
                     args.append(entry).append(NL);

maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/ForkConfigurationTest.java

@@ -192,7 +192,7 @@ public void testCliArgs() throws Exception {
         assertThat(endOfFileArg).isPositive();
         Path argFile = Paths.get(cliAsString.substring(beginOfFileArg + 1, endOfFileArg));
         String argFileText = new String(readAllBytes(argFile));
-        assertThat(argFileText).contains("arg2").contains("arg3").contains("--add-modules" + NL + "test.module");
+        assertThat(argFileText).contains("arg2").contains("arg3").contains("--add-modules" + NL + "ALL-MODULE-PATH");
     }
 
     @Test

maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfigurationTest.java

@@ -127,13 +127,13 @@ public void shouldCreateModularArgsFile() throws Exception {
 
         assertThat(argsFileLines.get(7)).isEqualTo("abc/org.apache.abc=ALL-UNNAMED");
 
-        assertThat(argsFileLines.get(8)).isEqualTo("--add-modules");
+        assertThat(argsFileLines.get(8)).isEqualTo("--add-reads");
 
-        assertThat(argsFileLines.get(9)).isEqualTo("abc");
+        assertThat(argsFileLines.get(9)).isEqualTo("abc=ALL-UNNAMED");
 
-        assertThat(argsFileLines.get(10)).isEqualTo("--add-reads");
+        assertThat(argsFileLines.get(10)).isEqualTo("--add-modules");
 
-        assertThat(argsFileLines.get(11)).isEqualTo("abc=ALL-UNNAMED");
+        assertThat(argsFileLines.get(11)).isEqualTo("ALL-MODULE-PATH");
 
         assertThat(argsFileLines.get(12)).isEqualTo(ForkedBooter.class.getName());
 

surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire2190JUnitIT.java

@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.surefire.its.jiras;
+
+import org.apache.maven.surefire.its.fixture.AbstractJava9PlusIT;
+import org.junit.Test;
+
+/**
+ * Integration test for <a href="https://issues.apache.org/jira/browse/SUREFIRE-2190">SUREFIRE-2190</a>.
+ */
+public class Surefire2190JUnitIT extends AbstractJava9PlusIT {
+    @Test
+    public void test() throws Exception {
+        assumeJava9().debugLogging().executeVerify().assertTestSuiteResults(4, 0, 0, 0);
+    }
+
+    @Override
+    protected String getProjectDirectoryName() {
+        return "surefire-2190";
+    }
+}

surefire-its/src/test/resources/surefire-2190/pom.xml

@@ -0,0 +1,68 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+~   Licensed under the Apache License, Version 2.0 (the "License");
+~   you may not use this file except in compliance with the License.
+~   You may obtain a copy of the License at
+~
+~   http://www.apache.org/licenses/LICENSE-2.0
+~
+~   Unless required by applicable law or agreed to in writing, software
+~   distributed under the License is distributed on an "AS IS" BASIS,
+~   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+~   See the License for the specific language governing permissions and
+~   limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>org.apache.maven.plugins.surefire</groupId>
+    <artifactId>surefire-2190-it</artifactId>
+    <version>1.0</version>
+
+    <properties>
+        <maven.compiler.source>${java.specification.version}</maven.compiler.source>
+        <maven.compiler.target>${java.specification.version}</maven.compiler.target>
+        <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
+    </properties>
+
+    <dependencies>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <version>5.9.1</version>
+            <scope>test</scope>
+        </dependency>
+
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-params</artifactId>
+            <version>5.9.1</version>
+            <scope>test</scope>
+        </dependency>
+
+        <dependency>
+            <groupId>jakarta.annotation</groupId>
+            <artifactId>jakarta.annotation-api</artifactId>
+            <version>2.1.1</version>
+            <scope>provided</scope>
+            <optional>true</optional>
+        </dependency>
+    </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <version>3.8.1</version>
+                <configuration>
+                    <encoding>UTF-8</encoding>
+                </configuration>
+            </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <version>${surefire.version}</version>
+            </plugin>
+        </plugins>
+    </build>
+</project>

surefire-its/src/test/resources/surefire-2190/src/main/java/module-info.java

@@ -0,0 +1,5 @@
+module surefire.bug.thing3 {
+    requires static jakarta.annotation;
+
+    exports surefirebug.thing;
+}

surefire-its/src/test/resources/surefire-2190/src/main/java/surefirebug/thing/Main.java

@@ -0,0 +1,44 @@
+package surefirebug.thing;
+
+import java.util.concurrent.Callable;
+import java.util.function.Supplier;
+
+public final class Main implements Callable<String> {
+
+  private static final Supplier<String> DEFAULT_PROVIDER = () -> "Hello, World";
+
+
+  public static void main(String... args) throws Exception {
+    Main main = new Main();
+    String text = main.call();
+
+    System.out.println(text);
+  }
+
+  private final Supplier<String> supplier;
+
+  public Main(Supplier<String> supplier) {
+    this.supplier = supplier;
+  }
+
+  public Main() {
+    this(DEFAULT_PROVIDER);
+  }
+
+  @Override
+  public String call() {
+    var value = supplier.get();
+
+    if (value == null) {
+      var annotations = supplier.getClass().getAnnotations();
+      for (var annotation : annotations) {
+        if (annotation.annotationType().getSimpleName().equals("Nullable")) {
+          return "<null value>";
+        }
+      }
+      throw new IllegalStateException("null without @Nullable annotation");
+    }
+
+    return value;
+  }
+}

surefire-its/src/test/resources/surefire-2190/src/test/java/surefirebug/thing/MainTest.java

@@ -0,0 +1,55 @@
+package surefirebug.thing;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import jakarta.annotation.Nullable;
+import java.util.function.Supplier;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class MainTest {
+
+    @Test
+    public void testDefault() {
+        Main main = new Main();
+        assertEquals("Hello, World", main.call());
+    }
+
+    @Test
+    public void testNonStandard() {
+        Main main = new Main(new NonstandardSupplier());
+        assertEquals("Hello, People", main.call());
+    }
+
+    @Test
+    public void testNullSupplierWithNullable() {
+        Main main = new Main(new NullSupplierWithNullable());
+        assertEquals("<null value>", main.call());
+    }
+
+    @Test
+    public void testNullSupplierWithoutNullable() {
+        Main main = new Main(new NullSupplierWithoutNullable());
+        IllegalStateException e = Assertions.assertThrows(IllegalStateException.class, main::call);
+        assertEquals("null without @Nullable annotation", e.getMessage());
+    }
+
+    public static class NonstandardSupplier implements Supplier<String> {
+        public String get() {
+            return "Hello, People";
+        }
+    }
+
+    @Nullable
+    public static class NullSupplierWithNullable implements Supplier<String> {
+        public String get() {
+            return null;
+        }
+    }
+
+    public static class NullSupplierWithoutNullable implements Supplier<String> {
+        public String get() {
+            return null;
+        }
+    }
+}