Introduce and enforce syntax for tags

Prior to this commit, tags were trimmed (#942) but were otherwise
allowed to contain any type of character, including whitespace or ISO
control characters. However, this degree of flexibility can lead to
subtle configuration errors and also makes it challenging to reason
about a tag expression language as proposed in #927.

This commit addresses these issues by introducing and enforcing a
syntax for tags. Specifically, all tags must now conform to the
following syntax rules.

* A tag must not be null or blank.
* A trimmed tag must not contain whitespace.
* A trimmed tag must not contain ISO control characters.

Consequently, the following changes make it impossible to create a
TestTag or TagFilter based on a tag that is not syntactically valid.

* The TagFilter.includeTags(), TagFilter.excludeTags(), and
  TestTag.create() factory methods now throw a
  PreconditionViolationException if a supplied tag is not syntactically
  valid.
* @tag declarations containing invalid tag syntax will now be logged
  as a warning but effectively ignored.

Furthermore, a new TestTag.isValid(String) method has been introduced
for checking if a tag is syntactically valid. This allows test engines
to preemptively validate the syntax of a tag provided by a user before
attempting to register such an invalid tag.

Issue: #956

Author: sbrannen

documentation/src/docs/asciidoc/release-notes-5.0.0-M6.adoc

@@ -20,14 +20,21 @@ on GitHub.
 
 ===== Bug Fixes
 
-* All tags are now trimmed in order to remove leading and trailing whitespace. This
+* All tags are now _trimmed_ in order to remove leading and trailing whitespace. This
   applies to any tag supplied _directly_ to the `Launcher` via `TagFilter.includeTags()`
   and `TagFilter.excludeTags()` or _indirectly_ via `@IncludeTags`, `@ExcludeTags`, the
   JUnit Platform Console Launcher, the JUnit Platform Gradle plugin, and the JUnit
-  Platform Maven Surefire provider. Furthermore, blank tags are now ignored.
+  Platform Maven Surefire provider.
 
 ===== Deprecations and Breaking Changes
 
+* All tags must now conform to the following syntax rules:
+** A tag must not be `null` or _blank_.
+** A _trimmed_ tag must not contain whitespace.
+** A _trimmed_ tag must not contain ISO control characters.
+* The `TagFilter.includeTags()`, `TagFilter.excludeTags()`, and `TestTag.create()`
+  factory methods now throw a `PreconditionViolationException` if a supplied tag is not
+  syntactically valid.
 * The method `getDiscoveryFiltersByType` of `EngineDiscoveryRequest` has been renamed to
   `getFiltersByType`.
 * The method `getSegments()` of `UniqueId` now returns an immutable list.
@@ -36,9 +43,9 @@ on GitHub.
 
 ===== New Features and Improvements
 
+* New `TestTag.isValid(String)` method for checking if a tag is syntactically valid.
 * The `findAnnotation()` method in `AnnotationSupport` now searches on interfaces
   implemented by a class if the supplied element is a class.
-
 * The following methods of `org.junit.platform.commons.util.ReflectionUtils` are now
   exposed via `org.junit.platform.commons.support.ReflectionSupport`:
 ** `public static Optional<Class<?>> loadClass(String name)`
@@ -48,13 +55,14 @@ on GitHub.
 ** `public static Object invokeMethod(Method method, Object target, Object... args)`
 ** `public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate)`
 
+
 [[release-notes-5.0.0-m6-junit-jupiter]]
 ==== JUnit Jupiter
 
 ===== Bug Fixes
 
 * All tags declared via `@Tag` are now trimmed in order to remove leading and trailing
-  whitespace. Furthermore, blank tags are now ignored.
+  whitespace.
 * All primitive array types (from `boolean[]` to `short[]`) are now supported as a
   return type of static argument providing methods for parameterized tests.
 
@@ -79,7 +87,8 @@ on GitHub.
   method. This helps to debug errors resulting from a method being simultaneously
   annotated with multiple competing annotations such as `@Test`, `@RepeatedTest`,
   `@ParameterizedTest`, `@TestFactory`, etc.
-
+* `@Tag` declarations containing invalid tag syntax will now be logged as a warning but
+  effectively ignored.
 
 
 [[release-notes-5.0.0-m6-junit-vintage]]

documentation/src/docs/asciidoc/writing-tests.adoc

@@ -154,6 +154,15 @@ include::{testDir}/example/DisabledTestsDemo.java[tags=user_guide]
 Test classes and methods can be tagged. Those tags can later be used to filter
 <<running-tests,test discovery and execution>>.
 
+==== Syntax Rules for Tags
+
+* A tag must not be `null` or _blank_.
+* A _trimmed_ tag must not contain whitespace.
+* A _trimmed_ tag must not contain ISO control characters.
+
+NOTE: In the above context, "trimmed" means that leading and trailing whitespace
+characters have been removed.
+
 [source,java,indent=0]
 ----
 include::{testDir}/example/TaggingDemo.java[tags=user_guide]

documentation/src/test/java/example/TestInfoDemo.java

@@ -35,10 +35,10 @@ void init(TestInfo testInfo) {
 
 	@Test
 	@DisplayName("TEST 1")
-	@Tag("my tag")
+	@Tag("my-tag")
 	void test1(TestInfo testInfo) {
 		assertEquals("TEST 1", testInfo.getDisplayName());
-		assertTrue(testInfo.getTags().contains("my tag"));
+		assertTrue(testInfo.getTags().contains("my-tag"));
 	}
 
 	@Test

junit-jupiter-api/src/main/java/org/junit/jupiter/api/Tag.java

@@ -31,6 +31,13 @@
  * a list of tags to be used for the current test plan, potentially
  * dependent on the current environment.
  *
+ * <h3>Syntax Rules for Tags</h3>
+ * <ul>
+ * <li>A tag must not be blank.</li>
+ * <li>A trimmed tag must not contain whitespace.</li>
+ * <li>A trimmed tag must not contain ISO control characters.</li>
+ * </ul>
+ *
  * @since 5.0
  * @see Tags
  * @see Test
@@ -45,8 +52,10 @@
 	/**
 	 * The <em>tag</em>.
 	 *
-	 * <p>Note: the tag will be {@linkplain String#trim() trimmed},
-	 * and a blank value will be ignored.
+	 * <p>Note: the tag will first be {@linkplain String#trim() trimmed}. If the
+	 * supplied tag is syntactically invalid after trimming, the error will be
+	 * logged as a warning, and the invalid tag will be effectively ignored. See
+	 * {@linkplain #Tag Syntax Rules for Tags}.
 	 */
 	String value();
 

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/JupiterTestDescriptor.java

@@ -64,7 +64,20 @@ protected static Set<TestTag> getTags(AnnotatedElement element) {
 		// @formatter:off
 		return findRepeatableAnnotations(element, Tag.class).stream()
 				.map(Tag::value)
-				.filter(StringUtils::isNotBlank)
+				.filter(tag -> {
+					boolean isValid = TestTag.isValid(tag);
+					if (!isValid) {
+						// TODO [#242] Replace logging with precondition check once we have a proper mechanism for
+						// handling validation exceptions during the TestEngine discovery phase.
+						//
+						// As an alternative to a precondition check here, we could catch any
+						// PreconditionViolationException thrown by TestTag::create.
+						logger.warning(String.format(
+							"Configuration error: invalid tag syntax in @Tag(\"%s\") declaration on [%s]. Tag will be ignored.",
+							tag, element));
+					}
+					return isValid;
+				})
 				.map(TestTag::create)
 				.collect(toCollection(LinkedHashSet::new));
 		// @formatter:on

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/descriptor/JupiterTestDescriptorTests.java

@@ -88,7 +88,7 @@ void constructFromMethodWithCustomTestAnnotation() throws Exception {
 
 		assertEquals(testMethod, descriptor.getTestMethod());
 		assertEquals("custom name", descriptor.getDisplayName(), "display name:");
-		assertThat(descriptor.getTags()).containsExactly(TestTag.create("custom tag"));
+		assertThat(descriptor.getTags()).containsExactly(TestTag.create("custom-tag"));
 	}
 
 	@Test
@@ -181,6 +181,7 @@ void test(String[][][][][] info) {
 		@DisplayName("custom test name")
 		@Tag("methodTag1")
 		@Tag("methodTag2")
+		@Tag("tag containing whitespace")
 		void foo() {
 		}
 
@@ -192,7 +193,7 @@ void customTestAnnotation() {
 
 	@Test
 	@DisplayName("custom name")
-	@Tag("custom tag")
+	@Tag("  custom-tag  ")
 	@Target(ElementType.METHOD)
 	@Retention(RetentionPolicy.RUNTIME)
 	@interface CustomTestAnnotation {

junit-platform-engine/src/main/java/org/junit/platform/engine/TestTag.java

@@ -16,13 +16,17 @@
 import java.util.Objects;
 
 import org.junit.platform.commons.meta.API;
+import org.junit.platform.commons.util.PreconditionViolationException;
 import org.junit.platform.commons.util.Preconditions;
+import org.junit.platform.commons.util.StringUtils;
 
 /**
  * Immutable value object for a <em>tag</em> that is assigned to a test or
  * container.
  *
  * @since 1.0
+ * @see #isValid(String)
+ * @see #create(String)
  */
 @API(Experimental)
 public final class TestTag implements Serializable {
@@ -31,24 +35,70 @@ public final class TestTag implements Serializable {
 
 	private final String name;
 
+	/**
+	 * Determine if the supplied tag name is valid with regard to the supported
+	 * syntax for tags.
+	 *
+	 * <h3>Syntax Rules for Tags</h3>
+	 * <ul>
+	 * <li>A tag must not be {@code null}.</li>
+	 * <li>A tag must not be blank.</li>
+	 * <li>A trimmed tag must not contain whitespace.</li>
+	 * <li>A trimmed tag must not contain ISO control characters.</li>
+	 * </ul>
+	 *
+	 * <p>If this method returns {@code true} for a given name, it is then a
+	 * valid candidate for the {@link TestTag#create(String) create()} factory
+	 * method.
+	 *
+	 * @param name the name of the tag to validate; may be {@code null} or blank
+	 * @return {@code true} if the supplied tag name conforms to the supported
+	 * syntax for tags
+	 * @see StringUtils#isNotBlank(String)
+	 * @see String#trim()
+	 * @see StringUtils#doesNotContainWhitespace(String)
+	 * @see StringUtils#doesNotContainIsoControlCharacter(String)
+	 * @see TestTag#create(String)
+	 */
+	public static boolean isValid(String name) {
+		if (name == null) {
+			return false;
+		}
+		name = name.trim();
+
+		return !name.isEmpty() && //
+				StringUtils.doesNotContainWhitespace(name) && //
+				StringUtils.doesNotContainIsoControlCharacter(name);
+	}
+
 	/**
 	 * Create a {@code TestTag} from the supplied {@code name}.
 	 *
+	 * <p>Consider checking whether the syntax of the supplied {@code name}
+	 * is {@linkplain #isValid(String) valid} before attempting to create a
+	 * {@code TestTag} using this factory method.
+	 *
 	 * <p>Note: the supplied {@code name} will be {@linkplain String#trim() trimmed}.
 	 *
-	 * @param name the name of the tag; must not be {@code null} or blank
+	 * @param name the name of the tag; must be syntactically <em>valid</em>
+	 * @throws PreconditionViolationException if the supplied tag name is not
+	 * syntactically <em>valid</em>
+	 * @see TestTag#isValid(String)
 	 */
-	public static TestTag create(String name) {
+	public static TestTag create(String name) throws PreconditionViolationException {
 		return new TestTag(name);
 	}
 
 	private TestTag(String name) {
-		Preconditions.notBlank(name, "name must not be null or blank");
+		Preconditions.condition(TestTag.isValid(name),
+			() -> String.format("Tag name [%s] must be syntactically valid", name));
 		this.name = name.trim();
 	}
 
 	/**
 	 * Get the name of this tag.
+	 *
+	 * @return the name of this tag; never {@code null} or blank
 	 */
 	public String getName() {
 		return this.name;

junit-platform-launcher/src/main/java/org/junit/platform/launcher/TagFilter.java

@@ -15,13 +15,11 @@
 import static org.junit.platform.commons.meta.API.Usage.Experimental;
 
 import java.util.List;
-import java.util.stream.Stream;
 
 import org.junit.platform.commons.meta.API;
+import org.junit.platform.commons.util.PreconditionViolationException;
 import org.junit.platform.commons.util.Preconditions;
-import org.junit.platform.commons.util.StringUtils;
 import org.junit.platform.engine.FilterResult;
-import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.TestTag;
 
 /**
@@ -44,91 +42,87 @@ private TagFilter() {
 	/**
 	 * Create an <em>include</em> filter based on the supplied {@code tags}.
 	 *
-	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}, and any
-	 * blank tag will be ignored.
+	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}.
 	 *
 	 * <p>Containers and tests will only be executed if they are tagged with
 	 * at least one of the supplied <em>included</em> tags.
 	 *
 	 * @param tags the included tags; never {@code null} or empty
+	 * @throws PreconditionViolationException if the supplied tags array is
+	 * {@code null} or empty, or if any individual tag is not syntactically
+	 * valid
 	 * @see #includeTags(List)
+	 * @see TestTag#isValid(String)
 	 */
-	public static PostDiscoveryFilter includeTags(String... tags) {
+	public static PostDiscoveryFilter includeTags(String... tags) throws PreconditionViolationException {
 		Preconditions.notNull(tags, "tags array must not be null");
 		return includeTags(asList(tags));
 	}
 
 	/**
 	 * Create an <em>include</em> filter based on the supplied {@code tags}.
 	 *
-	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}, and any
-	 * blank tag will be ignored.
+	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}.
 	 *
 	 * <p>Containers and tests will only be executed if they are tagged with
 	 * at least one of the supplied <em>included</em> tags.
 	 *
 	 * @param tags the included tags; never {@code null} or empty
+	 * @throws PreconditionViolationException if the supplied tags list is
+	 * {@code null} or empty, or if any individual tag is not syntactically
+	 * valid
 	 * @see #includeTags(String...)
+	 * @see TestTag#isValid(String)
 	 */
-	public static PostDiscoveryFilter includeTags(List<String> tags) {
+	public static PostDiscoveryFilter includeTags(List<String> tags) throws PreconditionViolationException {
 		Preconditions.notEmpty(tags, "tags list must not be null or empty");
-		Preconditions.containsNoNullElements(tags, "individual tags must not be null");
-		List<String> trimmedTags = trimmedCopyOf(tags);
-		return descriptor -> FilterResult.includedIf(trimmedTagsOf(descriptor).anyMatch(trimmedTags::contains));
+		List<TestTag> activeTags = toTestTags(tags);
+		return descriptor -> FilterResult.includedIf(descriptor.getTags().stream().anyMatch(activeTags::contains));
 	}
 
 	/**
 	 * Create an <em>exclude</em> filter based on the supplied {@code tags}.
 	 *
-	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}, and any
-	 * blank tag will be ignored.
+	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}.
 	 *
 	 * <p>Containers and tests will only be executed if they are <em>not</em>
 	 * tagged with any of the supplied <em>excluded</em> tags.
 	 *
 	 * @param tags the excluded tags; never {@code null} or empty
+	 * @throws PreconditionViolationException if the supplied tags array is
+	 * {@code null} or empty, or if any individual tag is not syntactically
+	 * valid
 	 * @see #excludeTags(List)
+	 * @see TestTag#isValid(String)
 	 */
-	public static PostDiscoveryFilter excludeTags(String... tags) {
+	public static PostDiscoveryFilter excludeTags(String... tags) throws PreconditionViolationException {
 		Preconditions.notNull(tags, "tags array must not be null");
 		return excludeTags(asList(tags));
 	}
 
 	/**
 	 * Create an <em>exclude</em> filter based on the supplied {@code tags}.
 	 *
-	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}, and any
-	 * blank tag will be ignored.
+	 * <p>Note: each tag will be {@linkplain String#trim() trimmed}.
 	 *
 	 * <p>Containers and tests will only be executed if they are <em>not</em>
 	 * tagged with any of the supplied <em>excluded</em> tags.
 	 *
 	 * @param tags the excluded tags; never {@code null} or empty
+	 * @throws PreconditionViolationException if the supplied tags list is
+	 * {@code null} or empty, or if any individual tag is not syntactically
+	 * valid
 	 * @see #excludeTags(String...)
+	 * @see TestTag#isValid(String)
 	 */
-	public static PostDiscoveryFilter excludeTags(List<String> tags) {
+	public static PostDiscoveryFilter excludeTags(List<String> tags) throws PreconditionViolationException {
 		Preconditions.notEmpty(tags, "tags list must not be null or empty");
-		Preconditions.containsNoNullElements(tags, "individual tags must not be null");
-		List<String> trimmedTags = trimmedCopyOf(tags);
-		return descriptor -> FilterResult.includedIf(trimmedTagsOf(descriptor).noneMatch(trimmedTags::contains));
+		List<TestTag> activeTags = toTestTags(tags);
+		return descriptor -> FilterResult.includedIf(descriptor.getTags().stream().noneMatch(activeTags::contains));
 	}
 
-	private static List<String> trimmedCopyOf(List<String> tags) {
-		// @formatter:off
-		return tags.stream()
-				.filter(StringUtils::isNotBlank)
-				.map(String::trim)
-				.collect(toList());
-		// @formatter:on
-	}
-
-	private static Stream<String> trimmedTagsOf(TestDescriptor descriptor) {
-		// @formatter:off
-		return descriptor.getTags().stream()
-				.map(TestTag::getName)
-				.filter(StringUtils::isNotBlank)
-				.map(String::trim);
-		// @formatter:on
+	private static List<TestTag> toTestTags(List<String> tags) {
+		return tags.stream().map(TestTag::create).collect(toList());
 	}
 
 }