Skip to content

Commit 786de29

Browse files
marcphilippmarcphilipp
committed
Report discovery issues for ineffective @Order annotations
1 parent f0ac6e2 commit 786de29

File tree

6 files changed

+258
-78
lines changed

6 files changed

+258
-78
lines changed

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java

+25-11
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import java.util.HashMap;
1919
import java.util.List;
2020
import java.util.Map;
21+
import java.util.Optional;
2122
import java.util.function.Consumer;
2223
import java.util.function.Function;
2324
import java.util.stream.Stream;
@@ -61,17 +62,24 @@ protected <PARENT extends TestDescriptor> void doWithMatchingDescriptor(Class<PA
6162
*/
6263
protected <CHILD extends TestDescriptor, WRAPPER extends AbstractAnnotatedDescriptorWrapper<?>> void orderChildrenTestDescriptors(
6364
TestDescriptor parentTestDescriptor, Class<CHILD> matchingChildrenType,
64-
Function<CHILD, WRAPPER> descriptorWrapperFactory,
65-
DescriptorWrapperOrderer<WRAPPER> descriptorWrapperOrderer) {
65+
Optional<Consumer<CHILD>> validationAction, Function<CHILD, WRAPPER> descriptorWrapperFactory,
66+
DescriptorWrapperOrderer<?, WRAPPER> descriptorWrapperOrderer) {
67+
68+
Stream<CHILD> matchingChildren = parentTestDescriptor.getChildren()//
69+
.stream()//
70+
.filter(matchingChildrenType::isInstance)//
71+
.map(matchingChildrenType::cast);
6672

6773
if (!descriptorWrapperOrderer.canOrderWrappers()) {
74+
validationAction.ifPresent(matchingChildren::forEach);
6875
return;
6976
}
7077

71-
List<WRAPPER> matchingDescriptorWrappers = parentTestDescriptor.getChildren()//
72-
.stream()//
73-
.filter(matchingChildrenType::isInstance)//
74-
.map(matchingChildrenType::cast)//
78+
if (validationAction.isPresent()) {
79+
matchingChildren = matchingChildren.peek(validationAction.get());
80+
}
81+
82+
List<WRAPPER> matchingDescriptorWrappers = matchingChildren//
7583
.map(descriptorWrapperFactory)//
7684
.collect(toCollection(ArrayList::new));
7785

@@ -105,29 +113,35 @@ protected <CHILD extends TestDescriptor, WRAPPER extends AbstractAnnotatedDescri
105113
/**
106114
* @param <WRAPPER> the wrapper type for the children to order
107115
*/
108-
protected static class DescriptorWrapperOrderer<WRAPPER> {
116+
protected static class DescriptorWrapperOrderer<ORDERER, WRAPPER> {
109117

110-
private static final DescriptorWrapperOrderer<?> NOOP = new DescriptorWrapperOrderer<>(null, __ -> "",
118+
private static final DescriptorWrapperOrderer<?, ?> NOOP = new DescriptorWrapperOrderer<>(null, null, __ -> "",
111119
___ -> "");
112120

113121
@SuppressWarnings("unchecked")
114-
protected static <WRAPPER> DescriptorWrapperOrderer<WRAPPER> noop() {
115-
return (DescriptorWrapperOrderer<WRAPPER>) NOOP;
122+
protected static <ORDERER, WRAPPER> DescriptorWrapperOrderer<ORDERER, WRAPPER> noop() {
123+
return (DescriptorWrapperOrderer<ORDERER, WRAPPER>) NOOP;
116124
}
117125

126+
private final ORDERER orderer;
118127
private final Consumer<List<WRAPPER>> orderingAction;
119128
private final MessageGenerator descriptorsAddedMessageGenerator;
120129
private final MessageGenerator descriptorsRemovedMessageGenerator;
121130

122-
DescriptorWrapperOrderer(Consumer<List<WRAPPER>> orderingAction,
131+
DescriptorWrapperOrderer(ORDERER orderer, Consumer<List<WRAPPER>> orderingAction,
123132
MessageGenerator descriptorsAddedMessageGenerator,
124133
MessageGenerator descriptorsRemovedMessageGenerator) {
125134

135+
this.orderer = orderer;
126136
this.orderingAction = orderingAction;
127137
this.descriptorsAddedMessageGenerator = descriptorsAddedMessageGenerator;
128138
this.descriptorsRemovedMessageGenerator = descriptorsRemovedMessageGenerator;
129139
}
130140

141+
ORDERER getOrderer() {
142+
return orderer;
143+
}
144+
131145
private boolean canOrderWrappers() {
132146
return this.orderingAction != null;
133147
}

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassOrderingVisitor.java

+46-11
Original file line numberDiff line numberDiff line change
@@ -10,32 +10,51 @@
1010

1111
package org.junit.jupiter.engine.discovery;
1212

13+
import static org.junit.platform.commons.support.AnnotationSupport.isAnnotated;
14+
1315
import java.util.List;
16+
import java.util.Optional;
1417
import java.util.function.Consumer;
1518

1619
import org.junit.jupiter.api.ClassOrderer;
20+
import org.junit.jupiter.api.Order;
1721
import org.junit.jupiter.api.TestClassOrder;
1822
import org.junit.jupiter.engine.config.JupiterConfiguration;
1923
import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor;
2024
import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor;
2125
import org.junit.platform.commons.support.AnnotationSupport;
2226
import org.junit.platform.commons.support.ReflectionSupport;
2327
import org.junit.platform.commons.util.LruCache;
28+
import org.junit.platform.engine.DiscoveryIssue;
29+
import org.junit.platform.engine.DiscoveryIssue.Severity;
2430
import org.junit.platform.engine.TestDescriptor;
31+
import org.junit.platform.engine.support.descriptor.ClassSource;
32+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
33+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter.Condition;
2534

2635
/**
2736
* @since 5.8
2837
*/
2938
class ClassOrderingVisitor extends AbstractOrderingVisitor {
3039

31-
private final LruCache<ClassBasedTestDescriptor, DescriptorWrapperOrderer<DefaultClassDescriptor>> ordererCache = new LruCache<>(
40+
private final LruCache<ClassBasedTestDescriptor, DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor>> ordererCache = new LruCache<>(
3241
10);
3342
private final JupiterConfiguration configuration;
34-
private final DescriptorWrapperOrderer<DefaultClassDescriptor> globalOrderer;
43+
private final DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> globalOrderer;
44+
private final Condition<ClassBasedTestDescriptor> noOrderAnnotation;
3545

36-
ClassOrderingVisitor(JupiterConfiguration configuration) {
46+
ClassOrderingVisitor(JupiterConfiguration configuration, DiscoveryIssueReporter issueReporter) {
3747
this.configuration = configuration;
3848
this.globalOrderer = createGlobalOrderer(configuration);
49+
this.noOrderAnnotation = issueReporter.createReportingCondition(
50+
testDescriptor -> !isAnnotated(testDescriptor.getTestClass(), Order.class), testDescriptor -> {
51+
String message = String.format(
52+
"Ineffective @Order annotation on class '%s'. It will not be applied because ClassOrderer.OrderAnnotation is not in use.",
53+
testDescriptor.getTestClass().getName());
54+
return DiscoveryIssue.builder(Severity.INFO, message) //
55+
.source(ClassSource.from(testDescriptor.getTestClass())) //
56+
.build();
57+
});
3958
}
4059

4160
@Override
@@ -59,31 +78,37 @@ private void orderTopLevelClasses(JupiterEngineDescriptor engineDescriptor) {
5978
orderChildrenTestDescriptors(//
6079
engineDescriptor, //
6180
ClassBasedTestDescriptor.class, //
81+
toValidationAction(globalOrderer), //
6282
DefaultClassDescriptor::new, //
6383
globalOrderer);
6484
}
6585

6686
private void orderNestedClasses(ClassBasedTestDescriptor descriptor) {
87+
DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> wrapperOrderer = createAndCacheClassLevelOrderer(
88+
descriptor);
6789
orderChildrenTestDescriptors(//
6890
descriptor, //
6991
ClassBasedTestDescriptor.class, //
92+
toValidationAction(wrapperOrderer), //
7093
DefaultClassDescriptor::new, //
71-
createAndCacheClassLevelOrderer(descriptor));
94+
wrapperOrderer);
7295
}
7396

74-
private DescriptorWrapperOrderer<DefaultClassDescriptor> createGlobalOrderer(JupiterConfiguration configuration) {
97+
private DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> createGlobalOrderer(
98+
JupiterConfiguration configuration) {
7599
ClassOrderer classOrderer = configuration.getDefaultTestClassOrderer().orElse(null);
76100
return classOrderer == null ? DescriptorWrapperOrderer.noop() : createDescriptorWrapperOrderer(classOrderer);
77101
}
78102

79-
private DescriptorWrapperOrderer<DefaultClassDescriptor> createAndCacheClassLevelOrderer(
103+
private DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> createAndCacheClassLevelOrderer(
80104
ClassBasedTestDescriptor classBasedTestDescriptor) {
81-
DescriptorWrapperOrderer<DefaultClassDescriptor> orderer = createClassLevelOrderer(classBasedTestDescriptor);
105+
DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> orderer = createClassLevelOrderer(
106+
classBasedTestDescriptor);
82107
ordererCache.put(classBasedTestDescriptor, orderer);
83108
return orderer;
84109
}
85110

86-
private DescriptorWrapperOrderer<DefaultClassDescriptor> createClassLevelOrderer(
111+
private DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> createClassLevelOrderer(
87112
ClassBasedTestDescriptor classBasedTestDescriptor) {
88113
return AnnotationSupport.findAnnotation(classBasedTestDescriptor.getTestClass(), TestClassOrder.class)//
89114
.map(TestClassOrder::value)//
@@ -93,15 +118,16 @@ private DescriptorWrapperOrderer<DefaultClassDescriptor> createClassLevelOrderer
93118
Object parent = classBasedTestDescriptor.getParent().orElse(null);
94119
if (parent instanceof ClassBasedTestDescriptor) {
95120
ClassBasedTestDescriptor parentClassTestDescriptor = (ClassBasedTestDescriptor) parent;
96-
DescriptorWrapperOrderer<DefaultClassDescriptor> cacheEntry = ordererCache.get(
121+
DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> cacheEntry = ordererCache.get(
97122
parentClassTestDescriptor);
98123
return cacheEntry != null ? cacheEntry : createClassLevelOrderer(parentClassTestDescriptor);
99124
}
100125
return globalOrderer;
101126
});
102127
}
103128

104-
private DescriptorWrapperOrderer<DefaultClassDescriptor> createDescriptorWrapperOrderer(ClassOrderer classOrderer) {
129+
private DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> createDescriptorWrapperOrderer(
130+
ClassOrderer classOrderer) {
105131
Consumer<List<DefaultClassDescriptor>> orderingAction = classDescriptors -> classOrderer.orderClasses(
106132
new DefaultClassOrdererContext(classDescriptors, this.configuration));
107133

@@ -112,8 +138,17 @@ private DescriptorWrapperOrderer<DefaultClassDescriptor> createDescriptorWrapper
112138
"ClassOrderer [%s] removed %s ClassDescriptor(s) which will be retained with arbitrary ordering.",
113139
classOrderer.getClass().getName(), number);
114140

115-
return new DescriptorWrapperOrderer<>(orderingAction, descriptorsAddedMessageGenerator,
141+
return new DescriptorWrapperOrderer<>(classOrderer, orderingAction, descriptorsAddedMessageGenerator,
116142
descriptorsRemovedMessageGenerator);
117143
}
118144

145+
private Optional<Consumer<ClassBasedTestDescriptor>> toValidationAction(
146+
DescriptorWrapperOrderer<ClassOrderer, DefaultClassDescriptor> wrapperOrderer) {
147+
148+
if (wrapperOrderer.getOrderer() instanceof ClassOrderer.OrderAnnotation) {
149+
return Optional.empty();
150+
}
151+
return Optional.of(noOrderAnnotation::check);
152+
}
153+
119154
}

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolver.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ public class DiscoverySelectorResolver {
4444
ctx.getIssueReporter())) //
4545
.addSelectorResolver(ctx -> new MethodSelectorResolver(getConfiguration(ctx), ctx.getIssueReporter())) //
4646
.addTestDescriptorVisitor(ctx -> TestDescriptor.Visitor.composite( //
47-
new ClassOrderingVisitor(getConfiguration(ctx)), //
48-
new MethodOrderingVisitor(getConfiguration(ctx)), //
47+
new ClassOrderingVisitor(getConfiguration(ctx), ctx.getIssueReporter()), //
48+
new MethodOrderingVisitor(getConfiguration(ctx), ctx.getIssueReporter()), //
4949
descriptor -> {
5050
if (descriptor instanceof Validatable) {
5151
((Validatable) descriptor).validate(ctx.getIssueReporter());

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/MethodOrderingVisitor.java

+71-29
Original file line numberDiff line numberDiff line change
@@ -11,29 +11,46 @@
1111
package org.junit.jupiter.engine.discovery;
1212

1313
import static org.junit.platform.commons.support.AnnotationSupport.findAnnotation;
14+
import static org.junit.platform.commons.support.AnnotationSupport.isAnnotated;
1415

1516
import java.util.List;
1617
import java.util.Optional;
1718
import java.util.function.Consumer;
1819

1920
import org.junit.jupiter.api.MethodOrderer;
21+
import org.junit.jupiter.api.Order;
2022
import org.junit.jupiter.api.TestMethodOrder;
2123
import org.junit.jupiter.engine.config.JupiterConfiguration;
2224
import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor;
2325
import org.junit.jupiter.engine.descriptor.JupiterTestDescriptor;
2426
import org.junit.jupiter.engine.descriptor.MethodBasedTestDescriptor;
2527
import org.junit.platform.commons.support.ReflectionSupport;
28+
import org.junit.platform.engine.DiscoveryIssue;
29+
import org.junit.platform.engine.DiscoveryIssue.Severity;
2630
import org.junit.platform.engine.TestDescriptor;
31+
import org.junit.platform.engine.support.descriptor.MethodSource;
32+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
33+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter.Condition;
2734

2835
/**
2936
* @since 5.5
3037
*/
3138
class MethodOrderingVisitor extends AbstractOrderingVisitor {
3239

3340
private final JupiterConfiguration configuration;
41+
private final Condition<MethodBasedTestDescriptor> noOrderAnnotation;
3442

35-
MethodOrderingVisitor(JupiterConfiguration configuration) {
43+
MethodOrderingVisitor(JupiterConfiguration configuration, DiscoveryIssueReporter issueReporter) {
3644
this.configuration = configuration;
45+
this.noOrderAnnotation = issueReporter.createReportingCondition(
46+
testDescriptor -> !isAnnotated(testDescriptor.getTestMethod(), Order.class), testDescriptor -> {
47+
String message = String.format(
48+
"Ineffective @Order annotation on method '%s'. It will not be applied because MethodOrderer.OrderAnnotation is not in use.",
49+
testDescriptor.getTestMethod().toGenericString());
50+
return DiscoveryIssue.builder(Severity.INFO, message) //
51+
.source(MethodSource.from(testDescriptor.getTestMethod())) //
52+
.build();
53+
});
3754
}
3855

3956
@Override
@@ -54,37 +71,62 @@ protected boolean shouldNonMatchingDescriptorsComeBeforeOrderedOnes() {
5471
* @since 5.4
5572
*/
5673
private void orderContainedMethods(ClassBasedTestDescriptor classBasedTestDescriptor, Class<?> testClass) {
57-
findAnnotation(testClass, TestMethodOrder.class)//
74+
Optional<MethodOrderer> methodOrderer = findAnnotation(testClass, TestMethodOrder.class)//
5875
.map(TestMethodOrder::value)//
5976
.<MethodOrderer> map(ReflectionSupport::newInstance)//
6077
.map(Optional::of)//
61-
.orElseGet(configuration::getDefaultTestMethodOrderer)//
62-
.ifPresent(methodOrderer -> {
63-
64-
Consumer<List<DefaultMethodDescriptor>> orderingAction = methodDescriptors -> methodOrderer.orderMethods(
65-
new DefaultMethodOrdererContext(testClass, methodDescriptors, this.configuration));
66-
67-
MessageGenerator descriptorsAddedMessageGenerator = number -> String.format(
68-
"MethodOrderer [%s] added %s MethodDescriptor(s) for test class [%s] which will be ignored.",
69-
methodOrderer.getClass().getName(), number, testClass.getName());
70-
MessageGenerator descriptorsRemovedMessageGenerator = number -> String.format(
71-
"MethodOrderer [%s] removed %s MethodDescriptor(s) for test class [%s] which will be retained with arbitrary ordering.",
72-
methodOrderer.getClass().getName(), number, testClass.getName());
73-
74-
DescriptorWrapperOrderer<DefaultMethodDescriptor> descriptorWrapperOrderer = new DescriptorWrapperOrderer<>(
75-
orderingAction, descriptorsAddedMessageGenerator, descriptorsRemovedMessageGenerator);
76-
77-
orderChildrenTestDescriptors(classBasedTestDescriptor, //
78-
MethodBasedTestDescriptor.class, //
79-
DefaultMethodDescriptor::new, //
80-
descriptorWrapperOrderer);
81-
82-
// Note: MethodOrderer#getDefaultExecutionMode() is guaranteed
83-
// to be invoked after MethodOrderer#orderMethods().
84-
methodOrderer.getDefaultExecutionMode()//
85-
.map(JupiterTestDescriptor::toExecutionMode)//
86-
.ifPresent(classBasedTestDescriptor::setDefaultChildExecutionMode);
87-
});
78+
.orElseGet(configuration::getDefaultTestMethodOrderer);
79+
orderContainedMethods(classBasedTestDescriptor, testClass, methodOrderer);
80+
}
81+
82+
private void orderContainedMethods(ClassBasedTestDescriptor classBasedTestDescriptor, Class<?> testClass,
83+
Optional<MethodOrderer> methodOrderer) {
84+
DescriptorWrapperOrderer<?, DefaultMethodDescriptor> descriptorWrapperOrderer = createDescriptorWrapperOrderer(
85+
testClass, methodOrderer);
86+
87+
orderChildrenTestDescriptors(classBasedTestDescriptor, //
88+
MethodBasedTestDescriptor.class, //
89+
toValidationAction(methodOrderer), //
90+
DefaultMethodDescriptor::new, //
91+
descriptorWrapperOrderer);
92+
93+
// Note: MethodOrderer#getDefaultExecutionMode() is guaranteed
94+
// to be invoked after MethodOrderer#orderMethods().
95+
methodOrderer //
96+
.flatMap(it -> it.getDefaultExecutionMode().map(JupiterTestDescriptor::toExecutionMode)) //
97+
.ifPresent(classBasedTestDescriptor::setDefaultChildExecutionMode);
98+
}
99+
100+
private DescriptorWrapperOrderer<?, DefaultMethodDescriptor> createDescriptorWrapperOrderer(Class<?> testClass,
101+
Optional<MethodOrderer> methodOrderer) {
102+
103+
return methodOrderer //
104+
.map(it -> createDescriptorWrapperOrderer(testClass, it)) //
105+
.orElseGet(DescriptorWrapperOrderer::noop);
106+
107+
}
108+
109+
private DescriptorWrapperOrderer<?, DefaultMethodDescriptor> createDescriptorWrapperOrderer(Class<?> testClass,
110+
MethodOrderer methodOrderer) {
111+
Consumer<List<DefaultMethodDescriptor>> orderingAction = methodDescriptors -> methodOrderer.orderMethods(
112+
new DefaultMethodOrdererContext(testClass, methodDescriptors, this.configuration));
113+
114+
MessageGenerator descriptorsAddedMessageGenerator = number -> String.format(
115+
"MethodOrderer [%s] added %s MethodDescriptor(s) for test class [%s] which will be ignored.",
116+
methodOrderer.getClass().getName(), number, testClass.getName());
117+
MessageGenerator descriptorsRemovedMessageGenerator = number -> String.format(
118+
"MethodOrderer [%s] removed %s MethodDescriptor(s) for test class [%s] which will be retained with arbitrary ordering.",
119+
methodOrderer.getClass().getName(), number, testClass.getName());
120+
121+
return new DescriptorWrapperOrderer<>(methodOrderer, orderingAction, descriptorsAddedMessageGenerator,
122+
descriptorsRemovedMessageGenerator);
123+
}
124+
125+
private Optional<Consumer<MethodBasedTestDescriptor>> toValidationAction(Optional<MethodOrderer> methodOrderer) {
126+
if (methodOrderer.orElse(null) instanceof MethodOrderer.OrderAnnotation) {
127+
return Optional.empty();
128+
}
129+
return Optional.of(noOrderAnnotation::check);
88130
}
89131

90132
}

0 commit comments

Comments
 (0)