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

Introduce support for zero invocations in test templates and parameterized tests #1477

Closed
johnchurchill opened this issue Jun 23, 2018 · 64 comments · Fixed by #3890
Closed

Comments

@johnchurchill
Copy link

johnchurchill commented Jun 23, 2018

Overview

Using: JUnit: 5.2.0

Great job on the new ParameterizedTest support in v.5. The replacement of the static, one-per-class Parameters annotation with more flexible MethodSource, etc. has been like a breath of fresh air and allowed me to remove thousands (!!) of lines of supporting code from my system. I'm really loving it.
However, someone decided to disallow zero parameters with this precondition check in ParameterizedTestExtension.

.onClose(() ->
    Preconditions.condition(invocationCount.get() > 0,
    "Configuration error: You must provide at least one argument for this @ParameterizedTest"));

The problem with this is that we have some testing situations where parameterized tests with zero parameters are not exceptional. For example, we run tests against thousands of a certain type of class generically against a database of past production failures, and many of these classes have never experienced a production failure. When the tests are run, we now get failures due the above precondition check. JUnit 4 handled this cleanly: it would simply not run any tests on those classes.

Workaround

I can get around this by adding a null to the method creating the collection of parameters if it is empty, and then return from the beginning of the @ParameterizedTest method code if the passed parameter is null. That lets us continue to run the parameterized tests against all of the classes, but comes with some disadvantages:

  • We must add specific code to the front and back of every parameterized pair just to avoid a failure that doesn't matter to the tests.
  • The handling of the null causes the test count inflation for these "phantom" tests.
  • null is now reserved as a signal for no parameters, rather than something wrong in the parameter creation.

Proposal

If nobody has any strong feelings about disallowing the no-parameter case, can we just have this precondition removed from a future version?

Thanks.

@sbrannen
Copy link
Member

Tentatively slated for 5.3 RC1 for team discussion

@marcphilipp
Copy link
Member

@johnchurchill Could you please provide a few more details about your use case, particularly how you've written your tests? I assume it involves base classes that inherit @ParameterizedTest methods?

@gaganis
Copy link
Contributor

gaganis commented Jun 24, 2018

I have also been trying to envision what you are trying to do and was not able.

My best guess is that you have some sort of ArgumentsProvider that returns no elements, but still I am not sure. If my guess is valid, this behaviour was explicitly added as a solution for #923.

It would be great if you could provide a skeleton test that exhibits your issue.

@johnchurchill
Copy link
Author

johnchurchill commented Jun 24, 2018

Yes, there is a superclass of all tests which does the parameter loading and execution. There are thousands of subclasses that implement all kinds of functionality. Each (non-test) class is the logic behind an interactive web page in the system. Here is a boiled-down version of that superclass:

@ExtendWith(SpringExtension.class)
@SpringBootTest(classes = SpringDevTestServerConfiguration.class)
@TestExecutionListeners(listeners = { DependencyInjectionTestExecutionListener.class })
@ComponentScan(lazyInit = true)
@TestInstance(PER_CLASS)
public abstract class AbstractConfigTestClass implements ApplicationContextAware {

	protected List<SingleConfigTestDescriptor> configTests() {
		String testClassName = this.getClass().getName();
		// remove "Test" on the end to obtain the config class
		String clzName = testClassName.substring(0, testClassName.length() - 4);
		try {
			Class configClz = Class.forName(clzName);
			List<SingleConfigTestDescriptor> params = ConfigToTestParams.getTestParameters(configClz);
			if (params.size() == 0) {
				params.add(null); // need to add null here to avoid junit 5 failing
			}
			return params;
		}
		catch (ClassNotFoundException e1) {
			throw new RuntimeException("Could not find expected class: " + clzName, e1);
		}
	}

	@ParameterizedTest
	@MethodSource("configTests")
	public void runTest(SingleConfigTestDescriptor test) {
		if (test == null) {
			return; // nothing to do; this is the null to avoid the junit 5 failure
		}
		// use the test data to run the test against the class
	}

}

By the time we know that there are no available tests to run, it's too late.. JUnit 5 will throw an exception. There are 6 lines in there whose only purpose is to make it run successfully like it did under JUnit 4. I just started working with JUnit 5 a couple of days ago, so I haven't had the chance to look into using ArgumentsProvider. Would using ArgumentsProvider avoid the failure?

@sbrannen
Copy link
Member

In any case, we should improve the error message.

"Configuration error: You must provide at least one argument for this @ParameterizedTest" --> "Configuration error: You must provide at least one invocation context for this @ParameterizedTest".

The difference being "argument" vs. "invocation context".

And the invocation context can still supply zero "arguments" to the method, though that's not the topic of this issue.

