Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use JUnit Platform by default #3053

Merged
merged 8 commits into from
Feb 27, 2025
Merged

Use JUnit Platform by default #3053

merged 8 commits into from
Feb 27, 2025

Conversation

pkoenig10
Copy link
Member

@pkoenig10 pkoenig10 commented Feb 24, 2025

The JUnit 5.12.0 upgrade is causing test failures in all of our repos. See some prior discussion in junit-team/junit5#4335.

Caused by: org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests
        ...
Caused by: org.junit.platform.commons.JUnitException: OutputDirectoryProvider not available; probably due to unaligned versions of the junit-platform-engine and junit-platform-launcher jars on the classpath/module path.
        ...

The Gradle documentation states that, in the absence of test suites, the necessary test framework dependencies (ex. junit-platform-launcher) must be explicitly declared. I think we've just gotten lucky that nothing in the JUnit implementation has actually required this dependency until now.

The best solution here is to migrate BaselineTesting to configure tests using the newer jvm-test-suites plugin, which is applied automatically be the java plugin and already used to configured the default test test suite. The test suites plugin will automatically add the necessary test framework dependencies, and consumers can use their existing version management tool (ex. gradle-consistent-versions) to ensure consistent versions of the JUnit dependencies.

Consumers will just need to add a org.junit.platform:* constraints in their versions.props. We can easily automate this (this is already done).

@changelog-app
Copy link

changelog-app bot commented Feb 24, 2025

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

BaselineTesting now configures tests to run using JUnit Platform by default. Tests are configured using the test suites API, which ensures that test framework dependencies are added automatically.

Check the box to generate changelog(s)

  • Generate changelog entry

Sorry, something went wrong.

