Report discovery issues for invalid tag syntax on class level

Author: marcphilipp

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

@@ -136,18 +136,26 @@ public final String getLegacyReportingName() {
 	public final void validate(DiscoveryIssueReporter reporter) {
 		validateCoreLifecycleMethods(reporter);
 		validateClassTemplateInvocationLifecycleMethods(reporter);
+		validateTags(reporter);
 	}
 
 	protected void validateCoreLifecycleMethods(DiscoveryIssueReporter reporter) {
-		List<DiscoveryIssue> discoveryIssues = this.lifecycleMethods.discoveryIssues;
-		discoveryIssues.forEach(reporter::reportIssue);
-		discoveryIssues.clear();
+		reportAndClear(this.lifecycleMethods.discoveryIssues, reporter);
 	}
 
 	protected void validateClassTemplateInvocationLifecycleMethods(DiscoveryIssueReporter reporter) {
 		LifecycleMethodUtils.validateNoClassTemplateInvocationLifecycleMethodsAreDeclared(getTestClass(), reporter);
 	}
 
+	private void validateTags(DiscoveryIssueReporter reporter) {
+		reportAndClear(this.classInfo.discoveryIssues, reporter);
+	}
+
+	private static void reportAndClear(List<DiscoveryIssue> discoveryIssues, DiscoveryIssueReporter reporter) {
+		discoveryIssues.forEach(reporter::reportIssue);
+		discoveryIssues.clear();
+	}
+
 	// --- Node ----------------------------------------------------------------
 
 	@Override
@@ -539,6 +547,8 @@ private void invokeMethodInExtensionContext(Method method, ExtensionContext cont
 
 	protected static class ClassInfo {
 
+		private final List<DiscoveryIssue> discoveryIssues = new ArrayList<>();
+
 		final Class<?> testClass;
 		final Set<TestTag> tags;
 		final Lifecycle lifecycle;
@@ -547,7 +557,12 @@ protected static class ClassInfo {
 
 		ClassInfo(Class<?> testClass, JupiterConfiguration configuration) {
 			this.testClass = testClass;
-			this.tags = getTags(testClass);
+			List<DiscoveryIssue.Builder> tagIssues = new ArrayList<>();
+			this.tags = getTags(testClass, tagIssues::add);
+			if (!tagIssues.isEmpty()) {
+				ClassSource source = ClassSource.from(testClass);
+				tagIssues.forEach(builder -> discoveryIssues.add(builder.source(source).build()));
+			}
 			this.lifecycle = getTestInstanceLifecycle(testClass, configuration);
 			this.defaultChildExecutionMode = (this.lifecycle == Lifecycle.PER_CLASS ? ExecutionMode.SAME_THREAD : null);
 			this.exclusiveResourceCollector = ExclusiveResourceCollector.from(testClass);

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

@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.function.UnaryOperator;
 
@@ -39,10 +40,10 @@
 import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext;
 import org.junit.jupiter.engine.extension.ExtensionRegistry;
 import org.junit.platform.commons.JUnitException;
-import org.junit.platform.commons.logging.Logger;
-import org.junit.platform.commons.logging.LoggerFactory;
 import org.junit.platform.commons.util.ExceptionUtils;
 import org.junit.platform.commons.util.UnrecoverableExceptions;
+import org.junit.platform.engine.DiscoveryIssue;
+import org.junit.platform.engine.DiscoveryIssue.Severity;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.TestSource;
 import org.junit.platform.engine.TestTag;
@@ -58,8 +59,6 @@
 public abstract class JupiterTestDescriptor extends AbstractTestDescriptor
 		implements Node<JupiterEngineExecutionContext> {
 
-	private static final Logger logger = LoggerFactory.getLogger(JupiterTestDescriptor.class);
-
 	private static final ConditionEvaluator conditionEvaluator = new ConditionEvaluator();
 
 	final JupiterConfiguration configuration;
@@ -77,21 +76,15 @@ public abstract class JupiterTestDescriptor extends AbstractTestDescriptor
 
 	// --- TestDescriptor ------------------------------------------------------
 
-	static Set<TestTag> getTags(AnnotatedElement element) {
+	static Set<TestTag> getTags(AnnotatedElement element, Consumer<DiscoveryIssue.Builder> invalidTagHandler) {
 		// @formatter:off
 		return findRepeatableAnnotations(element, Tag.class).stream()
 				.map(Tag::value)
 				.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.warn(() -> String.format(
-							"Configuration error: invalid tag syntax in @Tag(\"%s\") declaration on [%s]. Tag will be ignored.",
-							tag, element));
+						String message = String.format("Invalid tag syntax in @Tag(\"%s\") declaration on [%s]. Tag will be ignored.", tag, element);
+						invalidTagHandler.accept(DiscoveryIssue.builder(Severity.WARNING, message));
 					}
 					return isValid;
 				})

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

@@ -75,7 +75,8 @@ public abstract class MethodBasedTestDescriptor extends JupiterTestDescriptor
 
 		this.testClass = Preconditions.notNull(testClass, "Class must not be null");
 		this.testMethod = testMethod;
-		this.tags = getTags(testMethod);
+		this.tags = getTags(testMethod,
+			builder -> logger.warn(() -> "Configuration error: " + builder.build().message()));
 	}
 
 	@Override

jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoveryTests.java

@@ -35,6 +35,7 @@
 
 import org.junit.jupiter.api.Named;
 import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.TestTemplate;
@@ -48,6 +49,7 @@
 import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.platform.engine.DiscoveryIssue;
 import org.junit.platform.engine.TestDescriptor;
+import org.junit.platform.engine.support.descriptor.ClassSource;
 import org.junit.platform.launcher.LauncherDiscoveryRequest;
 
 /**
@@ -284,6 +286,20 @@ void reportsWarningForTestClassWithPotentialNestedTestClasses() {
 					InvalidTestCases.InvalidTestClassTestCase.class.getName());
 	}
 
+	@Test
+	void reportsWarningsForInvalidTags() {
+
+		var results = discoverTestsForClass(InvalidTagsTestCase.class);
+
+		var discoveryIssues = results.getDiscoveryIssues();
+		assertThat(discoveryIssues).hasSize(1);
+		assertThat(discoveryIssues.getFirst().message()) //
+				.isEqualTo("Invalid tag syntax in @Tag(\"\") declaration on [class %s]. Tag will be ignored.",
+					InvalidTagsTestCase.class.getName());
+		assertThat(discoveryIssues.getFirst().source()) //
+				.contains(ClassSource.from(InvalidTagsTestCase.class));
+	}
+
 	// -------------------------------------------------------------------
 
 	@SuppressWarnings("unused")
@@ -393,4 +409,12 @@ private class InvalidTestClassSubclassTestCase extends InvalidTestClassTestCase
 
 	}
 
+	@SuppressWarnings("JUnitMalformedDeclaration")
+	@Tag("")
+	static class InvalidTagsTestCase {
+		@Test
+		void test() {
+		}
+	}
+
 }