Skip to content

Commit 5d07855

Browse files
marcphilippmarcphilipp
committed
Report discovery issues misbehaving orderers
1 parent 786de29 commit 5d07855

File tree

4 files changed

+67
-41
lines changed

4 files changed

+67
-41
lines changed

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

+26-12
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,11 @@
2323
import java.util.function.Function;
2424
import java.util.stream.Stream;
2525

26-
import org.junit.platform.commons.logging.Logger;
27-
import org.junit.platform.commons.logging.LoggerFactory;
2826
import org.junit.platform.commons.util.UnrecoverableExceptions;
27+
import org.junit.platform.engine.DiscoveryIssue;
28+
import org.junit.platform.engine.DiscoveryIssue.Severity;
2929
import org.junit.platform.engine.TestDescriptor;
30+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
3031

3132
/**
3233
* Abstract base class for {@linkplain TestDescriptor.Visitor visitors} that
@@ -36,7 +37,11 @@
3637
*/
3738
abstract class AbstractOrderingVisitor implements TestDescriptor.Visitor {
3839

39-
private static final Logger logger = LoggerFactory.getLogger(AbstractOrderingVisitor.class);
40+
private final DiscoveryIssueReporter issueReporter;
41+
42+
AbstractOrderingVisitor(DiscoveryIssueReporter issueReporter) {
43+
this.issueReporter = issueReporter;
44+
}
4045

4146
/**
4247
* @param <PARENT> the parent container type to search in for matching children
@@ -52,7 +57,10 @@ protected <PARENT extends TestDescriptor> void doWithMatchingDescriptor(Class<PA
5257
}
5358
catch (Throwable t) {
5459
UnrecoverableExceptions.rethrowIfUnrecoverable(t);
55-
logger.error(t, () -> errorMessageBuilder.apply(parentTestDescriptor));
60+
String message = errorMessageBuilder.apply(parentTestDescriptor);
61+
this.issueReporter.reportIssue(DiscoveryIssue.builder(Severity.ERROR, message) //
62+
.source(parentTestDescriptor.getSource()) //
63+
.cause(t));
5664
}
5765
}
5866
}
@@ -92,7 +100,8 @@ protected <CHILD extends TestDescriptor, WRAPPER extends AbstractAnnotatedDescri
92100
Stream<TestDescriptor> nonMatchingTestDescriptors = children.stream()//
93101
.filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor));
94102

95-
descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers);
103+
descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers,
104+
message -> reportWarning(parentTestDescriptor, message));
96105

97106
Stream<TestDescriptor> orderedTestDescriptors = matchingDescriptorWrappers.stream()//
98107
.map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor);
@@ -108,6 +117,11 @@ protected <CHILD extends TestDescriptor, WRAPPER extends AbstractAnnotatedDescri
108117
});
109118
}
110119

120+
private void reportWarning(TestDescriptor parentTestDescriptor, String message) {
121+
issueReporter.reportIssue(DiscoveryIssue.builder(Severity.WARNING, message) //
122+
.source(parentTestDescriptor.getSource()));
123+
}
124+
111125
protected abstract boolean shouldNonMatchingDescriptorsComeBeforeOrderedOnes();
112126

113127
/**
@@ -146,18 +160,18 @@ private boolean canOrderWrappers() {
146160
return this.orderingAction != null;
147161
}
148162

149-
private void orderWrappers(List<WRAPPER> wrappers) {
163+
private void orderWrappers(List<WRAPPER> wrappers, Consumer<String> errorHandler) {
150164
List<WRAPPER> orderedWrappers = new ArrayList<>(wrappers);
151165
this.orderingAction.accept(orderedWrappers);
152166
Map<Object, Integer> distinctWrappersToIndex = distinctWrappersToIndex(orderedWrappers);
153167

154168
int difference = orderedWrappers.size() - wrappers.size();
155169
int distinctDifference = distinctWrappersToIndex.size() - wrappers.size();
156170
if (difference > 0) { // difference >= distinctDifference
157-
logDescriptorsAddedWarning(difference);
171+
reportDescriptorsAddedWarning(difference, errorHandler);
158172
}
159173
if (distinctDifference < 0) { // distinctDifference <= difference
160-
logDescriptorsRemovedWarning(distinctDifference);
174+
reportDescriptorsRemovedWarning(distinctDifference, errorHandler);
161175
}
162176

163177
wrappers.sort(comparing(wrapper -> distinctWrappersToIndex.getOrDefault(wrapper, -1)));
@@ -175,12 +189,12 @@ private Map<Object, Integer> distinctWrappersToIndex(List<?> wrappers) {
175189
return toIndex;
176190
}
177191

178-
private void logDescriptorsAddedWarning(int number) {
179-
logger.warn(() -> this.descriptorsAddedMessageGenerator.generateMessage(number));
192+
private void reportDescriptorsAddedWarning(int number, Consumer<String> errorHandler) {
193+
errorHandler.accept(this.descriptorsAddedMessageGenerator.generateMessage(number));
180194
}
181195

182-
private void logDescriptorsRemovedWarning(int number) {
183-
logger.warn(() -> this.descriptorsRemovedMessageGenerator.generateMessage(Math.abs(number)));
196+
private void reportDescriptorsRemovedWarning(int number, Consumer<String> errorHandler) {
197+
errorHandler.accept(this.descriptorsRemovedMessageGenerator.generateMessage(Math.abs(number)));
184198
}
185199

186200
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ class ClassOrderingVisitor extends AbstractOrderingVisitor {
4444
private final Condition<ClassBasedTestDescriptor> noOrderAnnotation;
4545

4646
ClassOrderingVisitor(JupiterConfiguration configuration, DiscoveryIssueReporter issueReporter) {
47+
super(issueReporter);
4748
this.configuration = configuration;
4849
this.globalOrderer = createGlobalOrderer(configuration);
4950
this.noOrderAnnotation = issueReporter.createReportingCondition(

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

+1
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class MethodOrderingVisitor extends AbstractOrderingVisitor {
4141
private final Condition<MethodBasedTestDescriptor> noOrderAnnotation;
4242

4343
MethodOrderingVisitor(JupiterConfiguration configuration, DiscoveryIssueReporter issueReporter) {
44+
super(issueReporter);
4445
this.configuration = configuration;
4546
this.noOrderAnnotation = issueReporter.createReportingCondition(
4647
testDescriptor -> !isAnnotated(testDescriptor.getTestMethod(), Order.class), testDescriptor -> {

‎jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java

+39-29
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static org.junit.jupiter.engine.Constants.DEFAULT_TEST_METHOD_ORDER_PROPERTY_NAME;
2020
import static org.junit.jupiter.engine.Constants.PARALLEL_EXECUTION_ENABLED_PROPERTY_NAME;
2121
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
22+
import static org.junit.platform.launcher.LauncherConstants.CRITICAL_DISCOVERY_ISSUE_SEVERITY_PROPERTY_NAME;
2223

2324
import java.lang.annotation.Annotation;
2425
import java.lang.reflect.Method;
@@ -58,6 +59,7 @@
5859
import org.junit.platform.commons.util.ClassUtils;
5960
import org.junit.platform.engine.DiscoveryIssue;
6061
import org.junit.platform.engine.DiscoveryIssue.Severity;
62+
import org.junit.platform.engine.support.descriptor.ClassSource;
6163
import org.junit.platform.engine.support.descriptor.MethodSource;
6264
import org.junit.platform.testkit.engine.EngineDiscoveryResults;
6365
import org.junit.platform.testkit.engine.EngineTestKit;
@@ -289,35 +291,50 @@ void reportsDiscoveryIssuesForIneffectiveOrderAnnotations() throws Exception {
289291
}
290292

291293
@Test
292-
void misbehavingMethodOrdererThatAddsElements(@TrackLogRecords LogRecordListener listener) {
294+
void misbehavingMethodOrdererThatAddsElements() {
293295
Class<?> testClass = MisbehavingByAddingTestCase.class;
294296

295-
executeTestsInParallel(testClass, Random.class).assertStatistics(stats -> stats.succeeded(2));
297+
var discoveryIssues = discoverTests(testClass, null).getDiscoveryIssues();
298+
assertThat(discoveryIssues).hasSize(1);
296299

297-
assertThat(callSequence).containsExactly("test1()", "test2()");
300+
var issue = discoveryIssues.getFirst();
301+
assertThat(issue.severity()).isEqualTo(Severity.WARNING);
302+
assertThat(issue.message()).isEqualTo(
303+
"MethodOrderer [%s] added 2 MethodDescriptor(s) for test class [%s] which will be ignored.",
304+
MisbehavingByAdding.class.getName(), testClass.getName());
305+
assertThat(issue.source()).contains(ClassSource.from(testClass));
298306

299-
var expectedMessage = "MethodOrderer [" + MisbehavingByAdding.class.getName()
300-
+ "] added 2 MethodDescriptor(s) for test class [" + testClass.getName() + "] which will be ignored.";
307+
executeTestsInParallel(testClass, null, Severity.ERROR) //
308+
.assertStatistics(stats -> stats.succeeded(2));
301309

302-
assertExpectedLogMessage(listener, expectedMessage);
310+
assertThat(callSequence).containsExactly("test1()", "test2()");
303311
}
304312

305313
@Test
306-
void misbehavingMethodOrdererThatImpersonatesElements(@TrackLogRecords LogRecordListener listener) {
314+
void misbehavingMethodOrdererThatImpersonatesElements() {
307315
Class<?> testClass = MisbehavingByImpersonatingTestCase.class;
308316

309317
executeTestsInParallel(testClass, Random.class).assertStatistics(stats -> stats.succeeded(2));
310318

311319
assertThat(callSequence).containsExactlyInAnyOrder("test1()", "test2()");
312-
313-
assertThat(listener.stream(Level.WARNING)).isEmpty();
314320
}
315321

316322
@Test
317-
void misbehavingMethodOrdererThatRemovesElements(@TrackLogRecords LogRecordListener listener) {
323+
void misbehavingMethodOrdererThatRemovesElements() {
318324
Class<?> testClass = MisbehavingByRemovingTestCase.class;
319325

320-
executeTestsInParallel(testClass, Random.class).assertStatistics(stats -> stats.succeeded(4));
326+
var discoveryIssues = discoverTests(testClass, null).getDiscoveryIssues();
327+
assertThat(discoveryIssues).hasSize(1);
328+
329+
var issue = discoveryIssues.getFirst();
330+
assertThat(issue.severity()).isEqualTo(Severity.WARNING);
331+
assertThat(issue.message()).isEqualTo(
332+
"MethodOrderer [%s] removed 2 MethodDescriptor(s) for test class [%s] which will be retained with arbitrary ordering.",
333+
MisbehavingByRemoving.class.getName(), testClass.getName());
334+
assertThat(issue.source()).contains(ClassSource.from(testClass));
335+
336+
executeTestsInParallel(testClass, null, Severity.ERROR) //
337+
.assertStatistics(stats -> stats.succeeded(4));
321338

322339
assertThat(callSequence) //
323340
.containsExactlyInAnyOrder("test1()", "test2()", "test3()", "test4()") //
@@ -326,36 +343,29 @@ void misbehavingMethodOrdererThatRemovesElements(@TrackLogRecords LogRecordListe
326343
.containsSubsequence("test1()", "test4()") // removed item is re-added before ordered item
327344
.containsSubsequence("test2()", "test3()") // removed item is re-added before ordered item
328345
.containsSubsequence("test2()", "test4()");// removed item is re-added before ordered item
329-
330-
var expectedMessage = "MethodOrderer [" + MisbehavingByRemoving.class.getName()
331-
+ "] removed 2 MethodDescriptor(s) for test class [" + testClass.getName()
332-
+ "] which will be retained with arbitrary ordering.";
333-
334-
assertExpectedLogMessage(listener, expectedMessage);
335-
}
336-
337-
private void assertExpectedLogMessage(LogRecordListener listener, String expectedMessage) {
338-
// @formatter:off
339-
assertThat(listener.stream(Level.WARNING)
340-
.map(LogRecord::getMessage))
341-
.contains(expectedMessage);
342-
// @formatter:on
343346
}
344347

345348
private EngineDiscoveryResults discoverTests(Class<?> testClass, Class<? extends MethodOrderer> defaultOrderer) {
346-
return testKit(testClass, defaultOrderer).discover();
349+
return testKit(testClass, defaultOrderer, Severity.INFO).discover();
347350
}
348351

349352
private Events executeTestsInParallel(Class<?> testClass, Class<? extends MethodOrderer> defaultOrderer) {
350-
return testKit(testClass, defaultOrderer) //
353+
return executeTestsInParallel(testClass, defaultOrderer, Severity.INFO);
354+
}
355+
356+
private Events executeTestsInParallel(Class<?> testClass, Class<? extends MethodOrderer> defaultOrderer,
357+
Severity criticalSeverity) {
358+
return testKit(testClass, defaultOrderer, criticalSeverity) //
351359
.execute() //
352360
.testEvents();
353361
}
354362

355-
private static EngineTestKit.Builder testKit(Class<?> testClass, Class<? extends MethodOrderer> defaultOrderer) {
363+
private static EngineTestKit.Builder testKit(Class<?> testClass, Class<? extends MethodOrderer> defaultOrderer,
364+
Severity criticalSeverity) {
356365
var testKit = EngineTestKit.engine("junit-jupiter") //
357366
.configurationParameter(PARALLEL_EXECUTION_ENABLED_PROPERTY_NAME, "true") //
358-
.configurationParameter(DEFAULT_PARALLEL_EXECUTION_MODE, "concurrent");
367+
.configurationParameter(DEFAULT_PARALLEL_EXECUTION_MODE, "concurrent") //
368+
.configurationParameter(CRITICAL_DISCOVERY_ISSUE_SEVERITY_PROPERTY_NAME, criticalSeverity.name());
359369
if (defaultOrderer != null) {
360370
testKit.configurationParameter(DEFAULT_TEST_METHOD_ORDER_PROPERTY_NAME, defaultOrderer.getName());
361371
}

0 commit comments

Comments
 (0)