From addac0a7cf2f5227c7ec6789e6a399c5167b6c56 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Mon, 16 Sep 2019 20:04:36 +0100 Subject: [PATCH 1/9] New checkJUnitDependencies task --- .../baseline/plugins/BaselineTesting.java | 56 ++++++--- .../tasks/CheckJUnitDependencies.java | 117 ++++++++++++++++++ 2 files changed, 157 insertions(+), 16 deletions(-) create mode 100644 gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java index 7b70e6bac..ff7892a22 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java @@ -16,16 +16,21 @@ 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.Task; import org.gradle.api.artifacts.Dependency; +import org.gradle.api.artifacts.ModuleVersionIdentifier; +import org.gradle.api.artifacts.result.ResolvedComponentResult; 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; @@ -51,28 +56,37 @@ public void apply(Project project) { 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) { + Optional maybeTestTask = getTestTaskForSourceSet(proj, ss); + if (!maybeTestTask.isPresent()) { // 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) { + maybeTestTask = Optional.ofNullable((Test) proj.getTasks().findByName(ss.getName())); + 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 task = project.getTasks() + .register("checkJUnitDependencies", CheckJUnitDependencies.class); + + project.getTasks().findByName(JavaPlugin.TEST_TASK_NAME).dependsOn(task); }); } - private boolean hasCompileDependenciesMatching(Project project, SourceSet sourceSet, Spec spec) { + public static Optional getTestTaskForSourceSet(Project proj, SourceSet ss) { + String testTaskName = ss.getTaskName(null, "test"); + return Optional.ofNullable((Test) proj.getTasks().findByName(testTaskName)); + } + + private static boolean hasCompileDependenciesMatching(Project project, SourceSet sourceSet, Spec spec) { return project.getConfigurations() .getByName(sourceSet.getCompileClasspathConfigurationName()) .getAllDependencies() @@ -82,14 +96,16 @@ 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"); + public static boolean isJunitJupiter(Dependency dep) { + return Objects.equals(dep.getGroup(), "org.junit.jupiter") && dep.getName().equals("junit-jupiter"); + } + + public static boolean isJunitJupiter(ModuleVersionIdentifier 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 +121,12 @@ 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; + } + + public static boolean isVintageEngine(ModuleVersionIdentifier dep) { + return "org.junit.vintage".equals(dep.getGroup()) && "junit-vintage-engine".equals(dep.getName()); + } } diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java new file mode 100644 index 000000000..6e9a481e5 --- /dev/null +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -0,0 +1,117 @@ +/* + * (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.util.Optional; +import java.util.function.Predicate; +import org.gradle.api.DefaultTask; +import org.gradle.api.GradleException; +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 void validateDependencies() { + getProject().getConvention() + .getPlugin(JavaPluginConvention.class) + .getSourceSets() + .forEach(ss -> { + Optional maybeTestTask = BaselineTesting.getTestTaskForSourceSet(getProject(), ss); + if (!maybeTestTask.isPresent()) { + return; + } + Test task = maybeTestTask.get(); + + validateSourceSet(ss, task); + }); + } + + private void validateSourceSet(SourceSet ss, Test task) { + boolean junitJupiterIsPresent = hasRuntimeDepMatching(ss, BaselineTesting::isJunitJupiter); + + // 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)) { + Preconditions.checkState(junitJupiterIsPresent, + "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" + + " runtimeOnly 'org.junit.jupiter:junit-jupiter'\n\n" + + "Otherwise they will silently not run."); + + boolean vintageEngineExists = hasRuntimeDepMatching(ss, BaselineTesting::isVintageEngine); + Preconditions.checkState(vintageEngineExists, + "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" + + " runtimeOnly 'org.junit.vintage:junit-vintage-engine'\n\n" + + "Otherwise they will silently not run."); + } else { + Preconditions.checkState(!junitJupiterIsPresent, + "Please remove 'org.junit.jupiter:junit-jupiter' from runtimeClasspath " + + "because tests use JUnit4 and useJUnitPlatform() is not enabled."); + + // we're confident they have 'junit:junit' on the classpath already in order to compile + } + } + + // 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)) { + if (BaselineTesting.useJUnitPlatformEnabled(task)) { + Preconditions.checkState(BaselineTesting.useJUnitPlatformEnabled(task), "TODO"); + Preconditions.checkState(junitJupiterIsPresent, "TODO"); + } else { + throw new GradleException("TODO"); + } + } + + // sourcesets might also contain Spock classes, but we don't have any special validation for these. + } + + private boolean hasRuntimeDepMatching(SourceSet ss, Predicate spec) { + return getProject().getConfigurations() + .getByName(ss.getRuntimeClasspathConfigurationName()) + .getIncoming() + .getResolutionResult() + .getAllComponents() + .stream() + .anyMatch(component -> spec.test(component.getModuleVersion())); + } + + private static boolean sourceSetMentionsJUnit4(SourceSet ss) { + // TODO(dfox): implement this + return false; + } + + private static boolean sourceSetMentionsJUnit5Api(SourceSet ss) { + // TODO(dfox): implement this + return false; + } +} From ee463281bdc64508dbe22df17c037e283565d659 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Mon, 16 Sep 2019 20:06:00 +0100 Subject: [PATCH 2/9] README note. --- README.md | 2 ++ .../groovy/com/palantir/baseline/plugins/BaselineTesting.java | 2 -- .../com/palantir/baseline/tasks/CheckJUnitDependencies.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ea4a5d2df..a06bb984e 100644 --- a/README.md +++ b/README.md @@ -373,3 +373,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. diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java index ff7892a22..4a3f92962 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java @@ -21,10 +21,8 @@ import java.util.Optional; import org.gradle.api.Plugin; import org.gradle.api.Project; -import org.gradle.api.Task; import org.gradle.api.artifacts.Dependency; import org.gradle.api.artifacts.ModuleVersionIdentifier; -import org.gradle.api.artifacts.result.ResolvedComponentResult; import org.gradle.api.plugins.JavaPlugin; import org.gradle.api.plugins.JavaPluginConvention; import org.gradle.api.specs.Spec; diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index 6e9a481e5..34a96b5c0 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -36,7 +36,7 @@ public CheckJUnitDependencies() { } @TaskAction - public void validateDependencies() { + public final void validateDependencies() { getProject().getConvention() .getPlugin(JavaPluginConvention.class) .getSourceSets() From 8022b05a977bea73f5f3a4d6c4cb44c8568ca0b8 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Mon, 16 Sep 2019 19:06:00 +0000 Subject: [PATCH 3/9] Add generated changelog entries --- changelog/@unreleased/pr-837.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-837.v2.yml diff --git a/changelog/@unreleased/pr-837.v2.yml b/changelog/@unreleased/pr-837.v2.yml new file mode 100644 index 000000000..7e0c12b3a --- /dev/null +++ b/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 From 082bfda23714fe53e2096649e9cd8393b932ce5a Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 19 Sep 2019 15:35:04 +0100 Subject: [PATCH 4/9] Trivial logs --- .../palantir/baseline/plugins/BaselineTesting.java | 11 ++++++++++- .../baseline/tasks/CheckJUnitDependencies.java | 6 ++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java index 4a3f92962..6273a3f34 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java @@ -21,6 +21,7 @@ import java.util.Optional; import org.gradle.api.Plugin; import org.gradle.api.Project; +import org.gradle.api.Task; import org.gradle.api.artifacts.Dependency; import org.gradle.api.artifacts.ModuleVersionIdentifier; import org.gradle.api.plugins.JavaPlugin; @@ -81,7 +82,15 @@ public void apply(Project project) { public static Optional getTestTaskForSourceSet(Project proj, SourceSet ss) { String testTaskName = ss.getTaskName(null, "test"); - return Optional.ofNullable((Test) proj.getTasks().findByName(testTaskName)); + + 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 spec) { diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index 34a96b5c0..75ff5860d 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -41,12 +41,18 @@ public final void validateDependencies() { .getPlugin(JavaPluginConvention.class) .getSourceSets() .forEach(ss -> { + if (ss.getName().equals("main")) { + return; + } + Optional maybeTestTask = BaselineTesting.getTestTaskForSourceSet(getProject(), ss); if (!maybeTestTask.isPresent()) { return; } Test task = maybeTestTask.get(); + getProject().getLogger().lifecycle("Analyzing source set {} with task {}", + ss.getName(), task.getName()); validateSourceSet(ss, task); }); } From 608aadb229b812e9556c06f0b721ab1dd31622d1 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 19 Sep 2019 15:58:40 +0100 Subject: [PATCH 5/9] Higher quality error messages --- .../tasks/CheckJUnitDependencies.java | 58 +++++++++++++------ 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index 75ff5860d..dcfd99bcc 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -18,8 +18,12 @@ 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.GradleException; import org.gradle.api.artifacts.ModuleVersionIdentifier; @@ -65,20 +69,25 @@ private void validateSourceSet(SourceSet ss, Test task) { // not run! if (sourceSetMentionsJUnit4(ss)) { if (BaselineTesting.useJUnitPlatformEnabled(task)) { - Preconditions.checkState(junitJupiterIsPresent, - "Some tests still use JUnit4, but Gradle has been set to use JUnit Platform." + 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" + " runtimeOnly 'org.junit.jupiter:junit-jupiter'\n\n" + "Otherwise they will silently not run."); boolean vintageEngineExists = hasRuntimeDepMatching(ss, BaselineTesting::isVintageEngine); - Preconditions.checkState(vintageEngineExists, - "Some tests still use JUnit4, but Gradle has been set to use JUnit Platform." + 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" + " runtimeOnly 'org.junit.vintage:junit-vintage-engine'\n\n" + "Otherwise they will silently not run."); } else { - Preconditions.checkState(!junitJupiterIsPresent, + Preconditions.checkState( + !junitJupiterIsPresent, "Please remove 'org.junit.jupiter:junit-jupiter' from runtimeClasspath " + "because tests use JUnit4 and useJUnitPlatform() is not enabled."); @@ -90,12 +99,12 @@ private void validateSourceSet(SourceSet ss, Test task) { // 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)) { - if (BaselineTesting.useJUnitPlatformEnabled(task)) { - Preconditions.checkState(BaselineTesting.useJUnitPlatformEnabled(task), "TODO"); - Preconditions.checkState(junitJupiterIsPresent, "TODO"); - } else { - throw new GradleException("TODO"); - } + 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:jupiter-engine'\n"); } // sourcesets might also contain Spock classes, but we don't have any special validation for these. @@ -111,13 +120,28 @@ private boolean hasRuntimeDepMatching(SourceSet ss, Predicate spec.test(component.getModuleVersion())); } - private static boolean sourceSetMentionsJUnit4(SourceSet ss) { - // TODO(dfox): implement this - return false; + 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 static boolean sourceSetMentionsJUnit5Api(SourceSet ss) { - // TODO(dfox): implement this - return false; + 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 substring) { + try (Stream lines = Files.lines(file.toPath())) { + boolean hit = lines.anyMatch(substring::test); + getProject().getLogger().lifecycle("[{}] {}", hit ? "hit" : "miss", file); + return hit; + } catch (IOException e) { + throw new RuntimeException("Unable to check file " + file, e); + } } } From 85f649e0b43448a8371b6a7d3caa0fd857eee0c6 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 19 Sep 2019 16:11:15 +0100 Subject: [PATCH 6/9] Improve errors more --- .../baseline/plugins/BaselineTesting.java | 12 +------- .../tasks/CheckJUnitDependencies.java | 29 ++++++++++++------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java index 6273a3f34..b8bdb533c 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java @@ -21,9 +21,7 @@ import java.util.Optional; import org.gradle.api.Plugin; import org.gradle.api.Project; -import org.gradle.api.Task; import org.gradle.api.artifacts.Dependency; -import org.gradle.api.artifacts.ModuleVersionIdentifier; import org.gradle.api.plugins.JavaPlugin; import org.gradle.api.plugins.JavaPluginConvention; import org.gradle.api.specs.Spec; @@ -103,11 +101,7 @@ private static boolean hasCompileDependenciesMatching(Project project, SourceSet .isPresent(); } - public static boolean isJunitJupiter(Dependency dep) { - return Objects.equals(dep.getGroup(), "org.junit.jupiter") && dep.getName().equals("junit-jupiter"); - } - - public static boolean isJunitJupiter(ModuleVersionIdentifier dep) { + private static boolean isJunitJupiter(Dependency dep) { return Objects.equals(dep.getGroup(), "org.junit.jupiter") && dep.getName().equals("junit-jupiter"); } @@ -132,8 +126,4 @@ private static void enableJunit5ForTestTask(Test task) { public static boolean useJUnitPlatformEnabled(Test task) { return task.getOptions() instanceof JUnitPlatformOptions; } - - public static boolean isVintageEngine(ModuleVersionIdentifier dep) { - return "org.junit.vintage".equals(dep.getGroup()) && "junit-vintage-engine".equals(dep.getName()); - } } diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index dcfd99bcc..92fe002dd 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -25,7 +25,6 @@ import java.util.function.Predicate; import java.util.stream.Stream; import org.gradle.api.DefaultTask; -import org.gradle.api.GradleException; import org.gradle.api.artifacts.ModuleVersionIdentifier; import org.gradle.api.plugins.JavaPluginConvention; import org.gradle.api.tasks.SourceSet; @@ -51,18 +50,20 @@ public final void validateDependencies() { Optional 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().lifecycle("Analyzing source set {} with task {}", + getProject().getLogger().info( + "Analyzing source set {} with task {}", ss.getName(), task.getName()); validateSourceSet(ss, task); }); } private void validateSourceSet(SourceSet ss, Test task) { - boolean junitJupiterIsPresent = hasRuntimeDepMatching(ss, BaselineTesting::isJunitJupiter); + boolean junitJupiterIsPresent = hasRuntimeDepMatching(ss, CheckJUnitDependencies::isJunitJupiter); // 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 @@ -77,21 +78,21 @@ private void validateSourceSet(SourceSet ss, Test task) { + " runtimeOnly 'org.junit.jupiter:junit-jupiter'\n\n" + "Otherwise they will silently not run."); - boolean vintageEngineExists = hasRuntimeDepMatching(ss, BaselineTesting::isVintageEngine); + boolean vintageEngineExists = hasRuntimeDepMatching(ss, CheckJUnitDependencies::isVintageEngine); + String testRuntimeOnly = ss.getRuntimeConfigurationName() + "Only"; 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" - + " runtimeOnly 'org.junit.vintage:junit-vintage-engine'\n\n" + + " " + testRuntimeOnly + " 'org.junit.vintage:junit-vintage-engine'\n\n" + "Otherwise they will silently not run."); } else { Preconditions.checkState( !junitJupiterIsPresent, - "Please remove 'org.junit.jupiter:junit-jupiter' from runtimeClasspath " + "Tests may be silently not running! Please remove " + + "'org.junit.jupiter:junit-jupiter' dependency " + "because tests use JUnit4 and useJUnitPlatform() is not enabled."); - - // we're confident they have 'junit:junit' on the classpath already in order to compile } } @@ -104,7 +105,7 @@ private void validateSourceSet(SourceSet ss, Test 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:jupiter-engine'\n"); + + " implementation 'org.junit.jupiter:junit-jupiter'\n"); } // sourcesets might also contain Spock classes, but we don't have any special validation for these. @@ -138,10 +139,18 @@ private boolean sourceSetMentionsJUnit5Api(SourceSet ss) { private boolean fileContainsSubstring(File file, Predicate substring) { try (Stream lines = Files.lines(file.toPath())) { boolean hit = lines.anyMatch(substring::test); - getProject().getLogger().lifecycle("[{}] {}", hit ? "hit" : "miss", file); + 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()); + } } From bcda92504c8877897528729c65c49729a3ce253b Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 19 Sep 2019 16:22:34 +0100 Subject: [PATCH 7/9] Also prompt people to fully exclude old junit when possible --- .../tasks/CheckJUnitDependencies.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index 92fe002dd..1a26deb19 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -63,7 +63,8 @@ public final void validateDependencies() { } private void validateSourceSet(SourceSet ss, Test task) { - boolean junitJupiterIsPresent = hasRuntimeDepMatching(ss, CheckJUnitDependencies::isJunitJupiter); + boolean junitJupiterIsPresent = hasDep( + ss.getRuntimeClasspathConfigurationName(), CheckJUnitDependencies::isJunitJupiter); // 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 @@ -78,7 +79,8 @@ private void validateSourceSet(SourceSet ss, Test task) { + " runtimeOnly 'org.junit.jupiter:junit-jupiter'\n\n" + "Otherwise they will silently not run."); - boolean vintageEngineExists = hasRuntimeDepMatching(ss, CheckJUnitDependencies::isVintageEngine); + boolean vintageEngineExists = hasDep( + ss.getRuntimeClasspathConfigurationName(), CheckJUnitDependencies::isVintageEngine); String testRuntimeOnly = ss.getRuntimeConfigurationName() + "Only"; Preconditions.checkState( vintageEngineExists, @@ -94,6 +96,14 @@ private void validateSourceSet(SourceSet ss, Test task) { + "'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"); } // If some testing library happens to provide the junit-jupiter-api, then users might start using the @@ -111,9 +121,9 @@ private void validateSourceSet(SourceSet ss, Test task) { // sourcesets might also contain Spock classes, but we don't have any special validation for these. } - private boolean hasRuntimeDepMatching(SourceSet ss, Predicate spec) { + private boolean hasDep(String configurationName, Predicate spec) { return getProject().getConfigurations() - .getByName(ss.getRuntimeClasspathConfigurationName()) + .getByName(configurationName) .getIncoming() .getResolutionResult() .getAllComponents() @@ -153,4 +163,8 @@ private static boolean isJunitJupiter(ModuleVersionIdentifier dep) { 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()); + } } From ca240d8fca69d326630392b39846c25d464688cd Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 19 Sep 2019 16:57:56 +0100 Subject: [PATCH 8/9] integration tests --- .../baseline/plugins/BaselineErrorProne.java | 2 +- .../baseline/plugins/BaselineTesting.java | 11 +--- .../tasks/CheckJUnitDependencies.java | 2 +- .../BaselineTestingIntegrationTest.groovy | 59 +++++++++++++++++++ 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java index a154f9da4..3d7442c5c 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java @@ -153,7 +153,7 @@ public void apply(Project project) { // To allow refactoring of deprecated methods, even when -Xlint:deprecation is specified, we need to remove // these compiler flags after all configuration has happened. - project.afterEvaluate(unused -> project.getTasks().withType(JavaCompile.class) + project.afterEvaluate(unusedProject -> project.getTasks().withType(JavaCompile.class) .configureEach(javaCompile -> { if (javaCompile.equals(compileRefaster)) { return; diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java index b8bdb533c..a5b9cff39 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java @@ -47,7 +47,7 @@ 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() @@ -57,13 +57,8 @@ public void apply(Project project) { .forEach(ss -> { Optional maybeTestTask = getTestTaskForSourceSet(proj, ss); if (!maybeTestTask.isPresent()) { - // Fall back to the source set name, since that is what gradle-testsets-plugin does - maybeTestTask = Optional.ofNullable((Test) proj.getTasks().findByName(ss.getName())); - if (!maybeTestTask.isPresent()) { - log.warn( - "Detected 'org:junit.jupiter:junit-jupiter', but unable to find test task"); - return; - } + 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 {}", maybeTestTask.get().getName()); diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index 1a26deb19..a7c001c1e 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -70,7 +70,7 @@ private void validateSourceSet(SourceSet ss, Test task) { // 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)) { + if (BaselineTesting.useJUnitPlatformEnabled(task)) { // people might manually enable this Preconditions.checkState( junitJupiterIsPresent, "Tests may be silently not running! Some tests still use JUnit4, but Gradle has " diff --git a/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineTestingIntegrationTest.groovy b/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineTestingIntegrationTest.groovy index d41ee6205..69b6a0cd8 100644 --- a/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineTestingIntegrationTest.groovy +++ b/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)' + } } From ce06878ab7dd697cb5a18ed715f043a9adbdcff8 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 19 Sep 2019 17:07:01 +0100 Subject: [PATCH 9/9] Re-order --- .../tasks/CheckJUnitDependencies.java | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java index a7c001c1e..3b679425a 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/tasks/CheckJUnitDependencies.java @@ -66,22 +66,35 @@ 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" - + " runtimeOnly 'org.junit.jupiter:junit-jupiter'\n\n" + + " " + testRuntimeOnly + " 'org.junit.jupiter:junit-jupiter'\n\n" + "Otherwise they will silently not run."); boolean vintageEngineExists = hasDep( ss.getRuntimeClasspathConfigurationName(), CheckJUnitDependencies::isVintageEngine); - String testRuntimeOnly = ss.getRuntimeConfigurationName() + "Only"; Preconditions.checkState( vintageEngineExists, "Tests may be silently not running! Some tests still use JUnit4, but Gradle has " @@ -106,18 +119,6 @@ private void validateSourceSet(SourceSet ss, Test task) { + " configurations." + compileClasspath + ".exclude module: 'junit'\n\n"); } - // 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)) { - 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"); - } - // sourcesets might also contain Spock classes, but we don't have any special validation for these. }