@pkoenig10 pkoenig10 force-pushed the pkoenig/testSuites branch 4 times, most recently from 41b91a2 to 8f9938c Compare February 24, 2025 17:30
Comment on lines 66 to 76
testingExtension
.getSuites()
.named(JvmTestSuitePlugin.DEFAULT_TEST_SUITE_NAME, JvmTestSuite.class, testSuite -> {
testSuite.useJUnitJupiter();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility reasons, Gradle uses the legacy JUnit4 test toolchain for the default test test suite and the JUnit Platform test toolchain for everything else.

https://github.com/gradle/gradle/blob/4bb4af6421896c3ef094349be8e33098d343db2b/platforms/jvm/plugins-jvm-test-suite/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java#L91-L100

Here we configure the default test test suite to also use the JUnit Platform test toolchain.

Comment on lines 80 to 90
// See https://github.com/gradle/gradle/pull/21919
if (GradleVersion.current().compareTo(GRADLE_8) < 0) {
project.getDependencies()
.add(sourceSet.getRuntimeOnlyConfigurationName(), JUNIT_PLATFORM_LAUNCHER);
}
Copy link
Member Author

@pkoenig10 pkoenig10 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gradle didn't start automatically adding this dependency until Gradle 8, so for earlier versions of Gradle we replicate that behavior here.

gradle/gradle#21919

Once we drop support for Gradle 7, this should be removed.

Comment on lines 86 to 96
// For test tasks not creating using test suites, we must explicitly the test to use JUnit Platform
// and add the junit-platform-launcher dependency.
if (testingExtension.getSuites().findByName(sourceSet.getName()) == null) {
task.useJUnitPlatform();
project.getDependencies()
.add(sourceSet.getRuntimeOnlyConfigurationName(), JUNIT_PLATFORM_LAUNCHER);
Copy link
Member Author

@pkoenig10 pkoenig10 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only exists to support the testsets plugin.

Once we migrate users from the testsets plugin to test suites, this should be removed.

}
return Optional.empty();
}
javaPluginExtension.getSourceSets().configureEach(sourceSet -> {
Copy link
Member Author

@pkoenig10 pkoenig10 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit weird that we iterate over source sets to find tests. Once we no longer support the testsets plugin (and thus, all tests are declared using test suites), we should change this to iterating over the test suites instead. Same goes for the CheckJUnitDependencies task.

@Property
void test(@ForAll byte value) {}
}
'''.stripIndent(true)

def '#gradleVersionNumber: capable of running both junit4 and junit5 tests'() {
def '#gradleVersionNumber: runs JUnit4 tests'() {
when:
gradleVersion = gradleVersionNumber
Copy link
Member Author

@pkoenig10 pkoenig10 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one of these tests was testing on both Gradle 7 and 8. I've updated these tests so we're testing both Gradle versions on all of the tests (except a couple at the bottom where the Gradle version is not relevant).

Comment on lines -73 to -75
private Stream<SourceSet> getProbablyTestSourceSets() {
return getProject().getExtensions().getByType(JavaPluginExtension.class).getSourceSets().stream()
.filter(ss -> !ss.getName().equals(SourceSet.MAIN_SOURCE_SET_NAME));
Copy link
Member Author

@pkoenig10 pkoenig10 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this filtering because:

  • It's inconsistent with the BaselineTesting plugin, where we don't filter the source sets.
  • It's almost certainly unnecessary. If it's a Test task, we probably want to perform this validation.

+ ss.getRuntimeOnlyConfigurationName()
+ " 'net.jqwik:jqwik-engine'\n\n");
}
} else {
Copy link
Member Author

@pkoenig10 pkoenig10 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, we should never reach this unless someone has explicitly configured a test suite to use something other than JUnit Platform. I don't think we care to support any other test frameworks (all the things we we care about integrate with JUnit Platform using a custom engine), so I'm tempted to just remove this.

@pkoenig10 pkoenig10 requested a review from crogoz February 24, 2025 19:03
@pkoenig10
Copy link
Member Author

pkoenig10 commented Feb 24, 2025

There may be a small number of repos that require manual changes after this upgrade. Those are repos with projects that exclusively use JUnit4 and are using the legacy JUnit4 test runner (which is the default) and not the JUnit Platform test runner.

Given that we performed a pretty extensive JUnit5 migration, the number of internal repos that meet this criteria is quite small (internal code search seems to confirm this, although I can't be certain from textual search alone). It is likely limited to the handful of repos that explicitly disabled the JUnit5 migration.

These repos don't have to migrate any of their tests to use JUnit5 APIs - the just need to migrate to use the JUnit Platform test runner (running their JUnit4 tests using the JUnit Vintage engine).

Alternatively, these repos can explicitly set the test runner to the JUnit4 test runner. I've verified that this works as expected, removing the JUnit Platorms dependencies rather than adding to them.

testing {
    suites {
        test {
            useJUnit()
        }
    }
}

@pkoenig10 pkoenig10 changed the title Implement BaselineTesting using test suites Use JUnit Platform by default Feb 25, 2025
// 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 runtime = ss.getRuntimeClasspathConfigurationName();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produces incorrect suggestions, dependencies should be declared using the runtime only configuration, not the runtime classpath configuration.

@pkoenig10 pkoenig10 force-pushed the pkoenig/testSuites branch 4 times, most recently from 7da5c0b to e2de58a Compare February 26, 2025 14:20
private void validateSourceSet(SourceSet ss, Test task) {
Set<ResolvedComponentResult> deps = getProject()
.getConfigurations()
.getByName(ss.getRuntimeClasspathConfigurationName())
.getIncoming()
.getResolutionResult()
.getAllComponents();
boolean junitJupiterIsPresent = hasDep(deps, CheckJUnitDependencies::isJunitJupiter);
boolean vintageEngineExists = hasDep(deps, CheckJUnitDependencies::isVintageEngine);
boolean spock1Dependency = hasDep(deps, CheckJUnitDependencies::isSpock1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal code search shows that there are no remaining users of Spock 1, so I've removed this.

@pkoenig10 pkoenig10 force-pushed the pkoenig/testSuites branch 4 times, most recently from d7e08f4 to 732c800 Compare February 26, 2025 14:54
Comment on lines 84 to 86
// We must eagerly create the test task in order to add the junit-platform-launcher dependency.
// Otherwise the dependencies will already have been resolved by the time we create the test task.
testTasks.all(task -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like this might be happening because the configuration to add the dependency happens on the dependencyset rather than test task or on source sets, so lazily configuring the test task/source set will not add the dep at the right time. I wonder if we can configure the dependencies of each Configuration instead - this is pretty much as lazy as possible, only being generated when the Configuration is resolved. Something like:

project.getConfigurations().configureEach(configuration -> {
    configuration.getDependencies().addAllLater(project.provider(() -> {
        for (Test test : project.getTasks().withType(Test.class)) {
            SourceSet sourceSet = sourceSets.getByName(test.getName());
            if (testingExtension.getSuites().findByName(sourceSet.getName()) != null) {
                return Set.of();
            }

            if (configuration.getName().equals(sourceSet.getImplementationConfigurationName())) {
                return Set.of(project.getDependencies().create(JUNIT_JUPITER));
            }

            if (configuration.getName().equals(sourceSet.getRuntimeOnlyConfigurationName())) {
                return Set.of(project.getDependencies().create(JUNIT_PLATFORM_LAUNCHER));
            }
        }

        return Set.of();
    }));
});

If addAllLater doesn't work (maybe someone iterates over the dependencyset) maybe beforeResolve will do the trick.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we need to do the gradle 8 checking too

task.useJUnitPlatform(); below can happen in a configureEach now, no need for .all.

Comment on lines 92 to 93
// For test tasks not created using test suites, we must explicitly the test to use JUnit Platform
// and add the junit-platform-launcher dependency.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this effectively just going to be people still using test-sets?

Copy link
Member Author

@pkoenig10 pkoenig10 Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be my expectation, but who knows.

javaPluginExtension.getSourceSets().configureEach(sourceSet -> {
TaskCollection<Test> testTasks = project.getTasks()
.withType(Test.class)
.matching(task -> task.getName().equals(sourceSet.getName()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun fact, I recently discovered that .matching realises all the tasks immediately, not just .all:


public CheckJUnitDependencies() {
setGroup("Verification");
setDescription("Ensures the correct JUnit4/5 dependencies are present, otherwise tests may silently not run");
getOutputs().upToDateWhen(_task -> true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tasks with no outputs are never up-to-date

…s/others stuff
@CRogers
Copy link
Contributor

CRogers commented Feb 27, 2025

👍

1 similar comment
@pkoenig10
Copy link
Member Author

👍

@pkoenig10
Copy link
Member Author

Thanks for fixing this up!

@FinlayRJW
Copy link
Contributor

👍

@bulldozer-bot bulldozer-bot bot merged commit 783256d into develop Feb 27, 2025
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the pkoenig/testSuites branch February 27, 2025 17:23
@autorelease3
Copy link

autorelease3 bot commented Feb 27, 2025

Released 6.17.0

@pkoenig10
Copy link
Member Author

pkoenig10 commented Feb 28, 2025

Ran into one small papercut in a repo that does not have any tests.

Because it does not have any tests, it does not currently have a org.junit.platform dependency, so our automation did not add a org.junit.platform: constraint to this repo.

Gradle declares a versioned dependency for junit-jupiter but not for junit-platform-launcher. Because of this dependency resolution fails:

$ ./gradlew --no-scan example:dependencies --configuration testRuntimeClasspath

> Task :example:dependencies

------------------------------------------------------------
Project ':example'
------------------------------------------------------------

testRuntimeClasspath - Runtime classpath of source set 'test'.
+--- root project :
|    \--- root project : (*)
+--- root project : (*)
+--- org.junit.jupiter:junit-jupiter:5.8.2
|    +--- org.junit.jupiter:junit-jupiter-api:5.8.2
|    |    +--- org.opentest4j:opentest4j:1.2.0
|    |    +--- org.junit.platform:junit-platform-commons:1.8.2
|    |    |    \--- org.apiguardian:apiguardian-api:1.1.2
|    |    \--- org.apiguardian:apiguardian-api:1.1.2
|    +--- org.junit.jupiter:junit-jupiter-params:5.8.2
|    |    +--- org.junit.jupiter:junit-jupiter-api:5.8.2 (*)
|    |    \--- org.apiguardian:apiguardian-api:1.1.2
|    \--- org.junit.jupiter:junit-jupiter-engine:5.8.2
|         +--- org.junit.platform:junit-platform-engine:1.8.2
|         |    +--- org.opentest4j:opentest4j:1.2.0
|         |    +--- org.junit.platform:junit-platform-commons:1.8.2 (*)
|         |    \--- org.apiguardian:apiguardian-api:1.1.2
|         +--- org.junit.jupiter:junit-jupiter-api:5.8.2 (*)
|         \--- org.apiguardian:apiguardian-api:1.1.2
\--- org.junit.platform:junit-platform-launcher FAILED

The test suites useJUnitJupiter() API allows you to set the version of junit-jupiter dependency but AFAICT there is not way to set the version of the junit-platform-launcher by setting the JUnitPlatformToolchainParameters.getPlatformVersion() property.

I'm not sure how much effort we should put in to address this - I'm not sure how common it is to have a repo with no tests.

The ideal solution is to ensure that every repo has a org.junit.platform:* constraint.

Alternatively we could unconditionally add a versioned junit-platform-launcher here, but I'd prefer not to take this responsibility from Gradle.

I'm considering putting up a Gradle PR proposing that JUnitPlatformTestToolchain fallback to a default versioned dependency rather than a versionless dependency.

@pkoenig10
Copy link
Member Author

Hmm I think there's something else going on here. This works with a super minimal repro.

Maybe GCV is somehow causing issues?

plugins {
    id 'java'
}

repositories {
    mavenCentral()
}

testing {
    suites {
        test {
            useJUnitJupiter()
        }
    }
}
$ ./gradlew --no-scan dependencies --configuration testRuntimeClasspath

> Task :dependencies

------------------------------------------------------------
Root project 'example'
------------------------------------------------------------

testRuntimeClasspath - Runtime classpath of source set 'test'.
+--- org.junit.jupiter:junit-jupiter:5.8.2
|    +--- org.junit:junit-bom:5.8.2
|    |    +--- org.junit.jupiter:junit-jupiter:5.8.2 (c)
|    |    +--- org.junit.jupiter:junit-jupiter-api:5.8.2 (c)
|    |    +--- org.junit.jupiter:junit-jupiter-engine:5.8.2 (c)
|    |    +--- org.junit.jupiter:junit-jupiter-params:5.8.2 (c)
|    |    +--- org.junit.platform:junit-platform-launcher:1.8.2 (c)
|    |    +--- org.junit.platform:junit-platform-commons:1.8.2 (c)
|    |    \--- org.junit.platform:junit-platform-engine:1.8.2 (c)
|    +--- org.junit.jupiter:junit-jupiter-api:5.8.2
|    |    +--- org.junit:junit-bom:5.8.2 (*)
|    |    +--- org.opentest4j:opentest4j:1.2.0
|    |    \--- org.junit.platform:junit-platform-commons:1.8.2
|    |         \--- org.junit:junit-bom:5.8.2 (*)
|    +--- org.junit.jupiter:junit-jupiter-params:5.8.2
|    |    +--- org.junit:junit-bom:5.8.2 (*)
|    |    \--- org.junit.jupiter:junit-jupiter-api:5.8.2 (*)
|    \--- org.junit.jupiter:junit-jupiter-engine:5.8.2
|         +--- org.junit:junit-bom:5.8.2 (*)
|         +--- org.junit.platform:junit-platform-engine:1.8.2
|         |    +--- org.junit:junit-bom:5.8.2 (*)
|         |    +--- org.opentest4j:opentest4j:1.2.0
|         |    \--- org.junit.platform:junit-platform-commons:1.8.2 (*)
|         \--- org.junit.jupiter:junit-jupiter-api:5.8.2 (*)
\--- org.junit.platform:junit-platform-launcher -> 1.8.2
     +--- org.junit:junit-bom:5.8.2 (*)
     \--- org.junit.platform:junit-platform-engine:1.8.2 (*)

@pkoenig10
Copy link
Member Author

Oh this is because of our internal disable-module-metadata plugin.

https://github.com/junit-team/junit5/blob/c4faa6c26918fbbb8e3dbbde0e8d5077c8873d7c/junit-jupiter/junit-jupiter.gradle.kts#L8

Will continue the discussion internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants