New checkJUnitDependencies task

README.md

@@ -378,3 +378,5 @@ Configures some sensible defaults:
 2. If Gradle detects you use JUnit 5 (i.e. you have a `testImplementation 'org:junit.jupiter:junit-jupiter'` dependency), it will automatically configure your `Test` tasks to run with `useJUnitPlatform()`, and configure all `@Test` methods to run in parallel by default.  Many other languages take this stance by default - if some tests rely on static state then you can mark them as non-parallel.
 
     See more here: https://junit.org/junit5/docs/current/user-guide/#writing-tests-parallel-execution
+
+The plugin also adds a `checkJUnitDependencies` to make the migration to JUnit5 safer.  Specifically, it should prevent cases where the tests could silently not run due to misconfigured dependencies.

changelog/@unreleased/pr-837.v2.yml

@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: A new `checkJUnitDependencies` task detects misconfigured JUnit dependencies
+    which could result in some tests silently not running.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/837

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java

@@ -16,16 +16,18 @@
 
 package com.palantir.baseline.plugins;
 
+import com.palantir.baseline.tasks.CheckJUnitDependencies;
 import java.util.Objects;
+import java.util.Optional;
 import org.gradle.api.Plugin;
 import org.gradle.api.Project;
 import org.gradle.api.artifacts.Dependency;
 import org.gradle.api.plugins.JavaPlugin;
 import org.gradle.api.plugins.JavaPluginConvention;
 import org.gradle.api.specs.Spec;
 import org.gradle.api.tasks.SourceSet;
+import org.gradle.api.tasks.TaskProvider;
 import org.gradle.api.tasks.testing.Test;
-import org.gradle.api.tasks.testing.TestFrameworkOptions;
 import org.gradle.api.tasks.testing.junitplatform.JUnitPlatformOptions;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -45,34 +47,46 @@ public void apply(Project project) {
             }
         });
 
