Skip to content

Commit 4696b7f

Browse files
marcphilippmarcphilipp
committed
Report discovery issues for invalid tag syntax on method level
1 parent aebce57 commit 4696b7f

File tree

6 files changed

+114
-62
lines changed

6 files changed

+114
-62
lines changed

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

+6-13
Original file line numberDiff line numberDiff line change
@@ -140,20 +140,15 @@ public final void validate(DiscoveryIssueReporter reporter) {
140140
}
141141

142142
protected void validateCoreLifecycleMethods(DiscoveryIssueReporter reporter) {
143-
reportAndClear(this.lifecycleMethods.discoveryIssues, reporter);
143+
Validatable.reportAndClear(this.lifecycleMethods.discoveryIssues, reporter);
144144
}
145145

146146
protected void validateClassTemplateInvocationLifecycleMethods(DiscoveryIssueReporter reporter) {
147147
LifecycleMethodUtils.validateNoClassTemplateInvocationLifecycleMethodsAreDeclared(getTestClass(), reporter);
148148
}
149149

150150
private void validateTags(DiscoveryIssueReporter reporter) {
151-
reportAndClear(this.classInfo.discoveryIssues, reporter);
152-
}
153-
154-
private static void reportAndClear(List<DiscoveryIssue> discoveryIssues, DiscoveryIssueReporter reporter) {
155-
discoveryIssues.forEach(reporter::reportIssue);
156-
discoveryIssues.clear();
151+
Validatable.reportAndClear(this.classInfo.discoveryIssues, reporter);
157152
}
158153

159154
// --- Node ----------------------------------------------------------------
@@ -557,12 +552,10 @@ protected static class ClassInfo {
557552

558553
ClassInfo(Class<?> testClass, JupiterConfiguration configuration) {
559554
this.testClass = testClass;
560-
List<DiscoveryIssue.Builder> tagIssues = new ArrayList<>();
561-
this.tags = getTags(testClass, tagIssues::add);
562-
if (!tagIssues.isEmpty()) {
563-
ClassSource source = ClassSource.from(testClass);
564-
tagIssues.forEach(builder -> discoveryIssues.add(builder.source(source).build()));
565-
}
555+
this.tags = getTags(testClass, //
556+
() -> String.format("class '%s'", testClass.getName()), //
557+
() -> ClassSource.from(testClass), //
558+
discoveryIssues::add);
566559
this.lifecycle = getTestInstanceLifecycle(testClass, configuration);
567560
this.defaultChildExecutionMode = (this.lifecycle == Lifecycle.PER_CLASS ? ExecutionMode.SAME_THREAD : null);
568561
this.exclusiveResourceCollector = ExclusiveResourceCollector.from(testClass);

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

+16-9
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.util.List;
2626
import java.util.Optional;
2727
import java.util.Set;
28+
import java.util.concurrent.atomic.AtomicReference;
2829
import java.util.function.Consumer;
2930
import java.util.function.Supplier;
3031
import java.util.function.UnaryOperator;
@@ -76,21 +77,27 @@ public abstract class JupiterTestDescriptor extends AbstractTestDescriptor
7677

7778
// --- TestDescriptor ------------------------------------------------------
7879

79-
static Set<TestTag> getTags(AnnotatedElement element, Consumer<DiscoveryIssue.Builder> invalidTagHandler) {
80-
// @formatter:off
81-
return findRepeatableAnnotations(element, Tag.class).stream()
82-
.map(Tag::value)
80+
static Set<TestTag> getTags(AnnotatedElement element, Supplier<String> elementDescription,
81+
Supplier<TestSource> sourceProvider, Consumer<DiscoveryIssue> issueCollector) {
82+
AtomicReference<TestSource> source = new AtomicReference<>();
83+
return findRepeatableAnnotations(element, Tag.class).stream() //
84+
.map(Tag::value) //
8385
.filter(tag -> {
8486
boolean isValid = TestTag.isValid(tag);
8587
if (!isValid) {
86-
String message = String.format("Invalid tag syntax in @Tag(\"%s\") declaration on [%s]. Tag will be ignored.", tag, element);
87-
invalidTagHandler.accept(DiscoveryIssue.builder(Severity.WARNING, message));
88+
String message = String.format(
89+
"Invalid tag syntax in @Tag(\"%s\") declaration on %s. Tag will be ignored.", tag,
90+
elementDescription.get());
91+
if (source.get() == null) {
92+
source.set(sourceProvider.get());
93+
}
94+
issueCollector.accept(
95+
DiscoveryIssue.builder(Severity.WARNING, message).source(source.get()).build());
8896
}
8997
return isValid;
90-
})
91-
.map(TestTag::create)
98+
}) //
99+
.map(TestTag::create) //
92100
.collect(collectingAndThen(toCollection(LinkedHashSet::new), Collections::unmodifiableSet));
93-
// @formatter:on
94101
}
95102

96103
/**

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

+66-36
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import static org.junit.platform.commons.util.CollectionUtils.forEachInReverseOrder;
1818

1919
import java.lang.reflect.Method;
20+
import java.util.ArrayList;
2021
import java.util.Collections;
2122
import java.util.LinkedHashSet;
2223
import java.util.List;
@@ -39,10 +40,12 @@
3940
import org.junit.platform.commons.util.Preconditions;
4041
import org.junit.platform.commons.util.ReflectionUtils;
4142
import org.junit.platform.commons.util.UnrecoverableExceptions;
43+
import org.junit.platform.engine.DiscoveryIssue;
4244
import org.junit.platform.engine.TestDescriptor;
4345
import org.junit.platform.engine.TestTag;
4446
import org.junit.platform.engine.UniqueId;
4547
import org.junit.platform.engine.support.descriptor.MethodSource;
48+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
4649

4750
/**
4851
* Base class for {@link TestDescriptor TestDescriptors} based on Java methods.
@@ -51,17 +54,11 @@
5154
*/
5255
@API(status = INTERNAL, since = "5.0")
5356
public abstract class MethodBasedTestDescriptor extends JupiterTestDescriptor
54-
implements ResourceLockAware, TestClassAware {
57+
implements ResourceLockAware, TestClassAware, Validatable {
5558

5659
private static final Logger logger = LoggerFactory.getLogger(MethodBasedTestDescriptor.class);
5760

58-
private final Class<?> testClass;
59-
private final Method testMethod;
60-
61-
/**
62-
* Set of method-level tags; does not contain tags from parent.
63-
*/
64-
private final Set<TestTag> tags;
61+
private final MethodInfo methodInfo;
6562

6663
MethodBasedTestDescriptor(UniqueId uniqueId, Class<?> testClass, Method testMethod,
6764
Supplier<List<Class<?>>> enclosingInstanceTypes, JupiterConfiguration configuration) {
@@ -72,21 +69,54 @@ public abstract class MethodBasedTestDescriptor extends JupiterTestDescriptor
7269
MethodBasedTestDescriptor(UniqueId uniqueId, String displayName, Class<?> testClass, Method testMethod,
7370
JupiterConfiguration configuration) {
7471
super(uniqueId, displayName, MethodSource.from(testClass, testMethod), configuration);
72+
this.methodInfo = new MethodInfo(testClass, testMethod);
73+
}
7574

76-
this.testClass = Preconditions.notNull(testClass, "Class must not be null");
77-
this.testMethod = testMethod;
78-
this.tags = getTags(testMethod,
79-
builder -> logger.warn(() -> "Configuration error: " + builder.build().message()));
75+
public final Method getTestMethod() {
76+
return this.methodInfo.testMethod;
8077
}
8178

79+
// --- TestDescriptor ------------------------------------------------------
80+
8281
@Override
8382
public final Set<TestTag> getTags() {
8483
// return modifiable copy
85-
Set<TestTag> allTags = new LinkedHashSet<>(this.tags);
84+
Set<TestTag> allTags = new LinkedHashSet<>(this.methodInfo.tags);
8685
getParent().ifPresent(parentDescriptor -> allTags.addAll(parentDescriptor.getTags()));
8786
return allTags;
8887
}
8988

89+
@Override
90+
public String getLegacyReportingName() {
91+
return String.format("%s(%s)", getTestMethod().getName(),
92+
ClassUtils.nullSafeToString(Class::getSimpleName, getTestMethod().getParameterTypes()));
93+
}
94+
95+
// --- TestClassAware ------------------------------------------------------
96+
97+
@Override
98+
public final Class<?> getTestClass() {
99+
return this.methodInfo.testClass;
100+
}
101+
102+
@Override
103+
public List<Class<?>> getEnclosingTestClasses() {
104+
return getParent() //
105+
.filter(TestClassAware.class::isInstance) //
106+
.map(TestClassAware.class::cast) //
107+
.map(TestClassAware::getEnclosingTestClasses) //
108+
.orElseGet(Collections::emptyList);
109+
}
110+
111+
// --- Validatable ---------------------------------------------------------
112+
113+
@Override
114+
public void validate(DiscoveryIssueReporter reporter) {
115+
Validatable.reportAndClear(this.methodInfo.discoveryIssues, reporter);
116+
}
117+
118+
// --- Node ----------------------------------------------------------------
119+
90120
@Override
91121
public ExclusiveResourceCollector getExclusiveResourceCollector() {
92122
// There's no need to cache this as this method should only be called once
@@ -108,34 +138,11 @@ public Function<ResourceLocksProvider, Set<ResourceLocksProvider.Lock>> getResou
108138
getTestMethod()));
109139
}
110140

111-
@Override
112-
public List<Class<?>> getEnclosingTestClasses() {
113-
return getParent() //
114-
.filter(TestClassAware.class::isInstance) //
115-
.map(TestClassAware.class::cast) //
116-
.map(TestClassAware::getEnclosingTestClasses) //
117-
.orElseGet(Collections::emptyList);
118-
}
119-
120141
@Override
121142
protected Optional<ExecutionMode> getExplicitExecutionMode() {
122143
return getExecutionModeFromAnnotation(getTestMethod());
123144
}
124145

125-
public final Class<?> getTestClass() {
126-
return this.testClass;
127-
}
128-
129-
public final Method getTestMethod() {
130-
return this.testMethod;
131-
}
132-
133-
@Override
134-
public String getLegacyReportingName() {
135-
return String.format("%s(%s)", testMethod.getName(),
136-
ClassUtils.nullSafeToString(Class::getSimpleName, testMethod.getParameterTypes()));
137-
}
138-
139146
/**
140147
* Invoke {@link TestWatcher#testDisabled(ExtensionContext, Optional)} on each
141148
* registered {@link TestWatcher}, in registration order.
@@ -181,4 +188,27 @@ protected void invokeTestWatchers(JupiterEngineExecutionContext context, boolean
181188
}
182189
}
183190

191+
private static class MethodInfo {
192+
193+
private final List<DiscoveryIssue> discoveryIssues = new ArrayList<>();
194+
195+
private final Class<?> testClass;
196+
private final Method testMethod;
197+
198+
/**
199+
* Set of method-level tags; does not contain tags from parent.
200+
*/
201+
private final Set<TestTag> tags;
202+
203+
MethodInfo(Class<?> testClass, Method testMethod) {
204+
this.testClass = Preconditions.notNull(testClass, "Class must not be null");
205+
this.testMethod = testMethod;
206+
this.tags = getTags(testMethod, //
207+
() -> String.format("method '%s'", testMethod.toGenericString()), //
208+
// Use _declaring_ class here because that's where the `@Tag` annotation is declared
209+
() -> MethodSource.from(testMethod.getDeclaringClass(), testMethod), //
210+
discoveryIssues::add);
211+
}
212+
}
213+
184214
}

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/Validatable.java

