junit5 icon indicating copy to clipboard operation
junit5 copied to clipboard

Support multiple `junit-platform.properties` on the classpath

Open famod opened this issue 3 years ago • 34 comments
trafficstars

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 avatar Dec 12 '21 21:12 famod

(I think the bot has mistakenly picked up the IntelliJ IDEA label because I merely used the word "idea".)

famod avatar Dec 12 '21 22:12 famod

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

stale[bot] avatar Dec 13 '22 12:12 stale[bot]

Unstale

stefanrybacki avatar Dec 13 '22 20:12 stefanrybacki

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

famod avatar Jan 30 '23 23:01 famod

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?

mpkorstanje avatar Jan 31 '23 00:01 mpkorstanje

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.

famod avatar Jan 31 '23 08:01 famod

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.

marcelstoer avatar Jan 03 '24 07:01 marcelstoer

@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 avatar Apr 19 '24 14:04 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.

marcphilipp avatar Apr 19 '24 14:04 marcphilipp

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.

dhoard avatar Apr 19 '24 15:04 dhoard

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.

sormuras avatar Apr 19 '24 15:04 sormuras

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.

sbrannen avatar Apr 19 '24 15:04 sbrannen

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.

mpkorstanje avatar Apr 19 '24 17:04 mpkorstanje

@sbrannen @mpkorstanje thanks for the clarification.

dhoard avatar Apr 19 '24 18:04 dhoard

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

mpkorstanje avatar Apr 19 '24 19:04 mpkorstanje

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.

marcelstoer avatar Apr 19 '24 20:04 marcelstoer

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 avatar Apr 20 '24 12:04 sbrannen

@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?

marcelstoer avatar Apr 22 '24 09:04 marcelstoer

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.

marcphilipp avatar Apr 22 '24 17:04 marcphilipp

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.

github-actions[bot] avatar May 07 '24 01:05 github-actions[bot]

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

cdprete avatar Aug 09 '24 09:08 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

sormuras avatar Aug 09 '24 09:08 sormuras

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

marcphilipp avatar Aug 09 '24 09:08 marcphilipp

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 avatar Aug 09 '24 09:08 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 avatar Aug 09 '24 15:08 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.

cdprete avatar Aug 09 '24 15:08 cdprete