@sbrannen sbrannen changed the title ParameterizedTestExtension disallows acceptable case of zero parameters ParameterizedTestExtension disallows acceptable case of zero arguments Jun 24, 2018
@sbrannen sbrannen changed the title ParameterizedTestExtension disallows acceptable case of zero arguments ParameterizedTestExtension disallows acceptable case of zero invocations Jun 24, 2018
@sbrannen
Copy link
Member

FYI: I reworded the title of this issue accordingly.

@sbrannen
Copy link
Member

Keep in mind that a @TestTemplate method in general cannot currently have zero invocations.

@sbrannen
Copy link
Member

In other words, see org.junit.jupiter.engine.descriptor.TestTemplateTestDescriptor.validateWasAtLeastInvokedOnce(int). 😉

@johnchurchill
Copy link
Author

I took a look at TestTemplateTestDescriptor, and yes the zero invocations would be stopped there too. I tried taking both of those preconditions out (TestTemplateTestDescriptor and ParameterizedTestExtension) and making a simple class with one @Test and one @ParameterizedTest which refers to a @MethodSource which returns an empty list. The result is that 1/1 tests pass (just bar2()), which is the same behavior we had in JUnit 4. There is no side-effect of allowing the @ParameterizedTest to run with no invocations.

public class FooTest {

	@ParameterizedTest
	@MethodSource("data")
	void bar1(String s) {
	}

	@Test
	public void bar2() {
		// pass
	}

	public static List<String> data() {
		// pretend we have no test cases in the QA database
		return new ArrayList<>();
	}
}

@sbrannen
Copy link
Member

There is no side-effect of allowing the @ParameterizedTest to run with no invocations.

Well, that's only partially true.

It's a breaking change in the sense that configuration errors previously caused the parameterized test container to fail; whereas, now such configuration errors would be silently ignored.

Of course, your use case is not a "configuration error". So we have to decide how to best support both scenarios. 😉

I suppose, if we wanted to support zero invocations, we could simply generate a log message informing the user. The tricky part is what log level to choose. Is that DEBUG, INFO, or WARN?

@sbrannen
Copy link
Member

sbrannen commented Jun 25, 2018

The result is that 1/1 tests pass

In such cases, I don't think the parameterized test container should be marked as "successful" (or somehow ignored, whatever was the case when you implemented that PoC).

Rather, I think we should mark the parameterized test container as "skipped" and provide a "reason" why it was skipped.

@junit-team/junit-lambda, thoughts?

@sbrannen
Copy link
Member

As a side note, since I'm the author of the Spring TestContext Framework...

@johnchurchill, out of curiosity, why do you disable all Spring test listeners except DependencyInjectionTestExecutionListener?

That's enabled by default.

Were other listeners somehow causing problems?

@gaganis
Copy link
Contributor

gaganis commented Jun 25, 2018

This case reminds me of the similar case #1298 which was handled by the --fail-if-no-tests cl option.

The similarity is that they both exhibit the same core dilemma:
If I have zero elements to process, is that because the user has done some error or is this lack of items actually valid and intentional? The error could be in configuration as in both cases or programmatic in the case of providing arguments_(invocation contexts)_.

The difference between the two is the default is reversed.

  • In the case of the Console Launcher. In cl the default was not to fail and we added an option to change this behavior.
  • For the parameterized case we fail if zero invocation contexts.

Maybe we could take a similar approach also in the case- with some sort of flag or parameter eg on the annotation.

For me personally I think the current default to fail if no elements are present and in esense guarding against error is important. I seen many case where something was silently turned off due to error and the error was missed.
*Also adding a skipped note in the execution plan could be in many cases be effectively silent if it does not actually fail a test run

@gaganis
Copy link
Contributor

gaganis commented Jun 25, 2018

n any case, we should improve the error message.

"Configuration error: You must provide at least one argument for this @ParameterizedTest" --> "Configuration error: You must provide at least one invocation context for this @ParameterizedTest".

The difference being "argument" vs. "invocation context".

I don't think invocation context would be more meaningful for the normal user. Though indeed it is more meaningful for someone who understands the internal of the platform.

If you consult the relevant section in the User Guide about ParameterizedTests there is no mention, let alone an explanation, of "Invocation Context".

@sbrannen
Copy link
Member

Sure.... "invocation context" is very technical.

How about the following instead?

"Configuration error: You must configure at least one set of arguments for this @ParameterizedTest"

@sbrannen
Copy link
Member

sbrannen commented Jun 25, 2018

Good points, @gaganis.

Maybe we could take a similar approach also in the case- with some sort of flag or parameter eg on the annotation.

I tend to agree that adding an opt-in flag as a annotation attribute in @ParameterizedTest would be a better, more robust, and more user friendly solution.

How about the following?

@ParameterizedTest(failIfNoArgumentsProvided = false)

... with the default being true.

Though, failIfNoArgumentsProvided is a bit verbose. Perhaps we can come up with something more succinct.

@sbrannen
Copy link
Member

Maybe the following is better:

@ParameterizedTest(requireArguments = false)

