Support multiple junit-platform.properties on the classpath

[famod]

I implemented a ClassOrderer for Quarkus and the idea was to enable that orderer by default for all Quarkus users (and also for the Quarkus CI).
But I overlooked the fact that junit-platform.properties is only expected once on the classpath and if there already is a project-specific properties file (e.g. with junit.jupiter.extensions.autodetection.enabled) there are many warnings issued by this code: https://github.com/junit-team/junit5/blob/r5.8.2/junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/LauncherConfigurationParameters.java#L232-L236

Btw, that orderer is configurable, so the original idea also was to just add (as a Quarkus user) a custom junit-platform.properties to your project with only the respective property (without repeating junit.jupiter.testclass.order.default).

It would come in very handy if all junit-platform.properties on the classpath would be merged, in the order as they appear on the cp.
I'm not sure whether a flag to go back to the previous behavior would be required, which is a bit of a chicken and egg situation I guess (where to put it?).

[famod]

Hi @marcphilipp, may I ask whether the team had time to discuss this?

[mpkorstanje]

Note: Not part of the JUnit team.

It seems to me that depending on the classpath order to determine the configuration of JUnit is fraught with unpredictability and would make it hard to reason about the effective configuration.

However the JUnit Platform accepts configuration from a few different sources, the properties file being only one of them.

Now I'm not familiar with Quarkus but I would expect that all projects inherit from some parent-pom. Have you considered providing the property through Surefire instead?

[famod]

Now I'm not familiar with Quarkus but I would expect that all projects inherit from some parent-pom. Have you considered providing the property through Surefire instead?

There is no parent pom in Quarkus but a BOM instead (which cannot transport plugin config).
Generated apps could be covered this way, but:
a) not everyone uses that generation
b) there are many existing projects out there
c) surefire config is not picked automatically in Eclipse IDE (and maybe others)

PS: ...and there is also Gradle.

[marcelstoer]

We were just bitten by this as well. Our project (transitively) depends on io.quarkus:quarkus-junit5-properties which ships a junit-platform.properties (source). This now clashes with our own file.

#replaces the need to set @TestInstance(TestInstance.Lifecycle.PER_CLASS) on every test class
junit.jupiter.testinstance.lifecycle.default=per_class

It seems to me that depending on the classpath order to determine the configuration of JUnit is fraught with unpredictability

👍

This is particularly so as the JUnit guide warns "It is therefore recommended to change the default in the JUnit Platform configuration file instead of via a JVM system property."

At the moment, the only sensible remedy appears to be to exclude the Quarkus dependency and to copy-paste their JUnit instructions into our junit-platform.properties.

[marcphilipp]

@famod Sorry for taking so long to get back to you. We finally got around to discussing this issue in the team. I'll follow-up with some brainstorming ideas what we can discuss here.

[marcphilipp]

Supporting multiple junit-platform.properties raises a number of issues. For example, how should conflicting values of the same property be merged. If this would throw an exception, there would be no way to resolve this by test authors.

Idea 1: Separate "default" properties files

