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

Return subcommand execution result in ConsoleLauncher #3322

Merged

Conversation

rdesgroppes
Copy link
Contributor

@rdesgroppes rdesgroppes commented May 24, 2023

Overview

When explicitly specifying the execute subcommand as advised by:

WARNING: Delegated to the 'execute' command.
This behaviour has been deprecated and will be removed in a future release.
Please use the 'execute' command directly.

... it happens that ConsoleLauncher.run returns a CommandResult whose getValue method always returns Optional.empty() instead of the expected (non-empty) Optional<TestExecutionSummary>.

It turns out that, when picocli executes a subcommand, it doesn't propagate the return value to the parent CommandLine, whose getExecutionResult method then returns null.

There was a question about it last year (remkop/picocli#1656) answered by the user manual:

The ParseResult can be used to get the return value from a Callable or Method subcommand:

// getting return value from Callable or Method command
Object topResult = cmd.getExecutionResult();

// getting return value from Callable or Method subcommand
ParseResult parseResult = cmd.getParseResult();
if (parseResult.subcommand() != null) {
   CommandLine sub = parseResult.subcommand().commandSpec().commandLine();
   Object subResult = sub.getExecutionResult();
}

The present change therefore consists in implementing it.

Note: prior to the change, presently adapted tests (now parameterized so as to cover both forms) were all failing on:

java.lang.IllegalStateException: TestExecutionSummary not assigned. Exit code is: 0
	at app//org.junit.platform.console.ConsoleLauncherWrapperResult.checkTestExecutionSummaryState(ConsoleLauncherWrapperResult.java:42)
	at app//org.junit.platform.console.ConsoleLauncherWrapperResult.getTestsFoundCount(ConsoleLauncherWrapperResult.java:102)

I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@rdesgroppes rdesgroppes force-pushed the return-subcommand-execution-result branch from 29b6ddd to 7a0a854 Compare May 24, 2023 20:37
When explicitly specifying the `execute` subcommand as advised by:
> WARNING: Delegated to the 'execute' command.
>          This behaviour has been deprecated and will be removed in a future release.
>          Please use the 'execute' command directly.

... it happens that `ConsoleLauncher.run` returns a `CommandResult`
whose `getValue` method always returns `Optional.empty()` instead of the
expected (non-empty) `Optional<TestExecutionSummary>`.

It turns out that, when `picocli` executes a _subcommand_, it doesn't
propagate the return value to the parent `CommandLine`, whose
`getExecutionResult` method then returns `null`.

There was a question about it last year
(remkop/picocli#1656) answered by the [user
manual](https://picocli.info/#_executing_commands_with_subcommands):
> The `ParseResult` can be used to get the return value from a Callable
> or Method subcommand:
> ```java
> // getting return value from Callable or Method command
> Object topResult = cmd.getExecutionResult();
>
> // getting return value from Callable or Method subcommand
> ParseResult parseResult = cmd.getParseResult();
> if (parseResult.subcommand() != null) {
>    CommandLine sub = parseResult.subcommand().commandSpec().commandLine();
>    Object subResult = sub.getExecutionResult();
> }
> ```

The present change therefore consists in implementing it.

Note: prior to the change, presently adapted tests (now parameterized so
as to cover both forms) were all failing on:
```java
java.lang.IllegalStateException: TestExecutionSummary not assigned. Exit code is: 0
	at app//org.junit.platform.console.ConsoleLauncherWrapperResult.checkTestExecutionSummaryState(ConsoleLauncherWrapperResult.java:42)
	at app//org.junit.platform.console.ConsoleLauncherWrapperResult.getTestsFoundCount(ConsoleLauncherWrapperResult.java:102)
```
@rdesgroppes rdesgroppes force-pushed the return-subcommand-execution-result branch from 7a0a854 to e3b0824 Compare May 24, 2023 20:45
@ParameterizedTest
@ValueSource(strings = { "-e junit-jupiter -p org.junit.platform.console.subpackage",
"execute -e junit-jupiter -p org.junit.platform.console.subpackage" })
void executeWithoutExcludeClassnameOptionDoesNotExcludeClassesAndMustIncludeAllClassesMatchingTheStandardClassnamePattern(
Copy link
Member

Choose a reason for hiding this comment

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

Good candidate for longest method name in this project. ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to rotate my screen to read it in full ;)

Copy link
Member

Choose a reason for hiding this comment

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

Good candidate for longest method name in this project. ;-)

Mmmm.... looks like I'm getting some competition! 😈

@marcphilipp marcphilipp added this to the 5.10 RC1 milestone May 25, 2023
@marcphilipp marcphilipp self-assigned this May 27, 2023
@marcphilipp marcphilipp changed the title Return subcommand execution result (instead of null) Return subcommand execution result May 27, 2023
@marcphilipp marcphilipp merged commit e0970b6 into junit-team:main May 27, 2023
@rdesgroppes rdesgroppes deleted the return-subcommand-execution-result branch May 27, 2023 14:04
@rdesgroppes
Copy link
Contributor Author

Danke, Marc!

@sbrannen sbrannen changed the title Return subcommand execution result Return subcommand execution result in ConsoleLauncher May 27, 2023
@marcphilipp
Copy link
Member

@rdesgroppes Thank you! 🙂

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.

4 participants