@gaganis
Copy link
Contributor

gaganis commented Jun 25, 2018

How about the following instead?

"Configuration error: You must configure at least one set of arguments for this @ParameterizedTest"

That is exactly was I was thinking now too :)

@sbrannen
Copy link
Member

@johnchurchill, would the following proposal suit your needs?

@ParameterizedTest(requireArguments = false)

@sbrannen
Copy link
Member

FYI: I improved the error message in b1e533c.

@johnchurchill
Copy link
Author

Yes, requireArguments = false would suit my needs perfectly. I hate to see more corner-case parameters being introduced though. The simple API is what makes JUnit so approachable.

Andrei94 pushed a commit to Andrei94/junit5 that referenced this issue Jun 26, 2018
@sbrannen
Copy link
Member

Yes, requireArguments = false would suit my needs perfectly.

Thanks for the feedback.

I hate to see more corner-case parameters being introduced though. The simple API is what makes JUnit so approachable.

Well, therein lies the rub: We can't have our cake and eat it, too.

As already pointed out, switching the behavior would no longer be user friendly regarding user configuration errors. So, IMHO, the only viable option is to make it an opt-in feature while retaining backwards compatibility and upholding UX.

Of course, if anyone has any better ideas, I'm all ears.

@sbrannen
Copy link
Member

Thanks for all of the comments, everyone!

We will discuss this topic again within the team.

@mischa-n
Copy link

Regarding the comment

It's a breaking change in the sense that configuration errors previously caused the parameterized test container to fail; whereas, now such configuration errors would be silently ignored.

I can hardly imagine such a configuration error, can you name one?

@sbrannen
Copy link
Member

sbrannen commented Jan 31, 2024

I can hardly imagine such a configuration error, can you name one?

Yes.

@ParameterizedTest
@MethodSource
void strings(String str) {
}

static List<String> strings() {
	return List.of();
}

That's an obvious user/configuration error.

@sbrannen
Copy link
Member

Team Decision

  • Allow a TestTemplateInvocationContextProvider to signal that it may return an empty Stream from provideTestTemplateInvocationContexts() -- likely via a new default method which returns false and can be overridden in concrete implementations.
  • Provide an attribute in @ParameterizedTest which allows the user to signal that zero invocations are permissible -- for example, requireArguments = false (where requireArguments defaults to true) as mentioned previously in the discussions in this issue.

@sbrannen sbrannen changed the title ParameterizedTestExtension does not allow zero invocations Introduce support for zero invocations in test templates and parameterized tests Jan 31, 2024
@marcphilipp marcphilipp modified the milestones: 5.11 M1, 5.12 Feb 2, 2024
@marcphilipp marcphilipp modified the milestones: 5.11 M2, 5.11 M3 May 17, 2024
@marcphilipp marcphilipp modified the milestones: 5.11 M3, 5.12 Jul 5, 2024
@nskvortsov
Copy link
Contributor

We would also benefit from this fix. Is any help required here?

@marcphilipp
Copy link
Member

@nskvortsov A PR implementing the above team decision would be welcome!

nskvortsov added a commit to nskvortsov/junit5 that referenced this issue Jul 17, 2024
Add a flag to ParameterizedTest to control arguments requirement. This
allows users to explicitly opt out from validation of arguments set
count and silently skip a test if no arguments are provided

In general, support TestTemplateInvocationContextProvider returning zero
invocation contexts. Such providers must override new interface method
to indicate that the framework should expect "no context returned"

Resolves junit-team#1477
nskvortsov added a commit to nskvortsov/junit5 that referenced this issue Jul 17, 2024
Add a flag to ParameterizedTest to control arguments requirement. This
allows users to explicitly opt out from validation of arguments set
count and silently skip a test if no arguments are provided

In general, support TestTemplateInvocationContextProvider returning zero
invocation contexts. Such providers must override new interface method
to indicate that the framework should expect "no context returned"

Resolves junit-team#1477
@nskvortsov
Copy link
Contributor

Here it is 🙂,
Hope it helps.

@nskvortsov
Copy link
Contributor

Can I help in any way to proceed with code review of the pull request?

@marcphilipp
Copy link
Member

No, I don't think so. It's just a matter of someone from the core team taking a proper look.

@nskvortsov
Copy link
Contributor

Dear team, I would be happy to improve my pull request. Please give me some feedback on it )

@marcphilipp
Copy link
Member

@nskvortsov Sorry for the delay! I have reviewed the PR just now.

marcphilipp pushed a commit to nskvortsov/junit5 that referenced this issue Oct 15, 2024
Add a flag to ParameterizedTest to control arguments requirement. This
allows users to explicitly opt out from validation of arguments set
count and silently skip a test if no arguments are provided

In general, support TestTemplateInvocationContextProvider returning zero
invocation contexts. Such providers must override new interface method
to indicate that the framework should expect "no context returned"

Resolves junit-team#1477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment