-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add List-based support to Arguments API #4574
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
3f3aa29
d566f95
b73ae11
87ca6b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,10 @@ | |||||||||||||
import static org.apiguardian.api.API.Status.EXPERIMENTAL; | ||||||||||||||
import static org.apiguardian.api.API.Status.STABLE; | ||||||||||||||
|
||||||||||||||
import java.util.ArrayList; | ||||||||||||||
import java.util.Arrays; | ||||||||||||||
import java.util.List; | ||||||||||||||
|
||||||||||||||
import org.apiguardian.api.API; | ||||||||||||||
import org.jspecify.annotations.Nullable; | ||||||||||||||
import org.junit.platform.commons.util.Preconditions; | ||||||||||||||
|
@@ -172,4 +176,72 @@ public String toString() { | |||||||||||||
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Factory method for creating an instance of {@code Arguments} based on | ||||||||||||||
* the supplied {@code arguments} as a {@link List}. | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(same below for other methods) |
||||||||||||||
* | ||||||||||||||
* @param arguments the arguments as a List to be used for an invocation | ||||||||||||||
* of the test method; must not be {@code null} but may contain {@code null} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(same below for other methods) |
||||||||||||||
* @return an instance of {@code Arguments}; never {@code null} | ||||||||||||||
* @see #arguments(List) | ||||||||||||||
*/ | ||||||||||||||
@API(status = EXPERIMENTAL, since = "6.0") | ||||||||||||||
static Arguments of(@Nullable List<@Nullable Object> arguments) { | ||||||||||||||
if (arguments == null) { | ||||||||||||||
return of((Object) null); // Properly wrap null | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
if (arguments.isEmpty()) { | ||||||||||||||
// Must still return empty arguments array | ||||||||||||||
return of(new Object[0]); | ||||||||||||||
} | ||||||||||||||
return () -> arguments.toArray(new Object[0]); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Factory method for creating an instance of {@code Arguments} based on | ||||||||||||||
* the supplied {@code arguments} as a {@link List}. | ||||||||||||||
* | ||||||||||||||
* <p>This method is an <em>alias</em> for {@link Arguments#of} and is | ||||||||||||||
* intended to be used when statically imported — for example, via: | ||||||||||||||
* {@code import static org.junit.jupiter.params.provider.Arguments.arguments;} | ||||||||||||||
* | ||||||||||||||
* @param arguments the arguments as a List to be used for an invocation of the test | ||||||||||||||
* method; must not be {@code null} but may contain {@code null} | ||||||||||||||
* @return an instance of {@code Arguments}; never {@code null} | ||||||||||||||
* @since 6.0 | ||||||||||||||
* @see #argumentSet(String, Object...) | ||||||||||||||
*/ | ||||||||||||||
@API(status = EXPERIMENTAL, since = "6.0") | ||||||||||||||
static Arguments arguments(List<@Nullable Object> arguments) { | ||||||||||||||
return of(arguments); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Factory method for creating an {@link ArgumentSet} based on the supplied | ||||||||||||||
* {@code name} and {@code arguments} as a List. | ||||||||||||||
* | ||||||||||||||
* @param name the name of the argument set; must not be {@code null} or blank | ||||||||||||||
* @param arguments the arguments list to be used for an invocation of the test | ||||||||||||||
* method; must not be {@code null} but may contain {@code null} | ||||||||||||||
* @return an {@code ArgumentSet}; never {@code null} | ||||||||||||||
* @since 6.0 | ||||||||||||||
*/ | ||||||||||||||
@API(status = EXPERIMENTAL, since = "6.0") | ||||||||||||||
static ArgumentSet argumentSet(String name, List<@Nullable Object> arguments) { | ||||||||||||||
Preconditions.notBlank(name, "name must not be null or blank"); | ||||||||||||||
Preconditions.notNull(arguments, "arguments list must not be null"); | ||||||||||||||
return new ArgumentSet(name, arguments.toArray(new Object[0])); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* Convert the arguments to a mutable List. | ||||||||||||||
* | ||||||||||||||
* @return a mutable List of arguments; never {@code null} but may contain {@code null} | ||||||||||||||
* @since 6.0 | ||||||||||||||
*/ | ||||||||||||||
@API(status = EXPERIMENTAL, since = "6.0") | ||||||||||||||
default List<@Nullable Object> toList() { | ||||||||||||||
return new ArrayList<>(Arrays.asList(get())); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,7 @@ public Stream<? extends Arguments> provideArguments(ParameterDeclarations parame | |
return Stream.of(arguments(Collections.emptySet())); | ||
} | ||
if (List.class.equals(parameterType)) { | ||
return Stream.of(arguments(Collections.emptyList())); | ||
return Stream.of(Arguments.of((Object) Collections.emptyList())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me wonder whether we should choose different names instead of overloading @junit-team/junit-5 WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, we unfortunately overlooked the fact that a single Existing code must still compile without adding casts. Though, in our defense, it is easy to overlook something like that when "coding in the browser". 😉
I would rather go with something consistent, instead of having "copy" in only one of the names. How about something like We could also drop the Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the thoughtful feedback! I agree that it's important to avoid breaking existing usage. I also like the idea of going with a more general naming convention, so my preference would be to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on the shorter variants. We might even get away with adding overloads to that in the future because "conflicts" with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated the code with the suggestions! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd pondered it more while away from the computer and decided that I'm in favor of the
So, I'm glad to see you both agree and that you're already implemented the changes, @mariokhoury4. 👍 |
||
} | ||
if (Set.class.equals(parameterType)) { | ||
return Stream.of(arguments(Collections.emptySet())); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,7 +15,11 @@ | |||||
import static org.junit.jupiter.params.provider.Arguments.arguments; | ||||||
import static org.junit.jupiter.params.provider.Arguments.of; | ||||||
|
||||||
import java.util.Arrays; | ||||||
import java.util.List; | ||||||
|
||||||
import org.junit.jupiter.api.Test; | ||||||
import org.junit.jupiter.params.provider.Arguments.ArgumentSet; | ||||||
|
||||||
/** | ||||||
* Unit tests for {@link Arguments}. | ||||||
|
@@ -56,4 +60,50 @@ void argumentsReturnsSameArrayUsedForCreating() { | |||||
assertThat(arguments.get()).isSameAs(input); | ||||||
} | ||||||
|
||||||
@Test | ||||||
void ofSupportsList() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
List<Object> input = Arrays.asList(1, "two", null, 3.0); | ||||||
Arguments arguments = Arguments.of(input); | ||||||
|
||||||
assertArrayEquals(new Object[] { 1, "two", null, 3.0 }, arguments.get()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an assertion verifying that modifying the initial |
||||||
} | ||||||
|
||||||
@Test | ||||||
void argumentsSupportsListAlias() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
List<Object> input = Arrays.asList("a", 2, null); | ||||||
Arguments arguments = Arguments.arguments(input); | ||||||
|
||||||
assertArrayEquals(new Object[] { "a", 2, null }, arguments.get()); | ||||||
} | ||||||
|
||||||
@Test | ||||||
void argumentSetSupportsList() { | ||||||
List<Object> input = Arrays.asList("x", null, 42); | ||||||
ArgumentSet argumentSet = Arguments.argumentSet("list-test", input); | ||||||
|
||||||
assertArrayEquals(new Object[] { "x", null, 42 }, argumentSet.get()); | ||||||
assertThat(argumentSet.getName()).isEqualTo("list-test"); | ||||||
} | ||||||
|
||||||
@Test | ||||||
void toListReturnsMutableListOfArguments() { | ||||||
Arguments arguments = Arguments.of(Arrays.asList("a", 2, null)); | ||||||
|
||||||
List<Object> result = arguments.toList(); | ||||||
|
||||||
assertThat(result).containsExactly("a", 2, null); // preserves content | ||||||
result.add("extra"); // confirms mutability | ||||||
assertThat(result).contains("extra"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also verify that the array in |
||||||
} | ||||||
|
||||||
@Test | ||||||
void toListWorksOnEmptyArguments() { | ||||||
Arguments arguments = Arguments.of(Arrays.asList()); | ||||||
|
||||||
List<Object> result = arguments.toList(); | ||||||
|
||||||
assertThat(result).isEmpty(); | ||||||
result.add("extra"); | ||||||
assertThat(result).containsExactly("extra"); | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move these next to the other
static
factory methods above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I'll move the method next to the other static factory methods to keep things consistent.