Clean-up and refactoring for 2.0

src/main/java/org/junitpioneer/internal/PioneerAnnotationUtils.java

@@ -151,8 +151,18 @@ private static Stream<Annotation> flatten(Annotation annotation) {
 		}
 	}
 
+	/**
+	 * Checks whether the given annotation is a container annotation for a {@link Repeatable}
+	 * annotation. This is a non-exhaustive check, meaning it does not comply with
+	 * <a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.6.3">
+	 * the full JLS specification</a>. It could be considered "good enough"
+	 * in most practical use-cases.
+	 *
+	 * @param annotation the annotation we want to check for container annotation status
+	 * @return {@code true} if the annotation is a container annotation for a {@link Repeatable} annotation,
+	 * otherwise {@code false}
+	 */
 	public static boolean isContainerAnnotation(Annotation annotation) {
-		// See https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.6.3
 		try {
 			Method value = annotation.annotationType().getDeclaredMethod("value");
 			return value.getReturnType().isArray() && value.getReturnType().getComponentType().isAnnotation()
@@ -204,7 +214,7 @@ private static <A extends Annotation> List<A> findOnMethod(Method element, Class
 
 	private static <A extends Annotation> Stream<A> findOnOuterClasses(Optional<Class<?>> type, Class<A> annotationType,
 			boolean findRepeated, boolean findAllEnclosing) {
-		if (!type.isPresent())
+		if (type.isEmpty())
 			return Stream.empty();
 
 		List<A> onThisClass = Arrays.asList(type.get().getAnnotationsByType(annotationType));
@@ -250,6 +260,18 @@ private static <A extends Annotation> List<A> findOnType(Class<?> element, Class
 				.collect(Collectors.toList());
 	}
 
+	/**
+	 * A helper utility method for {@link org.junitpioneer.jupiter.cartesian.CartesianTest} for finding
+	 * argument sources on parameters. Note that, while it is possible there are multiple valid annotations
+	 * on the parameter, we only take the first annotation.
+	 *
+	 * @param testMethod the test method for which we want to find parameter arguments sources
+	 * @return a list of found arguments source annotations, including {@link CartesianArgumentsSource} annotations,
+	 * never {@code null} but potentially empty.
+	 *
+	 * @see CartesianArgumentsSource
+	 * @see ArgumentsSource
+	 */
 	public static List<Annotation> findParameterArgumentsSources(Method testMethod) {
 		return Arrays
 				.stream(testMethod.getParameters())
@@ -268,6 +290,17 @@ private static List<Annotation> collectArgumentSources(Parameter parameter) {
 		return annotations;
 	}
 
+	/**
+	 * A helper utility method for {@link org.junitpioneer.jupiter.cartesian.CartesianTest} for finding
+	 * argument sources on the test method.
+	 *
+	 * @param testMethod the test method for which we want to find arguments sources
+	 * @return a list of found arguments source annotations, including {@link CartesianArgumentsSource} annotations,
+	 * never {@code null} but potentially empty.
+	 *
+	 * @see CartesianArgumentsSource
+	 * @see ArgumentsSource
+	 */
 	public static List<Annotation> findMethodArgumentsSources(Method testMethod) {
 		return Arrays
 				.stream(testMethod.getAnnotations())

src/main/java/org/junitpioneer/internal/PioneerPreconditions.java

@@ -32,11 +32,7 @@ private PioneerPreconditions() {
 	 * @return the supplied string
 	 */
 	public static String notBlank(String str, String message) {
-		if (str == null || str.trim().isEmpty()) {
-			throw new PreconditionViolationException(message);
-		}
-
-		return str;
+		return notBlank(str, () -> message);
 	}
 
 	/**
@@ -60,10 +56,7 @@ public static String notBlank(String str, Supplier<String> messageSupplier) {
 	 * @return the supplied object
 	 */
 	public static <T> T notNull(T object, String message) {
-		if (object == null) {
-			throw new PreconditionViolationException(message);
-		}
-		return object;
+		return notNull(object, () -> message);
 	}
 
 	/**
@@ -86,10 +79,7 @@ public static <T> T notNull(T object, Supplier<String> messageSupplier) {
 	 * @return the supplied string
 	 */
 	public static <T extends Collection<?>> T notEmpty(T collection, String message) {
-		if (collection == null || collection.isEmpty()) {
-			throw new PreconditionViolationException(message);
-		}
-		return collection;
+		return notEmpty(collection, () -> message);
 	}
 
 	/**

src/main/java/org/junitpioneer/internal/PioneerUtils.java

@@ -81,7 +81,7 @@ public static Optional<Method> findMethodCurrentOrEnclosing(Class<?> clazz, Stri
 			// null checking done by ReflectionSupport.findMethod
 			method = findMethod(current, methodName, parameterTypes);
 			current = current.getEnclosingClass();
-		} while (!method.isPresent() && current != null);
+		} while (method.isEmpty() && current != null);
 		return method;
 	}
 
@@ -103,6 +103,29 @@ public static List<ExtensionContext> findAllContexts(ExtensionContext context) {
 		return allContexts;
 	}
 
+	/**
+	 * This is a self-hosted copy of {@code org.junit.platform.commons.util.StringUtils#nullSafeToString(Object)}.
+	 * This is intentionally not kept up-to-date with JUnit's implementation but that
+	 * does not guarantee that it won't be changed.
+	 *
+	 * Convert the supplied {@code Object} to a {@code String} using the
+	 * following algorithm.
+	 *
+	 * <ul>
+	 * <li>If the supplied object is {@code null}, this method returns {@code "null"}.</li>
+	 * <li>If the supplied object is a primitive array, the appropriate
+	 * {@code Arrays#toString(...)} variant will be used to convert it to a String.</li>
+	 * <li>If the supplied object is an object array, {@code Arrays#deepToString(Object[])}
+	 * will be used to convert it to a String.</li>
+	 * <li>Otherwise, {@code toString()} will be invoked on the object. If the
+	 * result is non-null, that result will be returned. If the result is
+	 * {@code null}, {@code "null"} will be returned.</li>
+	 * </ul>
+	 *
+	 * @param object the object to convert to a String; may be {@code null}
+	 * @return a String representation of the supplied object; never {@code null}
+	 * @see Arrays#deepToString(Object[])
+	 */
 	public static String nullSafeToString(Object object) {
 		if (object == null) {
 			return "null";
@@ -165,14 +188,56 @@ public static List<List<?>> cartesianProduct(List<List<?>> lists) {
 		return resultLists;
 	}
 
+	/**
+	 * Static factory method for creating a Locale using {@link Locale.Builder}.
+	 * Required because Locale constructors are deprecated.
+	 * Will rethrow any syntax exceptions from the builder.
+	 *
+	 * @param language An ISO 639 alpha-2 or alpha-3 language code, or a language subtag
+	 * up to 8 characters in length.
+	 * @param country An ISO 3166 alpha-2 country code or a UN M.49 numeric-3 area code.
+	 * @param variant Any arbitrary value used to indicate a variation of a {@code Locale}.
+	 * @return a new Locale instance constructed by {@link Locale.Builder}
+	 * @throws java.util.IllformedLocaleException if {@link Locale.Builder} throws
+	 *
+	 * @see Locale
+	 * @see Locale.Builder
+	 */
 	public static Locale createLocale(String language, String country, String variant) {
 		return new Locale.Builder().setLanguage(language).setRegion(country).setVariant(variant).build();
 	}
 
+	/**
+	 * Static factory method for creating a Locale using {@link Locale.Builder}.
+	 * Required because Locale constructors are deprecated.
+	 * Will rethrow any syntax exceptions from the builder.
+	 *
+	 * @param language An ISO 639 alpha-2 or alpha-3 language code, or a language subtag
+	 * up to 8 characters in length.
+	 * @param country An ISO 3166 alpha-2 country code or a UN M.49 numeric-3 area code.
+	 * @return a new Locale instance constructed by {@link Locale.Builder}
+	 * @throws java.util.IllformedLocaleException from the {@link Locale.Builder}
+	 *
+	 * @see Locale
+	 * @see Locale.Builder
+	 */
 	public static Locale createLocale(String language, String country) {
 		return new Locale.Builder().setLanguage(language).setRegion(country).build();
 	}
 
+	/**
+	 * Static factory method for creating a Locale using {@link Locale.Builder}.
+	 * Required because Locale constructors are deprecated.
+	 * Will rethrow any syntax exceptions from the builder.
+	 *
+	 * @param language An ISO 639 alpha-2 or alpha-3 language code, or a language subtag
+	 * up to 8 characters in length.
+	 * @return a new Locale instance constructed by {@link Locale.Builder}
+	 * @throws java.util.IllformedLocaleException from the {@link Locale.Builder}
+	 *
+	 * @see Locale
+	 * @see Locale.Builder
+	 */
 	public static Locale createLocale(String language) {
 		return new Locale.Builder().setLanguage(language).build();
 	}

src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java

@@ -138,7 +138,7 @@ private void setEntries(Map<K, V> entriesToSet) {
 	}
 
 	@Override
-	public void afterEach(ExtensionContext context) throws Exception {
+	public void afterEach(ExtensionContext context) {
 		// apply from innermost to outermost
 		PioneerUtils.findAllContexts(context).forEach(this::restoreOriginalEntries);
 	}

src/main/java/org/junitpioneer/jupiter/DefaultLocale.java

@@ -36,7 +36,7 @@
  *     </li>
  * </ul>
  *
- * <p>Please keep in mind the the {@code Locale.Builder} does a syntax check, if you use a variant!
+ * <p>Please keep in mind the {@code Locale.Builder} does a syntax check, if you use a variant!
  * The given string must match the BCP 47 (or more detailed <a href="https://www.rfc-editor.org/rfc/rfc5646.html">RFC 5646</a>) syntax.</p>
  *
  * <p>If a language tag is set, none of the other fields must be set. Otherwise an

src/main/java/org/junitpioneer/jupiter/DisableIfTestFailsExtension.java

@@ -72,7 +72,7 @@ public void handleTestExecutionException(ExtensionContext context, Throwable thr
 	private static Stream<Configuration> findConfigurations(ExtensionContext context) {
 		Optional<Class<?>> type = context.getTestClass();
 		// type may not be present because of recursion to the parent context
-		if (!type.isPresent())
+		if (type.isEmpty())
 			return Stream.empty();
 
 		List<DisableIfTestFails> annotations = findAnnotationOn(type.get()).collect(toList());
@@ -112,9 +112,7 @@ private static Stream<DisableIfTestFails> findAnnotationOn(Class<?> element) {
 
 		Stream<DisableIfTestFails> onElement = AnnotationSupport
 				.findAnnotation(element, DisableIfTestFails.class)
-				// turn Optional into Stream
-				.map(Stream::of)
-				.orElse(Stream.empty());
+				.stream();
 		Stream<DisableIfTestFails> onInterfaces = Arrays
 				.stream(element.getInterfaces())
 				.flatMap(DisableIfTestFailsExtension::findAnnotationOn);

src/main/java/org/junitpioneer/jupiter/EnvironmentVariableUtils.java

@@ -68,8 +68,12 @@ private static void trySystemEnvClass(Consumer<Map<String, String>> consumer,
 	/*
 	 * Works on Windows
 	 */
+	@SuppressWarnings("Java9ReflectionClassVisibility")
 	private static void setInProcessEnvironmentClass(Consumer<Map<String, String>> consumer)
 			throws ReflectiveOperationException {
+		// The Java 9 access restriction problem is explicitly documented in our user guide
+		// for the EnvironmentVariable extension
+		// https://junit-pioneer.org/docs/environment-variables/#warnings-for-reflective-access
 		Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment");
 		// The order of operations is critical here: On some operating systems, theEnvironment is present but
 		// theCaseInsensitiveEnvironment is not present. In such cases, this method must throw a

src/main/java/org/junitpioneer/jupiter/IssueExtension.java

@@ -17,7 +17,7 @@
 import org.junitpioneer.internal.PioneerAnnotationUtils;
 
 /**
- * This class implements the functionality for the {@code @Issue} annotation.
+ * This class implements the functionality for the {@link Issue} annotation.
  *
  * @see Issue
  */

src/main/java/org/junitpioneer/jupiter/IssueProcessor.java

@@ -21,7 +21,7 @@
 public interface IssueProcessor {
 
 	/**
-	 * Processes results of `@Issue` annotated test cases grouped by the issueId, called {@link IssueTestSuite}.
+	 * Processes results of {@link Issue} annotated test cases grouped by the issueId, called {@link IssueTestSuite}.
 	 *
 	 * @param issueTestSuites List of issues, each with a list of test cases annotated with their issueId
 	 */

src/main/java/org/junitpioneer/jupiter/cartesian/CartesianArgumentsSource.java

@@ -37,8 +37,12 @@
 
 	/**
 	 * The type of {@link CartesianArgumentsProvider} to be used.
+	 *
+	 * Note that {@link CartesianArgumentsProvider} is never used
+	 * directly, only as either {@link CartesianMethodArgumentsProvider}
+	 * or {@link CartesianParameterArgumentsProvider}.
 	 */
-	@SuppressWarnings("rawtypes")
+	@SuppressWarnings("ClassEscapesDefinedScope")
 	Class<? extends CartesianArgumentsProvider> value();
 
 }

src/main/java/org/junitpioneer/jupiter/issue/IssueExtensionExecutionListener.java

@@ -10,13 +10,11 @@
 
 package org.junitpioneer.jupiter.issue;
 
+import static java.util.stream.Collectors.groupingBy;
 import static java.util.stream.Collectors.toList;
-import static java.util.stream.Collectors.toMap;
+import static java.util.stream.Collectors.toUnmodifiableList;
 import static org.junit.platform.engine.TestExecutionResult.Status;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.ServiceLoader;
@@ -96,23 +94,18 @@ public void testPlanExecutionFinished(TestPlan testPlan) {
 
 	List<IssueTestSuite> createIssueTestSuites() {
 		//@formatter:off
-		List<IssueTestSuite> suites = testCases
+		return testCases
 				.values().stream()
-				.collect(toMap(IssueTestCaseBuilder::getIssueId, builder -> new ArrayList<>(Arrays.asList(builder)),
-						(builders1, builders2) -> {
-							builders1.addAll(builders2);
-							return builders1;
-						}))
+				.collect(groupingBy(IssueTestCaseBuilder::getIssueId))
 				.entrySet().stream()
 				.map(issueIdWithTestCases -> new IssueTestSuite(
 						issueIdWithTestCases.getKey(),
 						issueIdWithTestCases
 								.getValue().stream()
 								.map(IssueTestCaseBuilder::build)
 								.collect(toList())))
-				.collect(toList());
+				.collect(toUnmodifiableList());
 		//@formatter:on
-		return Collections.unmodifiableList(suites);
 	}
 
 }

src/main/java/org/junitpioneer/jupiter/params/DisableIfAllArguments.java

@@ -45,7 +45,7 @@
 
 	/**
 	 * Disable test cases if all arguments (converted to String with {@link Object#toString()})
-	 * contain any of the the specified strings (according to {@link String#contains(CharSequence)}).
+	 * contain any of the specified strings (according to {@link String#contains(CharSequence)}).
 	 */
 	String[] contains() default {};
 

src/main/java/org/junitpioneer/jupiter/params/DisableIfAnyArgument.java

@@ -45,7 +45,7 @@
 
 	/**
 	 * Disable test cases if any argument (converted to String with {@link Object#toString()})
-	 * contains any of the the specified strings (according to {@link String#contains(CharSequence)}).
+	 * contains any of the specified strings (according to {@link String#contains(CharSequence)}).
 	 */
 	String[] contains() default {};
 

src/main/java/org/junitpioneer/jupiter/params/DisableIfArgument.java

@@ -63,7 +63,7 @@
 
 	/**
 	 * Disable test cases whose argument (converted to String with {@link Object#toString()})
-	 * contains any of the the specified strings (according to {@link String#contains(CharSequence)}).
+	 * contains any of the specified strings (according to {@link String#contains(CharSequence)}).
 	 */
 	String[] contains() default {};
 

src/main/java/org/junitpioneer/jupiter/params/DisableIfArgumentExtension.java

@@ -70,30 +70,27 @@ public void interceptTestTemplateMethod(Invocation<Void> invocation,
 	}
 
 	private static void checkRequiredAnnotations(Method testMethod) {
-		if (!AnnotationSupport.findAnnotation(testMethod, DisableIfAnyArgument.class).isPresent()
-				&& !AnnotationSupport.findAnnotation(testMethod, DisableIfAllArguments.class).isPresent()
+		if (AnnotationSupport.findAnnotation(testMethod, DisableIfAnyArgument.class).isEmpty()
+				&& AnnotationSupport.findAnnotation(testMethod, DisableIfAllArguments.class).isEmpty()
 				&& AnnotationSupport.findRepeatableAnnotations(testMethod, DisableIfArgument.class).isEmpty()) {
 			throw new ExtensionConfigurationException(
 				"Required at least one of the following: @DisableIfArgument, @DisableIfAllArguments, @DisableIfAnyArgument but found none.");
 		}
 	}
 
-	private static DisableIfAllArguments verifyNonEmptyInputs(DisableIfAllArguments annotation) {
+	private static void verifyNonEmptyInputs(DisableIfAllArguments annotation) {
 		if (annotation.contains().length > 0 == annotation.matches().length > 0)
 			throw invalidInputs(DisableIfAllArguments.class);
-		return annotation;
 	}
 
-	private static DisableIfAnyArgument verifyNonEmptyInputs(DisableIfAnyArgument annotation) {
+	private static void verifyNonEmptyInputs(DisableIfAnyArgument annotation) {
 		if (annotation.contains().length > 0 == annotation.matches().length > 0)
 			throw invalidInputs(DisableIfAnyArgument.class);
-		return annotation;
 	}
 
-	private static DisableIfArgument verifyNonEmptyInputs(DisableIfArgument annotation) {
+	private static void verifyNonEmptyInputs(DisableIfArgument annotation) {
 		if (annotation.contains().length > 0 == annotation.matches().length > 0)
 			throw invalidInputs(DisableIfArgument.class);
-		return annotation;
 	}
 
 	private static RuntimeException invalidInputs(Class<?> annotationClass) {