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

Pass command line arguments via file to ConsoleLauncher #1498

Closed
yyoncho opened this issue Jul 17, 2018 · 23 comments
Closed

Pass command line arguments via file to ConsoleLauncher #1498

yyoncho opened this issue Jul 17, 2018 · 23 comments

Comments

@yyoncho
Copy link

yyoncho commented Jul 17, 2018

There is a command line argument limit on OS-es like Windows(8081). So it would be good if it is possible to pass command line arguments to junit-platform-console-standalone via file since the classpath might be very big.

@sormuras
Copy link
Member

The implementation c-/should follow most of the features supported javas @file option: https://docs.oracle.com/javase/10/tools/java.htm#JSWOR-GUID-4856361B-8BFD-4964-AE84-121F5F6CF111

@marcphilipp
Copy link
Member

@yyoncho Have you tried using the CLASSPATH environment variable instead?

@yyoncho
Copy link
Author

yyoncho commented Jul 26, 2018

@marcphilipp if you try to set env variable you will hit the command line limit again ...

@marcphilipp
Copy link
Member

@sormuras Is this @file thing new?

@yyoncho Given @file options, can you already use that like in the following example?

java -cp @my-classpath.txt junit-platform-console-standalone.jar ...

@yyoncho
Copy link
Author

yyoncho commented Jul 28, 2018

@marcphilipp yes, if this is the equivalent to passing the classpath as "--cp" console platform runner param.

@marcphilipp
Copy link
Member

@yyoncho Yes, it should be. Could you please give that a try?

@marcphilipp marcphilipp modified the milestones: 5.x Backlog, General Backlog Jul 29, 2018
@sbrannen sbrannen changed the title Feature Request: pass command line arguments via file Feature Request: pass command line arguments via file to console launcher Jul 29, 2018
@remkop
Copy link
Contributor

remkop commented Aug 1, 2018

If there is interest, I can provide a pull request to replace the current jopt-simple command line parser with picocli. That will give these benefits:

  • picocli has built-in support for @-files - this feature request
  • usage help with ANSI colors
  • TAB autocompletion on bash and zsh
  • less code: DetailsConverter, ThemeConverter and UriConverter can be removed (enums and URI conversion are supported out of the box), KeyValuePairConverter can likely be removed (picocli supports Maps out of the box); the builder-style code in AvailableOptions can be replaced with field annotations

