Skip to content

Include parameter names in default name of @ParameterizedTest #2040

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

Closed
vlsi opened this issue Oct 4, 2019 · 12 comments · Fixed by #2108
Closed

Include parameter names in default name of @ParameterizedTest #2040

vlsi opened this issue Oct 4, 2019 · 12 comments · Fixed by #2108

Comments

@vlsi
Copy link
Contributor

vlsi commented Oct 4, 2019

Suppose I have a test:

    @ParameterizedTest
    @MethodSource("implementationsAndIps")
    public void test(String httpImplementation, String targetHost, InetAddress sourceIp,
                     WireMockServer server, JMeterVariables vars) throws SocketException {

The result looks unreadable:

[3] HttpClient4, 127.0.0.1, /127.0.0.1
[1] HttpClient4, 127.0.0.1, /fe80:0:0:0:0:0:0:1%lo0
[7] Java, 127.0.0.1, /fe80:0:0:0:0:0:0:1%lo0
[4] HttpClient4, [::1], /fe80:0:0:0:0:0:0:1%lo0
[5] HttpClient4, [::1], /0:0:0:0:0:0:0:1%lo0
[8] Java, 127.0.0.1, /0:0:0:0:0:0:0:1%lo0
[10] Java, [::1], /fe80:0:0:0:0:0:0:1%lo0

It does not really help if the test fails.
That means every time I use @ParameterizedTest I have to declare test name explicitly.
That is sad.

What if JUnit generated something like

[3] httpImplementation=HttpClient4, targetHost=127.0.0.1, sourceIp=/127.0.0.1
[1] httpImplementation=HttpClient4, targetHost=127.0.0.1, sourceIp=/fe80:0:0:0:0:0:0:1%lo0
...

?

It would make lots of tests and test failures easier to understand.
I'm OK if it is {arguments_with_names}. Then I could create @ParameterizedTestWithArgNames annotation that uses detailed name.

PS. I've seen #1154, however, I don't really want to deal and/or declare resolvers.

@sbrannen sbrannen changed the title Print parameter names in default name of @ParameterizedTest Include parameter names in default name of @ParameterizedTest Oct 4, 2019
@sbrannen sbrannen added this to the 5.6 M1 milestone Oct 4, 2019
@sbrannen
Copy link
Member

sbrannen commented Oct 4, 2019

Tentatively slated for 5.6 M1 solely for the purpose of team discussion and triaging.

@sbrannen
Copy link
Member

sbrannen commented Oct 4, 2019

FYI: this would need to be an opt-in feature. In addition, parameter names are not always present in the compiled byte code. To include them in the byte code, the test code must be compiled with the -parameters compiler flag.

@vlsi
Copy link
Contributor Author

vlsi commented Oct 4, 2019

You assume I use Java, however, Kotlin does retain parameter names always 😉

@marcphilipp
Copy link
Member

@sbrannen Could we check whether parameter names are present and include them only if so?

@sbrannen
Copy link
Member

sbrannen commented Oct 5, 2019

@sbrannen Could we check whether parameter names are present and include them only if so?

Yes. See java.lang.reflect.Parameter.isNamePresent().

@marcphilipp marcphilipp modified the milestones: 5.6 M1, 5.6 M2 Oct 10, 2019
@marcphilipp
Copy link
Member

Team Decision: Introduce new {arguments_with_names} pattern and make it the default. Only include parameter names if present in bytecode and parameter types are known (e.g. not for ArgumentsAccessor and ArgumentsAggregator).

@marcphilipp
Copy link
Member

@vlsi Would you be interested in submitting a PR for this?

@juliette-derancourt
Copy link
Member

FYI, just started working on it.

@juliette-derancourt
Copy link
Member

Only include parameter names if present in bytecode and parameter types are known (e.g. not for ArgumentsAccessor and ArgumentsAggregator).

@junit-team/junit-lambda If one of these conditions are encountered for one of the parameters, do we fallback on the current behavior for all of them or do we still add names for the other parameters? (I'd rather fallback on the {arguments} pattern)

@marcphilipp
Copy link
Member

I think we could support the case of mixed position and accessor/aggregator arguments, i.e. for test like the following:

@ParameterizedTest
@ValueSource(ints = { 1, 2, 3 })
void testWithValueSource(int value, ArgumentsAccessor accessor) {
	assertTrue(value > 0 && value < 4);
}

The display names currently are [1] 1 etc. so we could make them [1] value=1 etc.

We should be able to extract the logic in ParameterizedTestParameterResolver, i.e. only add the parameter name if the parameter index is < methodContext.indexOfFirstAggregator() (if methodContext.hasAggregator()).

@juliette-derancourt
Copy link
Member

I'm wondering if we should call the pattern {arguments_with_names} or {argumentsWithNames}?
I just noticed that the existing {displayName} pattern is in camel case...

@sbrannen
Copy link
Member

Indeed. Better to stay consistent and go with camel case there. Good catch.

juliette-derancourt added a commit that referenced this issue Dec 1, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Introduced a new `{arguments_with_names}` pattern in `@ParameterizedTest`, which is now used in the default name pattern. The parameter names are included only if they are present in the bytecode and if the parameter types are known (`ArgumentsAccessor` and `ArgumentsAggregator` are therefore ignored).

Closes #2040.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment