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 @NullSource, @EmptySource, & @NullAndEmptySource for parameterized tests #1637

Open
ttddyy opened this Issue Oct 16, 2018 · 15 comments

Comments

Projects
None yet
4 participants
@ttddyy

ttddyy commented Oct 16, 2018

Overview

Similar to #1635, I stumbled upon a situation where I need to validate null in parameterized test.

@ParameterizedTest
@ValueSource(strings = { "wrong-input", "", " " })
void wrongInput(String input) {
  ...
}

It is certainly convenient to have a way of testing null use case without creating a custom provider or another test method just for null.

Original Proposal

How about adding a flag attribute to @ValueSource annotation such as testNull, checkNull, etc.

@ParameterizedTest
@ValueSource(strings = { "wrong-input", "", " " }, testNull=true)
void wrongInput(String input) {
  ...
}

Related Issues

Deliverables

  • Introduce @NullSource that provides null for any reference type and throws an exception for any primitive target type.
  • Introduce @EmptySource that provides an empty object for supported types and throws an exception for any primitive target type.
    • Initial supported types: String, List, Set, Map, and arrays of any type.
  • Introduce @NullAndEmptySource composed annotation that combines @NullSource and @EmptySource.
  • Document in User Guide.
  • Document in Release Notes.
@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 16, 2018

We typically would not want to introduce an annotation attribute that is only applicable part of the time. For example, null does not make sense for a primitive target type.

On the other hand, you can already pass null arguments via @MethodSource or @CsvSource as follows.

@ParameterizedTest
@CsvSource({ "wrong-input,", ",", "' '," })
void test(String s) {
	System.out.println(s == null ? null : "'" + s + "'");
}

That's perhaps a bit of a hack, but... it works. 😇

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 16, 2018

Oh yeah, the output for the above is:

'wrong-input'
null
' '
@marcphilipp

This comment has been minimized.

Member

marcphilipp commented Oct 17, 2018

You could define a @NullSource and use it in addition to @ValueSource.

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@ArgumentsSource(NullArgumentsProvider.class)
public @interface NullSource {}

class NullArgumentsProvider implements ArgumentsProvider {
	@Override
	public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
		return Stream.of(Arguments.of(new Object[context.getRequiredTestMethod().getParameterCount()]));
	}
}

@sbrannen sbrannen added this to the 5.4 M1 milestone Oct 17, 2018

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 17, 2018

@junit-team/junit-lambda, Tentatively slated for 5.4 M1 for the purpose of team discussion in conjunction with #1635.

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 17, 2018

Good point, @marcphilipp.

In light of that proposal and the discussions in #1635, if we decide to include any such built-in sources, I think it might be best to introduce the following.

  • @NullSource: provides null for any reference type and throws an exception for any primitive target type.
  • @EmptySource: provides an empty object for supported types and throws an exception for any primitive target type.
    • Initial supported types: String, List, Set, Map, and arrays of any type.

For the blank String source proposed in #1635, I think we should just let people continue to use @ValueSource( strings = { ... } ) for that purpose.

Thoughts?

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 17, 2018

That would lead to solutions similar to the following for the proposals in #1635.

class Tests {

	@ParameterizedTest
	@NullSource
	@EmptySource
	void nullAndEmptyStrings(String str) {
		assertTrue(str == null || str.isEmpty());
	}

	@ParameterizedTest
	@NullSource
	@EmptySource
	void nullAndEmptyLists(List<?> list) {
		assertTrue(list == null || list.isEmpty());
	}
}
@ttddyy

This comment has been minimized.

ttddyy commented Oct 17, 2018

Sounds great to have them in junit-jupiter-params

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 18, 2018

And for blank strings, one can simply use @ValueSource in contrast to the proposal in #1635.

The following would supply null, empty, and blank strings as arguments.

class Tests {

	@ParameterizedTest
	@NullSource
	@EmptySource
	@ValueSource(strings = {" ", "   ", "\t", "\n"})
	void nullEmptyAndBlankStrings(String str) {
		assertTrue(str == null || str.trim().isEmpty());
	}
}

If somebody wants to reuse all three of those sources, it's straightforward to introduce a custom composed annotation that is meta-annotated with those three source annotations.

Of course, JUnit Jupiter could also provide a pre-built @NullAndEmptySource composed annotation out of the box if we want to.

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 18, 2018

You could define a @NullSource and use it in addition to @ValueSource.

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@ArgumentsSource(NullArgumentsProvider.class)
public @interface NullSource {}

class NullArgumentsProvider implements ArgumentsProvider {
	@Override
	public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
		return Stream.of(Arguments.of(new Object[context.getRequiredTestMethod().getParameterCount()]));
	}
}

I just noticed that your proposal would generate nulls for all arguments, which would be risky since that would prevent arguments being supplied by other ParameterResolvers.

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 18, 2018

In light of my previous comment, I'm thinking that @NullSource and @EmptySource should only provide a single argument (namely, the first argument).

@jmaniquet

This comment has been minimized.

jmaniquet commented Oct 19, 2018

Hi

If i understand right, those new annotations can be combined with another source annotation, to supplement it with the null or empty value?

That would solve the problem i tried to solve in #1635, so i like it.

As for a @NullAndEmptySource, i think it is one of the most common use case (if not the most common one). So it would be valuable to have it baseline in the framework.

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 20, 2018

If i understand right, those new annotations can be combined with another source annotation, to supplement it with the null or empty value?

Yes, that is correct.

All sources are combined into a single stream:

public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts(
ExtensionContext extensionContext) {
Method templateMethod = extensionContext.getRequiredTestMethod();
String displayName = extensionContext.getDisplayName();
ParameterizedTestMethodContext methodContext = getStore(extensionContext)//
.get(METHOD_CONTEXT_KEY, ParameterizedTestMethodContext.class);
ParameterizedTestNameFormatter formatter = createNameFormatter(templateMethod, displayName);
AtomicLong invocationCount = new AtomicLong(0);
// @formatter:off
return findRepeatableAnnotations(templateMethod, ArgumentsSource.class)
.stream()
.map(ArgumentsSource::value)
.map(this::instantiateArgumentsProvider)
.map(provider -> AnnotationConsumerInitializer.initialize(templateMethod, provider))
.flatMap(provider -> arguments(provider, extensionContext))
.map(Arguments::get)
.map(arguments -> consumedArguments(arguments, methodContext))
.map(arguments -> createInvocationContext(formatter, methodContext, arguments))
.peek(invocationContext -> invocationCount.incrementAndGet())
.onClose(() ->
Preconditions.condition(invocationCount.get() > 0,
"Configuration error: You must configure at least one set of arguments for this @ParameterizedTest"));
// @formatter:on
}

That would solve the problem i tried to solve in #1635, so i like it.

Great. In light of that, I'll close #1635.

As for a @NullAndEmptySource, i think it is one of the most common use case (if not the most common one). So it would be valuable to have it baseline in the framework.

OK. We will take that into consideration.

Thanks for the feedback!

@sbrannen sbrannen changed the title from Add null usecase flag on @ValueSource to Introduce @NullSource, @EmptySource, & @NullAndEmptySource for parameterized tests Oct 20, 2018

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 20, 2018

Update: I have renamed this issue, reworked the description, and added Deliverables.

@sbrannen

This comment has been minimized.

Member

sbrannen commented Oct 27, 2018

In response to this blog which I discovered via this tweet, there is no need to implement a custom NullableConverter for use with @CsvSource.

Given the following DateRange:

import java.time.LocalDate;

public class DateRange {

	private final LocalDate startDate;
	private final LocalDate endDate;

	public DateRange(LocalDate startDate, LocalDate endDate) {
		if (startDate == null && endDate == null) {
			throw new IllegalArgumentException();
		}
		if (startDate != null && endDate != null && startDate.isAfter(endDate)) {
			throw new IllegalArgumentException();
		}
		this.startDate = startDate;
		this.endDate = endDate;
	}

	@Override
	public String toString() {
		return "DateRange [startDate=" + startDate + ", endDate=" + endDate + "]";
	}

}

... the solution to the problem mentioned in that blog is to use empty values as follows:

@ParameterizedTest
@CsvSource({ "2017-06-01, 2018-10-15", ", 2018-10-15", "2017-06-01," })
void shouldCreateValidDateRange(LocalDate startDate, LocalDate endDate) {
	System.out.println(new DateRange(startDate, endDate));
}

@ParameterizedTest
@CsvSource({ "2018-10-15, 2017-06-01", "," })
void shouldNotCreateInvalidDateRange(LocalDate startDate, LocalDate endDate) {
	assertThrows(IllegalArgumentException.class, () -> new DateRange(startDate, endDate));
}

@sbrannen sbrannen self-assigned this Nov 6, 2018

@marcphilipp marcphilipp modified the milestones: 5.4 M1, 5.4 M2 Nov 23, 2018

@marcphilipp

This comment has been minimized.

Member

marcphilipp commented Nov 23, 2018

Team Decision:

Let's add all three annotations.

@sbrannen sbrannen modified the milestones: 5.4 M2, 5.4 M1 Dec 1, 2018

@marcphilipp marcphilipp modified the milestones: 5.4 M1, 5.4 M2 Dec 7, 2018

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