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

@ParameterizedTest ignores additional arguments beyond formal parameter list #3708

Open
milliken99 opened this issue Feb 26, 2024 · 9 comments

Comments

@milliken99
Copy link

milliken99 commented Feb 26, 2024

junit-jupiter-params-5.9.2

Steps to reproduce

    private static Stream<Arguments> fooData() {
        return Stream.of(
            Arguments.of("1",
            Arguments.of(""))
        );
    }

    @ParameterizedTest
    @MethodSource("fooData")
    void testFoo(final String fooString) {
        assertFalse(fooString.isEmpty());
    }

I would expect a runtime error as the stream of arguments return in fooData() aren't compatible with testFoo(String s). In this instance, only the first argument (i.e. "1") is fed to testFoo(), all other arguments are unceremoniously dropped on the floor. Note that the second argument (i.e., "") is not tested and would have led to a unit test failure.

Context

  • Used versions (Jupiter/Vintage/Platform): 5.9.2
  • Build Tool/IDE: IntelliJ
@sbrannen
Copy link
Member

sbrannen commented Feb 27, 2024

Hi @milliken99,

Congratulations on opening your first issue on GitHub! 👍

Regarding your test setup, you have incorrectly created your stream of arguments.

With the following, the 2nd invocation fails as expected.

return Stream.of(Arguments.of("1"), Arguments.of(""));

In light of that, I am closing this issue.

@milliken99
Copy link
Author

Hi Sam, thanks for your comment. The incorrect stream of arguments was intentional. Since there are no suitable arguments for the indicated test method, I expected an error message. Instead I receive output that all tests pass. This is at best misleading.

@sbrannen
Copy link
Member

The incorrect stream of arguments was intentional.

Oh. Then I wouldn't use Arguments.of() for the second argument to convey your point, since nested Arguments are not supported.

Thus, if I understand you correctly, the following better illustrates your use case.

	private static Stream<Arguments> fooData() {
		return Stream.of(Arguments.of("1", ""));
	}

	@ParameterizedTest
	@MethodSource("fooData")
	void testFoo(String fooString) {
		assertFalse(fooString.isEmpty());
	}

That still ignores the "" argument, but that's by design.

Your testFoo() method only accepts a single argument, and JUnit therefore only provides a single argument, namely the first argument available from each Arguments instance in the stream.

For example, if you keep the above revised fooData() method and modify testFoo() to accept two String arguments as follows...

	@ParameterizedTest
	@MethodSource("fooData")
	void testFoo(String str1, String str2) {
		assertFalse(str1.isEmpty(), "str1");
		assertFalse(str2.isEmpty(), "str2");
	}

That will then fail as follows.

org.opentest4j.AssertionFailedError: str2 ==> expected: <false> but was: <true>

In general, JUnit Jupiter's @ParameterizedTest infrastructure invokes a parameterized test method with as many arguments as can be consumed by the declared formal parameters of the method. This is in order to support use cases where the author of the test may only be interested in the first n elements of the argument set -- for example, when a test is parameterized by CSV data.

The code which implements this logic can be viewed here:

private Object[] consumedArguments(Object[] arguments, ParameterizedTestMethodContext methodContext) {
if (methodContext.hasAggregator()) {
return arguments;
}
int parameterCount = methodContext.getParameterCount();
return arguments.length > parameterCount ? Arrays.copyOf(arguments, parameterCount) : arguments;
}


Since there are no suitable arguments for the indicated test method, I expected an error message. Instead I receive output that all tests pass. This is at best misleading.

Unfortunately, we cannot throw an exception in such cases since it's an existing feature that people depend on.

Though I suppose it would be possible to introduce a flag (boolean attribute) in @ParameterizedTest to fail the test if the method accepts fewer arguments than the number actually supplied.

Is that a feature that you would be interested in?

@sbrannen sbrannen reopened this Feb 28, 2024
@sbrannen sbrannen changed the title ParameterizedTest argument mismatch drops test cases @ParameterizedTest ignores additional arguments beyond formal parameter list Feb 28, 2024
@milliken99
Copy link
Author

I think we would be interested in this feature. Call it strictParameterListSize or something similar, defaulting it to false (or whatever gives the current behavior depending on the name).

If we go this route, would it also be possible to set this globally in junit-platform.properties?

@sbrannen
Copy link
Member

sbrannen commented Mar 9, 2024

I think we would be interested in this feature.

OK.

Call it strictParameterListSize or something similar,

As we all know, "naming is hard" in computer science. 😉

defaulting it to false (or whatever gives the current behavior depending on the name).

If we go this route, would it also be possible to set this globally in junit-platform.properties?

If we want users to be able to change the default globally, we will have to introduce an enum (3 states: default/global, true, false) instead of using a simple boolean flag (2 states: true, false), but that's certainly an option.

We'll discuss the topic it within the team during an upcoming team call.

@marcphilipp
Copy link
Member

Team decision: Add argumentCountValidation attribute to @ParameterizedTest that is backed by an ArgumentCountValidationMode enum (DEFAULT, NONE, STRICT) with default of DEFAULT. In addition, add a configuration parameter named junit.jupiter.params.argumentCountValidation with allowed values of NONE and STRICT which, when evaluated and not present, defaults to NONE.

@bottlerocketjonny
Copy link

Hey @marcphilipp @sbrannen could I work on this issue? Cheers

@marcphilipp
Copy link
Member

@bottlerocketjonny Sure, go ahead! I've assigned the issue to you now. Please let us know if you need any help.

@bottlerocketjonny
Copy link

@bottlerocketjonny Sure, go ahead! I've assigned the issue to you now. Please let us know if you need any help.

Thanks! I'll take a look myself over the next week but if I get stuck I'll be in touch 👍🏻

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

4 participants