In terms of track record, Groovy uses picocli for its standalone tools and recently replaced Commons CLI with picocli in their CliBuilder DSL implementation (see https://blogs.apache.org/logging/entry/groovy-2-5-clibuilder-renewal).

Please let me know if there would be interest in a PR.

@sbrannen
Copy link
Member

sbrannen commented Aug 1, 2018

@remkop, thanks for the offer!

@yyoncho Given @file options, can you already use that like in the following example?

java -cp @my-classpath.txt junit-platform-console-standalone.jar ...

@marcphilipp, I think the request is for @file support for the Console Launcher itself (i.e., not the java executable):

--class-path, --classpath, --cp <Path:         Provide additional classpath entries --
  path1:path2:...>                               for example, for adding engines and
                                                 their dependencies. This option can be
                                                 repeated.

So, if jOpt Simple doesn't support that, perhaps we should consider the proposal from @remkop.

@junit-team/junit-lambda, thoughts?

@sbrannen sbrannen modified the milestones: General Backlog, 5.3 RC1 Aug 1, 2018
@sbrannen
Copy link
Member

sbrannen commented Aug 1, 2018

Tentatively slated for 5.3 RC1 though currently only for the sake of team discussion.

@remkop
Copy link
Contributor

remkop commented Aug 1, 2018

Out of interest, what is the timeline for 5.3 RC1?

@yyoncho
Copy link
Author

yyoncho commented Aug 1, 2018

@marcphilipp @remkop I tried it with java 8 and it failed to find main class. Just for the record, you cannot use -jar + classpath. Here it is what I tried to use.

java -cp @classpath.txt  org.junit.platform.console.ConsoleLauncher -m xxx.QueryMatcherTest#testString

@marcphilipp
Copy link
Member

I tried it with java 8 and it failed to find main class.

@yyoncho What was in classpath.txt?

@marcphilipp, I think the request is for @file support for the Console Launcher itself (i.e., not the java executable)

@sbrannen I know, I was just trying to come up with a workaround. 😉

If there is interest, I can provide a pull request to replace the current jopt-simple command line parser with picocli.

@remkop picocli does look very nice! We're shading/relocating jopt-simple so JUnit users can use a different version of it in their own applications. Would that also be possible with picocli?

@remkop
Copy link
Contributor

remkop commented Aug 1, 2018

@marcphilipp Yes, Groovy and Micronaut are shading picocli in their distributions so it is certainly possible.

@marcphilipp
Copy link
Member

@remkop Cool, in that case I don't see a reason why we shouldn't explore this option. So, feel free to submit a PR, if you feel up for it. 🙂

@remkop
Copy link
Contributor

remkop commented Aug 3, 2018

Question about the expected behaviour:

In JOptSimple options are defined without specifying hyphens: parser.acceptsAll(asList("o", "select-module"), .... It appears that JOptSimple accepts any number of leading dashes when recognizing options. The unit tests expect that any option is recognized with one dash as well as with two dashes:

-o com.acme.foo
--o com.acme.foo
-select-module com.acme.foo
--select-module com.acme.foo

This surprised me a bit, since the JOptSimple usage help only shows the "canonical" number of dashes:

-o, --select-module <String: module name>      EXPERIMENTAL: Select single module for
                                                 test discovery. This option can be
                                                 repeated.

In picocli, dashes are explicit when options are defined: @Option(names = {"-o", "--select-module"}, .... In my first cut of a picocli-based ConsoleLauncher, I followed the current usage help message.

However, this doesn’t recognize --o or -select-module, and some existing unit tests fail because of this.

In this version, usage help now looks like this: (https://pasteboard.co/HxwGmsa.png shows the ANSI colors)

  -o, --select-module=NAME   EXPERIMENTAL: Select single module for test discovery. This
                               option can be repeated.

It is possible to simply define all options with both a single dash and with two dashes: @Option(names = {"-o", "--o", "-select-module", "--select-module"}, ..., but these would all show in the usage help:

  -o, --o, -select-module, --select-module=NAME
                             EXPERIMENTAL: Select single module for test discovery. This
                               option can be repeated.

Do you prefer to recognize all options with both a single dash and with two dashes (with a longer usage help message), or is it acceptable to reject user input that does not match the format in the current usage help message?

@remkop
Copy link
Contributor

remkop commented Aug 4, 2018

I thought of a way to keep the current lenient parser behavior and still display the current usage help message. Picocli allows hidden options that are not shown in the usage help.

// shown in usage help
@Option(names = {"-o", "--select-module"}, ...)
List<String> selectModules;

// not shown 
@Option(names = {"--o", "-select-module"}, hidden = true)
List<String> additionalSelectModules;

The values can be merged.

I’ll proceed with this for now. Let me know if you prefer one of the other possibilities I mentioned in my previous comment.

@sbrannen
Copy link
Member

sbrannen commented Aug 4, 2018

Since we want the usage to be backward compatible, let's just go with the "hidden options" approach.

remkop added a commit to remkop/junit5 that referenced this issue Aug 9, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 9, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 9, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 9, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 9, 2018
@remkop
Copy link
Contributor

remkop commented Aug 9, 2018

PR #1530 for this ticket now includes documentation and passes all tests. Kindly review at your convenience.

remkop added a commit to remkop/junit5 that referenced this issue Aug 9, 2018
@remkop
Copy link
Contributor

remkop commented Aug 11, 2018

It would be great if this could be included in 5.3 RC1. Please let me know if there’s anything I can do to facilitate that.

@marcphilipp
Copy link
Member

Fully agreed! I'll review #1530 today.

@marcphilipp marcphilipp modified the milestones: 5.4 Backlog, 5.3 RC1 Aug 11, 2018
@marcphilipp marcphilipp self-assigned this Aug 11, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
* remove arity = "0..1" from disable-ansi-colors options: no need to accept a "true|false" parameter
* remove single-dash -fail-if-no-tests option: recently introduced so no worries about breaking existing apps
* option names attribute for --config option does not need to be an array
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
…colors no longer accepts a "true|false" parameter
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
… message and usage help instead of stacktrace
remkop added a commit to remkop/junit5 that referenced this issue Aug 12, 2018
@ghost ghost removed the status: in progress label Aug 12, 2018
@sbrannen sbrannen changed the title Feature Request: pass command line arguments via file to console launcher Pass command line arguments via file to ConsoleLauncher Aug 12, 2018
sbrannen added a commit that referenced this issue Aug 12, 2018
dotCipher pushed a commit to dotCipher/junit5 that referenced this issue Sep 18, 2018
This commit introduces support for @-files (parameter files) by using
the picocli command line parser instead of jopt-simple. This also gives
ANSI colored usage help on supported platforms.

Classes `DetailsConverter`, `ThemeConverter`, `UriConverter`, and 
`KeyValuePairConverter` were removed since they are not longer needed.

Resolves junit-team#1498.
dotCipher pushed a commit to dotCipher/junit5 that referenced this issue Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants