Accessing test options no longer warns, but locks in test framework selection

subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java

@@ -26,7 +26,6 @@
 import org.gradle.api.internal.artifacts.dsl.dependencies.DefaultDependencyAdder;
 import org.gradle.api.internal.tasks.AbstractTaskDependency;
 import org.gradle.api.internal.tasks.TaskDependencyResolveContext;
-import org.gradle.api.internal.tasks.testing.TestFramework;
 import org.gradle.api.internal.tasks.testing.filter.DefaultTestFilter;
 import org.gradle.api.internal.tasks.testing.junit.JUnitTestFramework;
 import org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestFramework;
@@ -46,8 +45,6 @@
 
 import javax.annotation.Nullable;
 import javax.inject.Inject;
-import java.util.HashMap;
-import java.util.Map;
 
 public abstract class DefaultJvmTestSuite implements JvmTestSuite {
     public enum Frameworks {
@@ -149,24 +146,21 @@ public DefaultJvmTestSuite(String name, DependencyFactory dependencyFactory, Con
 
         addDefaultTestTarget();
 
-        // Until the values here can be finalized upon the user setting them (see the org.gradle.api.tasks.testing.Test#testFramework(Closure) method),
-        // in Gradle 8, we will be executing the provider lambda used as the convention multiple times.  So make sure, within a Test Suite, that we
-        // always return the same one via computeIfAbsent() against this map.
-        final Map<Frameworks, TestFramework> frameworkLookup = new HashMap<>(3);
-
+        // The values here can now be considered finalized upon the user setting them (see the org.gradle.api.tasks.testing.Test#testFramework(Closure) method).  So
+        // there is now no need to use a map to ensure a given test framework is only created once.
         this.targets.withType(JvmTestSuiteTarget.class).configureEach(target -> {
             target.getTestTask().configure(task -> {
                 task.getTestFrameworkProperty().convention(getVersionedTestingFramework().map(vtf -> {
                     switch(vtf.type) {
                         case NONE: // fall-through
                         case JUNIT4:
-                            return frameworkLookup.computeIfAbsent(vtf.type, f -> new JUnitTestFramework(task, (DefaultTestFilter) task.getFilter()));
+                            return new JUnitTestFramework(task, (DefaultTestFilter) task.getFilter());
                         case KOTLIN_TEST: // fall-through
                         case JUNIT_JUPITER: // fall-through
                         case SPOCK:
-                            return frameworkLookup.computeIfAbsent(vtf.type, f -> new JUnitPlatformTestFramework((DefaultTestFilter) task.getFilter()));
+                            return new JUnitPlatformTestFramework((DefaultTestFilter) task.getFilter());
                         case TESTNG:
-                            return frameworkLookup.computeIfAbsent(vtf.type, f -> new TestNGTestFramework(task, task.getClasspath(), (DefaultTestFilter) task.getFilter(), getObjectFactory()));
+                            return new TestNGTestFramework(task, task.getClasspath(), (DefaultTestFilter) task.getFilter(), getObjectFactory());
                         default:
                             throw new IllegalStateException("do not know how to handle " + vtf);
                     }

subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/TestTaskIntegrationTest.groovy

@@ -282,7 +282,7 @@ class TestTaskIntegrationTest extends JUnitMultiVersionIntegrationSpec {
         extraArgs << [[], ["--tests", "MyTest"]]
     }
 
-    def "options set prior to setting same test framework will warn and have no effect"() {
+    def "options set prior to setting same test framework will cause error"() {
         ignoreWhenJUnitPlatform()
 
         given:
@@ -302,22 +302,16 @@ class TestTaskIntegrationTest extends JUnitMultiVersionIntegrationSpec {
                 }
                 useJUnit()
             }
-
-            tasks.register('verifyTestOptions') {
-                doLast {
-                    assert tasks.getByName("test").getOptions().getClass() == JUnitOptions
-                    assert !tasks.getByName("test").getOptions().getExcludeCategories().contains("Slow")
-                }
-            }
         """.stripIndent()
 
-        executer.expectDeprecationWarning("Accessing test options prior to setting test framework has been deprecated.")
+        when:
+        fails("test")
 
-        expect:
-        succeeds("test", "verifyTestOptions", "--warn")
+        then:
+        failure.assertHasErrorOutput("You cannot set the test framework to: JUnit 4 after accessing test options. The current framework is: JUnit 4.")
     }
 
-    def "options set prior to changing test framework will produce additional warning and have no effect"() {
+    def "options set prior to changing test framework will cause error"() {
         ignoreWhenJUnitPlatform()
 
         given:
@@ -340,24 +334,16 @@ class TestTaskIntegrationTest extends JUnitMultiVersionIntegrationSpec {
             test {
                 useJUnitPlatform()
             }
-
-            tasks.register('verifyTestOptions') {
-                doLast {
-                    assert tasks.getByName("test").getOptions().getClass() == JUnitPlatformOptions
-                }
-            }
         """.stripIndent()
 
-        executer.expectDeprecationWarning("Accessing test options prior to setting test framework has been deprecated.")
-
         when:
-        succeeds("test", "verifyTestOptions", "--warn")
+        fails("test")
 
         then:
-        outputContains("Test framework is changing from 'JUnitTestFramework', previous option configuration would not be applicable.")
+        failure.assertHasErrorOutput("You cannot change the test framework to: JUnit Platform after accessing test options. The current framework is: JUnit 4.")
     }
 
-    def "options accessed and not explicitly configured prior to setting test framework will also warn"() {
+    def "options accessed and not explicitly configured prior to setting test framework also fails"() {
         given:
         file('src/test/java/MyTest.java') << junitJupiterStandaloneTestClass()
 
@@ -377,19 +363,13 @@ class TestTaskIntegrationTest extends JUnitMultiVersionIntegrationSpec {
             test {
                 useJUnitPlatform()
             }
-
-            tasks.register('verifyTestOptions') {
-                doLast {
-                    assert options.getClass() == JUnitOptions
-                    assert tasks.getByName("test").getOptions().getClass() == JUnitPlatformOptions
-                }
-            }
         """.stripIndent()
 
-        executer.expectDeprecationWarning("Accessing test options prior to setting test framework has been deprecated.")
+        when:
+        fails("test")
 
-        expect:
-        succeeds("test", "verifyTestOptions", "--warn")
+        then:
+        failure.assertHasErrorOutput("You cannot change the test framework to: JUnit Platform after accessing test options. The current framework is: JUnit 4.")
     }
 
     def "options configured after setting test framework works"() {

subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/testsuites/TestSuitesIntegrationTest.groovy

@@ -23,7 +23,6 @@ import org.gradle.api.plugins.jvm.internal.DefaultJvmTestSuite
 import org.gradle.api.tasks.testing.junit.JUnitOptions
 import org.gradle.api.tasks.testing.junitplatform.JUnitPlatformOptions
 import org.gradle.integtests.fixtures.AbstractIntegrationSpec
-import org.gradle.integtests.fixtures.DefaultTestExecutionResult
 import org.gradle.integtests.fixtures.JUnitXmlTestExecutionResult
 import spock.lang.Issue
 
@@ -480,8 +479,7 @@ class TestSuitesIntegrationTest extends AbstractIntegrationSpec {
         succeeds("checkConfiguration")
     }
 
-    // TODO: Test Framework Selection - Revert this to may NOT in Gradle 8
-    def "test framework MAY be changed once options have been used with test suites"() {
+    def "test framework can not be changed once options have been used with test suites"() {
         buildFile << """
             plugins {
                 id 'java'
@@ -511,14 +509,14 @@ class TestSuitesIntegrationTest extends AbstractIntegrationSpec {
             check.dependsOn testing.suites
         """
 
-        executer.expectDeprecationWarning("Accessing test options prior to setting test framework has been deprecated. This is scheduled to be removed in Gradle 8.0.")
+        when:
+        fails("check")
 
-        expect:
-        succeeds("check")
+        then:
+        failure.assertHasErrorOutput("You cannot change the test framework to: Test NG after accessing test options. The current framework is: JUnit 4.")
     }
 
-    // This checks for backwards compatibility with builds that may rely on this
-    def "can change the test framework multiple times before execution when not using test suites"() {
+    def "can change not the test framework multiple times before execution when not using test suites"() {
         given:
         buildFile << """
             plugins {
@@ -550,16 +548,11 @@ class TestSuitesIntegrationTest extends AbstractIntegrationSpec {
             }
         """
 
-        executer.expectDeprecationWarning("Accessing test options prior to setting test framework has been deprecated. This is scheduled to be removed in Gradle 8.0.")
-        executer.expectDeprecationWarning("Accessing test options prior to setting test framework has been deprecated. This is scheduled to be removed in Gradle 8.0.")
-
         when:
-        succeeds("test")
+        fails("test")
 
         then:
-        executedAndNotSkipped(":test")
-        DefaultTestExecutionResult result = new DefaultTestExecutionResult(testDirectory)
-        result.assertTestClassesExecuted("SomeTest")
+        failure.assertHasErrorOutput("You cannot change the test framework to: JUnit Platform after accessing test options. The current framework is: JUnit 4.")
     }
 
     // This is not the behavior we want in the long term because this makes build configuration sensitive to the order

subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/TestFramework.java

@@ -61,4 +61,10 @@ public interface TestFramework extends Closeable {
      */
     @Internal
     List<String> getTestWorkerImplementationModules();
+
+    /**
+     * This property controls how we describe this test framework to the user (i.e., in error messages).
+     */
+    @Internal
+    String getDisplayName();
 }

subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/junit/JUnitTestFramework.java

@@ -73,6 +73,11 @@ public List<String> getTestWorkerImplementationModules() {
         return Collections.emptyList();
     }
 
+    @Override
+    public String getDisplayName() {
+        return "JUnit 4";
+    }
+
     @Override
     public JUnitOptions getOptions() {
         return options;

subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/junitplatform/JUnitPlatformTestFramework.java

@@ -75,6 +75,11 @@ public List<String> getTestWorkerImplementationModules() {
         return ImmutableList.of("junit-platform-engine", "junit-platform-launcher", "junit-platform-commons");
     }
 
+    @Override
+    public String getDisplayName() {
+        return "JUnit Platform";
+    }
+
     @Override
     public JUnitPlatformOptions getOptions() {
         return options;

subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestFramework.java

@@ -171,6 +171,10 @@ public List<String> getTestWorkerImplementationModules() {
         return Collections.emptyList();
     }
 
+    @Override
+    public String getDisplayName() {
+        return "Test NG";
+    }
     @Override
     public TestNGOptions getOptions() {
         return options;

subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java

@@ -21,6 +21,7 @@
 import groovy.lang.DelegatesTo;
 import org.gradle.StartParameter;
 import org.gradle.api.Action;
+import org.gradle.api.GradleException;
 import org.gradle.api.Incubating;
 import org.gradle.api.JavaVersion;
 import org.gradle.api.NonNullApi;
@@ -69,7 +70,6 @@
 import org.gradle.internal.Factory;
 import org.gradle.internal.actor.ActorFactory;
 import org.gradle.internal.concurrent.CompositeStoppable;
-import org.gradle.internal.deprecation.DeprecationLogger;
 import org.gradle.internal.jvm.DefaultModularitySpec;
 import org.gradle.internal.jvm.JavaModuleDetector;
 import org.gradle.internal.jvm.UnsupportedJavaRuntimeException;
@@ -172,6 +172,12 @@ public class Test extends AbstractTestTask implements JavaForkOptions, PatternFi
     private FileCollection classpath;
     private final ConfigurableFileCollection stableClasspath;
     private final Property<TestFramework> testFramework;
+
+    /*
+     * These fields are status variables that are used to keep track of the state of the test task
+     * to prevent potential errors or unexpected behavior caused by setting the test framework
+     * after setting options.
+     */
     private boolean userHasConfiguredTestFramework;
     private boolean optionsAccessed;
 
@@ -920,31 +926,6 @@ public TestFramework testFramework(@Nullable Closure testFrameworkConfigure) {
             useJUnit(testFrameworkConfigure);
         }
 
-        // To maintain backwards compatibility with builds that may configure the test framework
-        // multiple times for a single task--either switching between frameworks or overwriting
-        // the existing configuration for a test framework--we need to keep track if the user has
-        // explicitly set a test framework
-        // With test suites, users should never need to call the useXXX methods, so we can warn if
-        // them from doing something like this (for now, in order to preserve existing builds).
-        // This behavior should be restored to fail fast once again with the next major version.
-        //
-        // testTask.configure {
-        //      options {
-        //          /* configure JUnit Platform */
-        //      }
-        // }
-        // testTask.configure {
-        //      useJUnit()
-        //      options {
-        //          /* configure JUnit */
-        //      }
-        // }
-
-        // TODO: Test Framework Selection - Restore this to re-enable fail-fast behavior for Gradle 8
-//        if (!userHasConfiguredTestFramework) {
-//            testFramework.finalizeValue();
-//        }
-
         return testFramework.get();
     }
 
@@ -988,16 +969,13 @@ TestFramework useTestFramework(TestFramework testFramework) {
     }
 
     private <T extends TestFrameworkOptions> TestFramework useTestFramework(TestFramework testFramework, @Nullable Action<? super T> testFrameworkConfigure) {
+        final TestFramework oldFramework = Test.this.testFramework.get();
+
         if (optionsAccessed) {
-            DeprecationLogger.deprecateAction("Accessing test options prior to setting test framework")
-                .withContext("\nTest options have already been accessed for task: '" + getProject().getName() + ":" + getName() + "' prior to setting the test framework to: '" + testFramework.getClass().getSimpleName() + "'. Any previous configuration will be discarded.\n")
-                .willBeRemovedInGradle8()
-                .withDslReference(Test.class, "options")
-                .nagUser();
-
-            if (!this.testFramework.get().getClass().equals(testFramework.getClass())) {
-                getLogger().warn("Test framework is changing from '{}', previous option configuration would not be applicable.", this.testFramework.get().getClass().getSimpleName());
-            }
+            throw new GradleException(String.format("You cannot %s the test framework to: %s after accessing test options. The current framework is: %s.",
+                    testFramework.getClass() == oldFramework.getClass() ? "set" : "change",
+                    testFramework.getDisplayName(),
+                    oldFramework.getDisplayName()));
         }
 
         userHasConfiguredTestFramework = true;