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

Rename new ParameterizedTest attributes to avoid confusion #4137

Closed
1 task
marcphilipp opened this issue Nov 19, 2024 · 7 comments · Fixed by #4149
Closed
1 task

Rename new ParameterizedTest attributes to avoid confusion #4137

marcphilipp opened this issue Nov 19, 2024 · 7 comments · Fixed by #4149

Comments

@marcphilipp
Copy link
Member

With #3708 (PR #4045) adding argumentCountValidation to @ParameterizedTest, I feel like there is a bit lack of clarity for users about what requireArguments does compared to argumentCountValidation. Specifically, I think it's not very clear which of these applies to arguments within an argument set and which applies to argument sets, if that is even the right wording.

@marcphilipp since you worked on both PRs, what's your opinion on this?
Maybe, it'd make sense to rename requireArguments to requireArgumentSets and improve the docs? Or it's not that big of a deal. I'm not sure.

For context these are the two fields in @ParameterizedTest:

/**
* Configure whether at least one set of arguments is required for this
* parameterized test.
*
* <p>Set this attribute to {@code false} if the absence of arguments is
* expected in some cases and should not cause a test failure.
*
* <p>Defaults to {@code true}.
*
* @since 5.12
*/
@API(status = EXPERIMENTAL, since = "5.12")
boolean requireArguments() default true;
/**
* Configure how the number of arguments provided by an {@link ArgumentsSource} are validated.
*
* <p>Defaults to {@link ArgumentCountValidationMode#DEFAULT}.
*
* <p>When an {@link ArgumentsSource} provides more arguments than declared by the test method,
* there might be a bug in the test method or the {@link ArgumentsSource}.
* By default, the additional arguments are ignored.
* {@code argumentCountValidation} allows you to control how additional arguments are handled.
* The default can be configured via the {@value ArgumentCountValidator#ARGUMENT_COUNT_VALIDATION_KEY}
* configuration parameter (see the User Guide for details on configuration parameters).
*
* @since 5.12
* @see ArgumentCountValidationMode
*/
@API(status = EXPERIMENTAL, since = "5.12")
ArgumentCountValidationMode argumentCountValidation() default ArgumentCountValidationMode.DEFAULT;

Originally posted by @JonasJebing in #1477

Deliverables

  • Rename one or both attributes to make their effect clearer
@marcphilipp
Copy link
Member Author

marcphilipp commented Nov 19, 2024

Idea: keep argumentCountValidation and rename requireArguments to requireInvocations or requireAtLeastOneInvocation

@JonasJebing
Copy link
Contributor

JonasJebing commented Nov 19, 2024

I like requireInvocations and requireAtLeastOneInvocation better than my requireArgumentSets idea 👍
Maybe I have a slight preference for the more explicit requireAtLeastOneInvocation

@marcphilipp
Copy link
Member Author

Team decision: Rename ParameterizedTest.requireArguments to allowZeroInvocations (and change default to true)

@Anmavel
Copy link
Contributor

Anmavel commented Nov 23, 2024

Hi, can i work on this issue? :)

@juliette-derancourt
Copy link
Member

Hey @Anmavel, I just assigned the issue to you! :)

@sormuras
Copy link
Member

Have fun hacking @Anmavel !

@JonasJebing
Copy link
Contributor

JonasJebing commented Nov 23, 2024

Team decision: Rename ParameterizedTest.requireArguments to allowZeroInvocations (and change default to true)

@marcphilipp I think there's a typo. Shouldn't it be "change default to false"?

Edit: I just saw the PR, where we're changing the default to false. So, all good from that side IMO 👍

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