junit5 icon indicating copy to clipboard operation
junit5 copied to clipboard

Make console launcher --classpath argument parser work like java's when referring to a jar directory

Open mbucc opened this issue 3 years ago • 10 comments

The junit-platform-console-standalone --classpath option requires you to list all the jars in a directory; for example, -cp "testlib/mockito-core-4.1.0.jar:testlib/byte-buddy-1.12.3.jar:testlib/byte-buddy-agent-1.12.3.jar:testlib/objenesis-3.2.jar:classes.

This is different than Java's --classpath, which supports the more compact -cp "testlib/*. Coming from java and javac, this was unexpected behavior from junit. As far as I can tell, the difference is not documented.

Deliverables

  • [ ] Update documentation to specify syntax of PATH (section 4.3.1 of User Guide)
  • [x] #2789
  • [ ] Update -cp processing to support the same syntax as javac and java.

mbucc avatar Dec 08 '21 13:12 mbucc

It's not a goal of the junit-platform-console launcher to be on par with options available for java and javac.

You may always resort to set up a custom java command, using all its options and pass-in desired arguments (including junit-platform-console-standalone's JAR on the classpath) and then call the main entry-point via: org.junit.platform.console.ConsoleLauncher

java -cp classes:testlib/* org.junit.platform.console.ConsoleLauncher --scan-classpath

@junit-team Shall we update the user-guide with a better explanation and/or an example as shown above?

Having said that, I like the idea of showing information about installed/available/observable test engines and their properties, including their engine IDs. Let's track this in a dedicated issue.

sormuras avatar Dec 08 '21 16:12 sormuras

@junit-team Shall we update the user-guide with a better explanation and/or an example as shown above?

Yes, I think we should document that advanced option.

sbrannen avatar Dec 08 '21 16:12 sbrannen

@sbrannen How to tackle checkbox two above? Update the Console Launcher --help output?

I got as far as finding the gradle build script that (I think) loads a .txt file that contains the launcher option documentation. But I could not find that text file in the repo or any info on how it is created, so I'm guessing your build process writes the help output to this file.

mbucc avatar Dec 09 '21 12:12 mbucc

@sbrannen Never mind, I got it: https://github.com/junit-team/junit5/blob/5a20a2ad86365e06c48a68e2542b7d901e45042f/documentation/documentation.gradle.kts#L172-L176

mbucc avatar Dec 09 '21 12:12 mbucc

Team decision: We're not opposed to aligning the behavior with the JDK as long as it doesn't add considerable complexity to the codebase. Thus, we should first do some research whether the functionality from the JDK can be reused.

@mbucc Are you willing to look into this some more?

marcphilipp avatar Dec 10 '21 12:12 marcphilipp

@marcphilipp Sure.

mbucc avatar Dec 10 '21 12:12 mbucc

Here's my suggested approach. Let me know if this is OK and I'll start work a pull request.

Current code:

  • ConsoleLauncher.java is the entry point
  • CommandLineOptionsParser is a service used in the launcher
  • PicocliCommandLineOptionsParser is the provider of this service
  • CommandLineOptions is (kind of) a value type.

"kind of" because it is all getter/setter except for getExplicitSelectors() and getExistingAdditionalClasspathEntries():

141		public List<Path> getExistingAdditionalClasspathEntries() {
142			return this.additionalClasspathEntries.stream().filter(Files::exists).collect(toList());
143		}

My suggestion is to refactor the logic on line 142 into a new service whose job is to expand the classpath and then filter it.

So:

  1. new attribute to CommandLineOptions: List<Path> existingAdditonalClasspathEntries;
  2. change getExistingAdditionalClasspathEntries() to return this attribute
  3. new service: org.junit.platform.console.options.ClasspathExpander
  4. with one method:List<Path> expandClasspath(List<Path> paths);
  5. new provider: org.junit.platform.console.options.ClasspathExpanderProvider that expands and filters.
  6. update ConsoleLauncher.java to use this pair, making it look like the existing service/provider code. Add one line that calls CommandLineOptions setter for existingAdditonalClasspathEntries.

I considered putting this logic into the existing parser service. I didn't suggest that because the two services will change for different reasons. Arg parsing logic will change if you add/delete options. Path expansion logic will change (rarely!) to stay in sync with how java jar classpath expansion works.

I also thought about defining a new value type for the Expanded version of the command-line options. That is more disruptive to existing code, which I want to leave as-is as much as possible.

Finally, I also considered making the CommandLineOptions getExistingAdditionalClasspathEntries() return a copy of the list (immutability), but that could break existing code. And the class has a public setter. Step away from the keyboard! :)

mbucc avatar Dec 11 '21 13:12 mbucc

@mbucc Thanks for the analysis! I agree that expanding the entries should be done by a different class. I wonder if additionalClasspathEntries shouldn't be a List<String> now that we start accepting wildcard patterns.

Regarding, the implementation of the "expansion" logic: have you found anything in the JDK that we can reuse for that?

marcphilipp avatar Dec 18 '21 14:12 marcphilipp

have you found anything in the JDK that we can reuse

I will look.

mbucc avatar Dec 18 '21 22:12 mbucc

@marcphilipp No luck. Quoting https://stackoverflow.com/a/6577660,

Classpath wildcard expansion is not performed by a standard library in Java, and the expansion is performed even before any classes are loaded by the JVM. ... you'll need to roll your own implementation if you intend to do this in Java. You can look up the implementation in C (for the OpenJDK runtime) in the wildcard.c file located in the jdk/src/share/bin directory of the sources.

https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/wildcard.c

<pendantry> As best I can figure, the license on that C code is GPL v2. My understanding is that a translation (e.g., C to Java) is a derived work. Based on this text from https://github.com/openjdk/jdk/blob/master/ADDITIONAL_LICENSE_INFO:

However, note that this would not permit you to commingle code under an incompatible license with Oracle's GPLv2 licensed code by, for example, cutting and pasting such code into a file also containing Oracle's GPLv2 licensed code and then distributing the result.

you cannot mix in a Java translation of GPL'd C code with JUnit's Eclipse license. </pendantry>

I'll take a crack at a patch that is a clean re-write.

mbucc avatar Dec 29 '21 17:12 mbucc

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.

stale[bot] avatar Apr 11 '23 20:04 stale[bot]

Turns out the comment on Dec 8, 2021 from @sormuras was enough to scratch this itch for me. I'll save the stale-bot some work, and close this now.

mbucc avatar Apr 17 '23 11:04 mbucc