-        project.getPlugins().withType(JavaPlugin.class, unused -> {
+        project.getPlugins().withType(JavaPlugin.class, unusedPlugin -> {
             // afterEvaluate necessary because the junit-jupiter dep might be added further down the build.gradle
             project.afterEvaluate(proj -> {
                 proj.getConvention()
                         .getPlugin(JavaPluginConvention.class)
                         .getSourceSets()
-                        .matching(ss -> hasCompileDependenciesMatching(proj, ss, this::isJunitJupiter))
+                        .matching(ss -> hasCompileDependenciesMatching(proj, ss, BaselineTesting::isJunitJupiter))
                         .forEach(ss -> {
-                            String testTaskName = ss.getTaskName(null, "test");
-                            Test testTask = (Test) proj.getTasks().findByName(testTaskName);
-                            if (testTask == null) {
-                                // Fall back to the source set name, since that is what gradle-testsets-plugin does
-                                testTask = (Test) proj.getTasks().findByName(ss.getName());
-                                if (testTask == null) {
-                                    log.warn(
-                                            "Detected 'org:junit.jupiter:junit-jupiter', but unable to find test task");
-                                    return;
-                                }
+                            Optional<Test> maybeTestTask = getTestTaskForSourceSet(proj, ss);
+                            if (!maybeTestTask.isPresent()) {
+                                log.warn("Detected 'org:junit.jupiter:junit-jupiter', but unable to find test task");
+                                return;
                             }
                             log.info("Detected 'org:junit.jupiter:junit-jupiter', enabling useJUnitPlatform() on {}",
-                                    testTask.getName());
-                            enableJunit5ForTestTask(testTask);
+                                    maybeTestTask.get().getName());
+                            enableJunit5ForTestTask(maybeTestTask.get());
                         });
             });
+
+            TaskProvider<CheckJUnitDependencies> task = project.getTasks()
+                    .register("checkJUnitDependencies", CheckJUnitDependencies.class);
+
+            project.getTasks().findByName(JavaPlugin.TEST_TASK_NAME).dependsOn(task);
         });
     }
 
-    private boolean hasCompileDependenciesMatching(Project project, SourceSet sourceSet, Spec<Dependency> spec) {
+    public static Optional<Test> getTestTaskForSourceSet(Project proj, SourceSet ss) {
+        String testTaskName = ss.getTaskName(null, "test");
+
+        Test task1 = (Test) proj.getTasks().findByName(testTaskName);
+        if (task1 != null) {
+            return Optional.of(task1);
+        }
+
+        // unbroken dome does this
+        Test task2 = (Test) proj.getTasks().findByName(ss.getName());
+        return Optional.ofNullable(task2);
+    }
+
+    private static boolean hasCompileDependenciesMatching(Project project, SourceSet sourceSet, Spec<Dependency> spec) {
         return project.getConfigurations()
                 .getByName(sourceSet.getCompileClasspathConfigurationName())
                 .getAllDependencies()
@@ -82,14 +96,12 @@ private boolean hasCompileDependenciesMatching(Project project, SourceSet source
                 .isPresent();
     }
 
-    private boolean isJunitJupiter(Dependency dep) {
-        return Objects.equals(dep.getGroup(), "org.junit.jupiter")
-                && dep.getName().equals("junit-jupiter");
+    private static boolean isJunitJupiter(Dependency dep) {
+        return Objects.equals(dep.getGroup(), "org.junit.jupiter") && dep.getName().equals("junit-jupiter");
     }
 
-    private void enableJunit5ForTestTask(Test task) {
-        TestFrameworkOptions options = task.getOptions();
-        if (!(options instanceof JUnitPlatformOptions)) {
+    private static void enableJunit5ForTestTask(Test task) {
+        if (!useJUnitPlatformEnabled(task)) {
             task.useJUnitPlatform();
         }
 
@@ -105,4 +117,8 @@ private void enableJunit5ForTestTask(Test task) {
         // provide some stdout feedback when tests fail
         task.testLogging(testLogging -> testLogging.events("failed"));
     }
+
+    public static boolean useJUnitPlatformEnabled(Test task) {
+        return task.getOptions() instanceof JUnitPlatformOptions;
+    }
 }

gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java

@@ -0,0 +1,171 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * 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.
+ */
+
+package com.palantir.baseline.tasks;
+
+import com.google.common.base.Preconditions;
+import com.palantir.baseline.plugins.BaselineTesting;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Optional;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
+import org.gradle.api.DefaultTask;
+import org.gradle.api.artifacts.ModuleVersionIdentifier;
+import org.gradle.api.plugins.JavaPluginConvention;
+import org.gradle.api.tasks.SourceSet;
+import org.gradle.api.tasks.TaskAction;
+import org.gradle.api.tasks.testing.Test;
+
+public class CheckJUnitDependencies extends DefaultTask {
+
+    public CheckJUnitDependencies() {
+        setGroup("Verification");
+        setDescription("Ensures the correct JUnit4/5 dependencies are present, otherwise tests may silently not run");
+    }
+
+    @TaskAction
+    public final void validateDependencies() {
+        getProject().getConvention()
+                .getPlugin(JavaPluginConvention.class)
+                .getSourceSets()
+                .forEach(ss -> {
+                    if (ss.getName().equals("main")) {
+                        return;
+                    }
+
+                    Optional<Test> maybeTestTask = BaselineTesting.getTestTaskForSourceSet(getProject(), ss);
+                    if (!maybeTestTask.isPresent()) {
+                        // source set doesn't have a test task, e.g. 'schema'
+                        return;
+                    }
+                    Test task = maybeTestTask.get();
+
+                    getProject().getLogger().info(
+                            "Analyzing source set {} with task {}",
+                            ss.getName(), task.getName());
+                    validateSourceSet(ss, task);
+                });
+    }
+
+    private void validateSourceSet(SourceSet ss, Test task) {
+        boolean junitJupiterIsPresent = hasDep(
+                ss.getRuntimeClasspathConfigurationName(), CheckJUnitDependencies::isJunitJupiter);
+
+        // If some testing library happens to provide the junit-jupiter-api, then users might start using the
+        // org.junit.jupiter.api.Test annotation, but as JUnit4 knows nothing about these, they'll silently not run
+        // unless the user has wired up the dependency correctly.
+        if (sourceSetMentionsJUnit5Api(ss)) {
+            String implementation = ss.getImplementationConfigurationName();
+            Preconditions.checkState(
+                    BaselineTesting.useJUnitPlatformEnabled(task),
+                    "Some tests mention JUnit5, but the '" + task.getName() + "' task does not have "
+                            + "useJUnitPlatform() enabled. This means tests may be silently not running! Please "
+                            + "add the following:\n\n"
+                            + "    " + implementation + " 'org.junit.jupiter:junit-jupiter'\n");
+        }
+
+        // When doing an incremental migration to JUnit5, a project may have some JUnit4 and some JUnit5 tests at the
+        // same time. It's crucial that they have the vintage engine set up correctly, otherwise tests may silently
+        // not run!
+        if (sourceSetMentionsJUnit4(ss)) {
+            if (BaselineTesting.useJUnitPlatformEnabled(task)) { // people might manually enable this
+                String testRuntimeOnly = ss.getRuntimeConfigurationName() + "Only";
+                Preconditions.checkState(
+                        junitJupiterIsPresent,
+                        "Tests may be silently not running! Some tests still use JUnit4, but Gradle has "
+                                + "been set to use JUnit Platform. "
+                                + "To ensure your old JUnit4 tests still run, please add the following:\n\n"
+                                + "    " + testRuntimeOnly + " 'org.junit.jupiter:junit-jupiter'\n\n"
+                                + "Otherwise they will silently not run.");
+
+                boolean vintageEngineExists = hasDep(
+                        ss.getRuntimeClasspathConfigurationName(), CheckJUnitDependencies::isVintageEngine);
+                Preconditions.checkState(
+                        vintageEngineExists,
+                        "Tests may be silently not running! Some tests still use JUnit4, but Gradle has "
+                                + "been set to use JUnit Platform. "
+                                + "To ensure your old JUnit4 tests still run, please add the following:\n\n"
+                                + "    " + testRuntimeOnly + " 'org.junit.vintage:junit-vintage-engine'\n\n"
+                                + "Otherwise they will silently not run.");
+            } else {
+                Preconditions.checkState(
+                        !junitJupiterIsPresent,
+                        "Tests may be silently not running! Please remove "
+                                + "'org.junit.jupiter:junit-jupiter' dependency "
+                                + "because tests use JUnit4 and useJUnitPlatform() is not enabled.");
+            }
+        } else {
+            String compileClasspath = ss.getCompileClasspathConfigurationName();
+            boolean compilingAgainstOldJunit = hasDep(compileClasspath, CheckJUnitDependencies::isJunit4);
+            Preconditions.checkState(
+                    !compilingAgainstOldJunit,
+                    "Extraneous dependency on JUnit4 (no test mentions JUnit4 classes). Please exclude "
+                            + "this from compilation to ensure developers don't accidentally re-introduce it, e.g.\n\n"
+                            + "    configurations." + compileClasspath + ".exclude module: 'junit'\n\n");
+        }
+
+        // sourcesets might also contain Spock classes, but we don't have any special validation for these.
+    }
+
+    private boolean hasDep(String configurationName, Predicate<ModuleVersionIdentifier> spec) {
+        return getProject().getConfigurations()
+                .getByName(configurationName)
+                .getIncoming()
+                .getResolutionResult()
+                .getAllComponents()
+                .stream()
+                .anyMatch(component -> spec.test(component.getModuleVersion()));
+    }
+
+    private boolean sourceSetMentionsJUnit4(SourceSet ss) {
+        return !ss.getAllSource()
+                .filter(file -> fileContainsSubstring(file, l ->
+                        l.contains("org.junit.Test")
+                                || l.contains("org.junit.runner")
+                                || l.contains("org.junit.ClassRule")))
+                .isEmpty();
+    }
+
+    private boolean sourceSetMentionsJUnit5Api(SourceSet ss) {
+        return !ss.getAllSource()
+                .filter(file -> fileContainsSubstring(file, l -> l.contains("org.junit.jupiter.api.")))
+                .isEmpty();
+    }
+
+    private boolean fileContainsSubstring(File file, Predicate<String> substring) {
+        try (Stream<String> lines = Files.lines(file.toPath())) {
+            boolean hit = lines.anyMatch(substring::test);
+            getProject().getLogger().debug("[{}] {}", hit ? "hit" : "miss", file);
+            return hit;
+        } catch (IOException e) {
+            throw new RuntimeException("Unable to check file " + file, e);
+        }
+    }
+
+    private static boolean isJunitJupiter(ModuleVersionIdentifier dep) {
+        return "org.junit.jupiter".equals(dep.getGroup()) && "junit-jupiter".equals(dep.getName());
+    }
+
+    private static boolean isVintageEngine(ModuleVersionIdentifier dep) {
+        return "org.junit.vintage".equals(dep.getGroup()) && "junit-vintage-engine".equals(dep.getName());
+    }
+
+    private static boolean isJunit4(ModuleVersionIdentifier dep) {
+        return "junit".equals(dep.getGroup()) && "junit".equals(dep.getName());
+    }
+}

gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineTestingIntegrationTest.groovy

@@ -104,4 +104,63 @@ class BaselineTestingIntegrationTest extends AbstractPluginTest {
         result.task(':integrationTest').outcome == TaskOutcome.SUCCESS
         new File(projectDir, "build/reports/tests/integrationTest/classes/test.TestClass5.html").exists()
     }
+
+    def 'checkJUnitDependencies ensures mixture of junit4 and 5 tests => legacy must be present'() {
+        when:
+        buildFile << '''
+        plugins {
+            id 'org.unbroken-dome.test-sets' version '2.1.1'
+        }
+        '''.stripIndent()
+        buildFile << standardBuildFile
+        buildFile << '''
+        testSets {
+            integrationTest
+        }
+
+        dependencies {
+            integrationTestImplementation "org.junit.jupiter:junit-jupiter:5.4.2"
+        }
+        '''.stripIndent()
+        file('src/integrationTest/java/test/TestClass2.java') << junit4Test
+        file('src/integrationTest/java/test/TestClass5.java') << junit5Test
+
+        then:
+        BuildResult result = with('checkJUnitDependencies', '--write-locks').buildAndFail()
+        result.output.contains 'Some tests still use JUnit4, but Gradle has been set to use JUnit Platform'
+    }
+
+    def 'checkJUnitDependencies ensures mixture of junit4 and 5 tests => new must be present'() {
+        when:
+        buildFile << standardBuildFile
+        buildFile << '''
+        dependencies {
+            testCompile "junit:junit:4.12"
+        }
+        '''.stripIndent()
+        file('src/test/java/test/TestClass2.java') << junit4Test
+        file('src/test/java/test/TestClass5.java') << junit5Test
+
+        then:
+        BuildResult result = with('checkJUnitDependencies', '--write-locks').buildAndFail()
+        result.output.contains 'Some tests mention JUnit5, but the \'test\' task does not have useJUnitPlatform() enabled'
+    }
+
+    def 'checkJUnitDependencies detects extraneous old junit'() {
+        when:
+        buildFile << standardBuildFile
+        buildFile << '''
+        dependencies {
+            testCompile "junit:junit:4.12"
+        }
+        test {
+          useJUnitPlatform()
+        }
+        '''.stripIndent()
+        file('src/test/java/test/TestClass5.java') << junit5Test
+
+        then:
+        BuildResult result = with('checkJUnitDependencies', '--write-locks').buildAndFail()
+        result.output.contains 'Extraneous dependency on JUnit4 (no test mentions JUnit4 classes)'
+    }
 }