Instead of supporting multiple junit-platform.properties files on the classpath, a new path (for example, META-INF/junit-platform-defaults.properties would be used that is designed to be used by third-party artifacts like the Quarkus one to define sensible defaults. Test authors could still override the properties in those files in their junit-platform.properties file which would always take precedence. Whether conflicts in META-INF/junit-platform-defaults.properties would cause failures when unresolved in junit-platform.properties or just be logged as warnings needs to be discussed as well.

Idea 2: Imports

imports=/META-INF/quarkus/junit-platform.properties,...

To avoid having to copy the values from Quarkus' properties file which could get outdated when updating Quarkus, a special import property would be supported that would contain a list of classpath resource paths that should be read. The downside of this approach would be that the Quarkus defaults wouldn't get applied without test authors adding the import to their junit-platform.properties.

Idea 3: ServiceLoader-like indirection

A new PropertyProvider interface would be added to the JUnit Platform that could either return properties directly or URLs to classpath resources.

Those providers could be registered via ServiceLoader, allowing to implicitly applying defaults as in the Quarkus use case.

Alternatively, they could be registered or filtered in junit-platform.properties:

junit.platform.properties.providers.includes=org.quarkus.junit.QuarkusJUnitPropertyProvider

If anyone else has other suggestions, please let us know.

[dhoard]

Idea 1

This approach would limit JUnit to 2 properties files.

Idea 2

This seems the easiest to understand and most flexible. Properties are loaded and applied based on the order listed in imports=

junit-platform.properties, /META-INF/quarkus/junit-platform.properties,...

Idea 3

Writing code to load properties (org.quarkus.junit.QuarkusJUnitPropertyProvider) seems like a developer burden.

[sormuras]

Idea 3
Writing code to load properties (org.quarkus.junit.QuarkusJUnitPropertyProvider) seems like a developer burden.

A burden for whom?

Only for JUnit team members and library authors (like Quarkus) that want to provide tailored (default) properties (overrides).
And in addition to ServiceLoader SPI working like a charm with Java 6+ it also behaves on the module path.

[sbrannen]

This approach would limit JUnit to 2 properties files.

@dhoard, perhaps it's not clear from the explanation, but that's not correct.

Idea 1 would find all META-INF/junit-platform-defaults.properties files present in the classpath and somehow merge them.

[mpkorstanje]

If anyone else has other suggestions, please let us know.

What I think we are trying to achieve isn't really about merging properties.

Rather it is about seamless configuration based on the available dependencies. Something like Spring Boots auto configuration. And with that in mind, there are only a handful of properties for which it would make sense to auto configure something.

1. junit.jupiter.testclass.order.default
2. junit.jupiter.testmethod.order.default
3. junit.jupiter.tempdir.factory.default
4. junit.jupiter.displayname.generator.default
5. junit.jupiter.execution.parallel.config.custom.class

With a ServiceLoader for each class configured by the property and a general set of rules on how to resolve conflicts when multiple service loaders for the same class are present it should be possible to automatically enable sensible things without also exposing users to the possibility of having to search their entire class path for potential sources of settings and having to rely on opinionated third parties to actually provide sensible settings.

[dhoard]

@sbrannen @mpkorstanje thanks for the clarification.

[mpkorstanje]

I wasn't clarifying. That was a different proposal. 😉

[marcelstoer]

Supporting multiple junit-platform.properties raises a number of issues. For example, how should conflicting values of the same property be merged.

I can think of only this one being an issue (what are the others?). Hence, I want to propose

Idea 4: Aggregation without auto-merging

Collect all junit-platform.properties from the classpath but give precedence to my own. If you encounter multiple conflicting values for the same property in other files but mine, throw an exception. I can then pick the one value I'd like to effectively use and add it to my own junit-platform.properties thereby resolving the conflict. Advantages:

• library authors won't need to change anything
• I only need to take action in case of conflicts
• in most cases (my expectation) no manual intervention is necessary

Issues with other proposals

Here's why I prefer my approach.

Idea 1:

Forcing library authors to use META-INF/junit-platform-defaults.properties will lead to slow adoption. It will take time until they all released new versions. For this proposal to become effective, it takes their new versions and a new JUnit.

Idea 2:

I may not even know that some of my dependencies include their own junit-platform-defaults.properties. Hence, I might struggle to find the correct imports.

Idea 1:

Pretty much the same concern as with idea 1.

[sbrannen]

Collect all junit-platform.properties from the classpath but give precedence to my own.

@marcelstoer, how do you propose that JUnit should detect which one is "your own"?

Please keep in mind that developers may choose to package their own properties file in a JAR and that the structure of the classpath (filesystem directories, JARs, exploded JARs, etc). may be different in the build vs. in the IDE, etc.

[marcelstoer]

@sbrannen I'm aware that this is definitely the Achilles' heel of my proposal. I don't have a perfect proposal I'm afraid. In a way, my proposal is (in terms of responsibility) the inverse of Idea 1 : rather than forcing all library authors to move their props file, it asks the application author to somehow mark/tag or otherwise identify their own file.

I guess if you detect a props file on a filesystem classpath you can safely assume that this must the one, right? Is it realistic to have multiples of those?

If it's packaged into a JAR it becomes impossible for JUnit to reliably detect my own file without any hints I think. Can I be asked to add "something" (a comment?) to my file to identify it as mine?

[marcphilipp]

In a multi-project setup, it's quite common for it to be in a JAR when shared across projects.

rather than forcing all library authors to move their props file

How many are we talking about? I'm only aware of Quarkus shipping a properties file.

[github-actions]

If you would like us to be able to process this issue, please provide the requested information. If the information is not provided within the next 3 weeks, we will be unable to proceed and this issue will be closed.

[cdprete]

In a multi-project setup, it's quite common for it to be in a JAR when shared across projects.

rather than forcing all library authors to move their props file

How many are we talking about? I'm only aware of Quarkus shipping a properties file.

Kafka seems to be shipping with such a file in its jars as well.

loader = jdk.internal.loader.ClassLoaders$AppClassLoader@76ed5528
loader.parent = jdk.internal.loader.ClassLoaders$PlatformClassLoader@5870fc05
loader.parent.parent = null
single: jar:file:/.../.m2/repository/org/apache/kafka/kafka-clients/3.7.1/kafka-clients-3.7.1-test.jar!/junit-platform.properties
multi: jar:file:/.../repository/org/apache/kafka/kafka-clients/3.7.1/kafka-clients-3.7.1-test.jar!/junit-platform.properties
multi: jar:file:/.../.m2/repository/org/apache/kafka/kafka-server-common/3.7.1/kafka-server-common-3.7.1-test.jar!/junit-platform.properties
distinct: jar:file:/.../.m2/repository/org/apache/kafka/kafka-clients/3.7.1/kafka-clients-3.7.1-test.jar!/junit-platform.properties
distinct: jar:file:/.../.m2/repository/org/apache/kafka/kafka-server-common/3.7.1/kafka-server-common-3.7.1-test.jar!/junit-platform.properties

[sormuras]

Kafka seems to be shipping with such a file a its jars as well.

But why? For the following?

https://github.com/apache/kafka/blob/trunk/clients/src/test/resources/junit-platform.properties

junit.jupiter.params.displayname.default = "{displayName}.{argumentsWithNames}"

Similar/same line in: https://github.com/apache/kafka/blob/trunk/server-common/src/test/resources/junit-platform.properties

[marcphilipp]

Those don't look like they're meant for consumption in downstream projects, do they?

[cdprete]

Kafka seems to be shipping with such a file a its jars as well.

But why? For the following?

https://github.com/apache/kafka/blob/trunk/clients/src/test/resources/junit-platform.properties

junit.jupiter.params.displayname.default = "{displayName}.{argumentsWithNames}"

Similar/same line in: https://github.com/apache/kafka/blob/trunk/server-common/src/test/resources/junit-platform.properties

I can't say why, honestly :D

It looks quite useless to me, to be honest, but it's there.

Idea 5: Strip out the file manually

I don't really like it (I would prefer some sort of merging done automatically) but, for Maven builds, https://www.mojohaus.org/truezip/truezip-maven-plugin/ could be used to strip out the file from the jars.
Of course, we've to do such a thing by ourselves and it can't be done by JUnit itself.

[cdprete]

Those don't look like they're meant for consumption in downstream projects, do they?

I agree.
In the meantime, I've opened a ticket on their issue tracker: https://issues.apache.org/jira/browse/KAFKA-17307 with https://github.com/cdprete/kafka-junit-platform-props as minimal example.

[cdprete]

Collect all junit-platform.properties from the classpath but give precedence to my own.

@marcelstoer, how do you propose that JUnit should detect which one is "your own"?

Please keep in mind that developers may choose to package their own properties file in a JAR and that the structure of the classpath (filesystem directories, JARs, exploded JARs, etc). may be different in the build vs. in the IDE, etc.

@sbrannen A naïve idea would be to assign the priority based on the path where the files have been found.
The further they're away from the current working (or, better, sub-module in case of multi-module projects) directory, the less important they're.
In such a scenario, "your own" file will be the one having the shortest path.

Moreover, configs within JARs (aka from some sort of direct or transitive dependency) could have less priority by default.

[mumrah]

Hello from Apache Kafka 👋 😄. I came across this thread while trying to figure out why we were seeing this warning in our build. I agree we should not be shipping this file in our test jars (I upgraded the priority of KAFKA-17307).

What we were attempting to do was set a default JUnit property for all of our tests. What is the recommended approach here? We have a multi-project Gradle build with a fairly complex tree of dependencies. Perhaps we need to introduce a testRuntimeOnly project dependency that includes this file as a resource?

[marcphilipp]

@mumrah Yes, that sounds like a sensible solution. Another would be to exclude junit-platform.properties when creating the test jar.

[cdprete]

Hello from Apache Kafka 👋 😄. I came across this thread while trying to figure out why we were seeing this warning in our build. I agree we should not be shipping this file in our test jars (I upgraded the priority of KAFKA-17307).

What we were attempting to do was set a default JUnit property for all of our tests. What is the recommended approach here? We have a multi-project Gradle build with a fairly complex tree of dependencies. Perhaps we need to introduce a testRuntimeOnly project dependency that includes this file as a resource?

@mumrah I would probably simply adapt the Gradle task responsible of creating the test-jar to not include such a file as @marcphilipp said.
This should be even safer for Maven projects since there is no test-runtime scope in Maven.

Another option would maybe be to configure the Gradle test runner to pass these properties as system properties, so that the file is not needed at all.

[mumrah]

Turns out, this was solved a little while ago in Kafka (KAFKA-17121) by explicitly excluding the file from the JARs. In the upcoming 3.9 release, this should no longer be a problem.