+12
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@
1212

1313
import static org.apiguardian.api.API.Status.INTERNAL;
1414

15+
import java.util.List;
16+
1517
import org.apiguardian.api.API;
18+
import org.junit.platform.engine.DiscoveryIssue;
1619
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
1720

1821
/**
@@ -29,4 +32,13 @@ public interface Validatable {
2932
*/
3033
void validate(DiscoveryIssueReporter reporter);
3134

35+
/**
36+
* Report and clear the given list of {@link DiscoveryIssue}s using the
37+
* supplied {@link DiscoveryIssueReporter}.
38+
*/
39+
static void reportAndClear(List<DiscoveryIssue> issues, DiscoveryIssueReporter reporter) {
40+
issues.forEach(reporter::reportIssue);
41+
issues.clear();
42+
}
43+
3244
}

‎junit-jupiter-engine/src/nativeImage/initialize-at-build-time

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor
1616
org.junit.jupiter.engine.descriptor.JupiterTestDescriptor
1717
org.junit.jupiter.engine.descriptor.JupiterTestDescriptor$1
1818
org.junit.jupiter.engine.descriptor.MethodBasedTestDescriptor
19+
org.junit.jupiter.engine.descriptor.MethodBasedTestDescriptor$MethodInfo
1920
org.junit.jupiter.engine.descriptor.NestedClassTestDescriptor
2021
org.junit.jupiter.engine.descriptor.TestFactoryTestDescriptor
2122
org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor

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

+13-4
Original file line numberDiff line numberDiff line change
@@ -287,17 +287,25 @@ void reportsWarningForTestClassWithPotentialNestedTestClasses() {
287287
}
288288

289289
@Test
290-
void reportsWarningsForInvalidTags() {
290+
void reportsWarningsForInvalidTags() throws NoSuchMethodException {
291291

292292
var results = discoverTestsForClass(InvalidTagsTestCase.class);
293293

294-
var discoveryIssues = results.getDiscoveryIssues();
295-
assertThat(discoveryIssues).hasSize(1);
294+
var discoveryIssues = results.getDiscoveryIssues().stream().sorted(comparing(DiscoveryIssue::message)).toList();
295+
assertThat(discoveryIssues).hasSize(2);
296+
296297
assertThat(discoveryIssues.getFirst().message()) //
297-
.isEqualTo("Invalid tag syntax in @Tag(\"\") declaration on [class %s]. Tag will be ignored.",
298+
.isEqualTo("Invalid tag syntax in @Tag(\"\") declaration on class '%s'. Tag will be ignored.",
298299
InvalidTagsTestCase.class.getName());
299300
assertThat(discoveryIssues.getFirst().source()) //
300301
.contains(ClassSource.from(InvalidTagsTestCase.class));
302+
303+
var method = InvalidTagsTestCase.class.getDeclaredMethod("test");
304+
assertThat(discoveryIssues.getLast().message()) //
305+
.isEqualTo("Invalid tag syntax in @Tag(\"|\") declaration on method '%s'. Tag will be ignored.",
306+
method.toGenericString());
307+
assertThat(discoveryIssues.getLast().source()) //
308+
.contains(org.junit.platform.engine.support.descriptor.MethodSource.from(method));
301309
}
302310

303311
// -------------------------------------------------------------------
@@ -413,6 +421,7 @@ private class InvalidTestClassSubclassTestCase extends InvalidTestClassTestCase
413421
@Tag("")
414422
static class InvalidTagsTestCase {
415423
@Test
424+
@Tag("|")
416425
void test() {
417426
}
418427
}

0 commit comments

Comments
 (0)