From 5f47dae008c6c45add6f43f0fe52f3a947864e77 Mon Sep 17 00:00:00 2001 From: Pavlo Shevchenko Date: Mon, 14 Aug 2023 16:45:51 +0200 Subject: [PATCH 1/7] Add test for wrong reporting of skipped JUnit3 test cases --- .../VintageTestEngineExecutionTests.java | 10 +++++++ .../samples/junit3/IgnoredJUnit3TestCase.java | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java index 66af815aff99..781b7ead1b9d 100644 --- a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java +++ b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java @@ -56,6 +56,7 @@ import org.junit.runner.notification.RunNotifier; import org.junit.runners.Suite; import org.junit.runners.Suite.SuiteClasses; +import org.junit.vintage.engine.samples.junit3.IgnoredJUnit3TestCase; import org.junit.vintage.engine.samples.junit3.JUnit3ParallelSuiteWithSubsuites; import org.junit.vintage.engine.samples.junit3.JUnit3SuiteWithSubsuites; import org.junit.vintage.engine.samples.junit3.PlainJUnit3TestCaseWithSingleTestWhichFails; @@ -896,6 +897,15 @@ void executesRegularSpockFeatureMethod() { event(engine(), finishedSuccessfully())); } + @Test + void executesIgnoredJUnit3TestCase() { + var suiteClass = IgnoredJUnit3TestCase.class; + execute(suiteClass).allEvents().assertEventsMatchExactly( // + event(engine(), started()), // + event(container(suiteClass), skippedWithReason(__ -> true)), // + event(engine(), finishedSuccessfully())); + } + private static EngineExecutionResults execute(Class testClass) { return execute(request(testClass)); } diff --git a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java new file mode 100644 index 000000000000..6f16db32e5c3 --- /dev/null +++ b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java @@ -0,0 +1,28 @@ +/* + * Copyright 2015-2023 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.vintage.engine.samples.junit3; + +import junit.framework.TestCase; + +import org.junit.Assert; +import org.junit.Ignore; + +/** + * @since 4.12 + */ +@Ignore +public class IgnoredJUnit3TestCase extends TestCase { + + public void test() { + Assert.fail("this test should be ignored"); + } + +} From 5d68ad5fb9f155c5ff0d39c84d78e550bf5b8647 Mon Sep 17 00:00:00 2001 From: Pavlo Shevchenko Date: Tue, 15 Aug 2023 08:59:07 +0200 Subject: [PATCH 2/7] Manually add `@Ignore` annotation to the `JUnit38ClassRunner` root description --- ...fensiveAllDefaultPossibilitiesBuilder.java | 11 +++-- .../FilterableIgnoringRunnerDecorator.java | 5 +- .../discovery/IgnoringRunnerDecorator.java | 49 ++++++++++++++++++- .../VintageTestEngineExecutionTests.java | 2 +- .../samples/junit3/IgnoredJUnit3TestCase.java | 2 +- 5 files changed, 58 insertions(+), 11 deletions(-) diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java index 0da94f689fde..ee47d1ec8e01 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java @@ -55,11 +55,12 @@ class DefensiveAllDefaultPossibilitiesBuilder extends AllDefaultPossibilitiesBui @Override public Runner runnerForClass(Class testClass) throws Throwable { Runner runner = super.runnerForClass(testClass); - if (testClass.getAnnotation(Ignore.class) != null) { + Ignore ignoreAnnotation = testClass.getAnnotation(Ignore.class); + if (ignoreAnnotation != null) { if (runner == null) { return new IgnoredClassRunner(testClass); } - return decorateIgnoredTestClass(runner); + return decorateIgnoredTestClass(runner, ignoreAnnotation); } return runner; } @@ -72,11 +73,11 @@ public Runner runnerForClass(Class testClass) throws Throwable { * override its runtime behavior (i.e. skip execution) but return its * regular {@link org.junit.runner.Description}. */ - private Runner decorateIgnoredTestClass(Runner runner) { + private Runner decorateIgnoredTestClass(Runner runner, Ignore ignoreAnnotation) { if (runner instanceof Filterable) { - return new FilterableIgnoringRunnerDecorator(runner); + return new FilterableIgnoringRunnerDecorator(runner, ignoreAnnotation); } - return new IgnoringRunnerDecorator(runner); + return new IgnoringRunnerDecorator(runner, ignoreAnnotation); } @Override diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java index 636ef2554802..29306be25caa 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java @@ -10,6 +10,7 @@ package org.junit.vintage.engine.discovery; +import org.junit.Ignore; import org.junit.platform.commons.util.Preconditions; import org.junit.runner.Runner; import org.junit.runner.manipulation.Filter; @@ -23,8 +24,8 @@ */ class FilterableIgnoringRunnerDecorator extends IgnoringRunnerDecorator implements Filterable { - FilterableIgnoringRunnerDecorator(Runner runner) { - super(runner); + FilterableIgnoringRunnerDecorator(Runner runner, Ignore ignoreAnnotation) { + super(runner, ignoreAnnotation); Preconditions.condition(runner instanceof Filterable, () -> "Runner must be an instance of Filterable: " + runner.getClass().getName()); } diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java index 3b5597e51169..a53fd9cb2566 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java @@ -10,11 +10,20 @@ package org.junit.vintage.engine.discovery; +import java.lang.annotation.Annotation; +import java.util.ArrayList; +import java.util.List; + +import org.junit.Ignore; +import org.junit.internal.runners.JUnit38ClassRunner; +import org.junit.platform.commons.logging.Logger; +import org.junit.platform.commons.logging.LoggerFactory; import org.junit.platform.commons.util.Preconditions; import org.junit.runner.Description; import org.junit.runner.Runner; import org.junit.runner.notification.RunNotifier; import org.junit.vintage.engine.descriptor.RunnerDecorator; +import org.junit.vintage.engine.descriptor.RunnerTestDescriptor; /** * Decorator for Runners that will be ignored completely. @@ -26,15 +35,29 @@ */ class IgnoringRunnerDecorator extends Runner implements RunnerDecorator { + private static final Logger logger = LoggerFactory.getLogger(RunnerTestDescriptor.class); + protected final Runner runner; + private final Ignore testClassIgnoreAnnotation; - IgnoringRunnerDecorator(Runner runner) { + IgnoringRunnerDecorator(Runner runner, Ignore ignoreAnnotation) { this.runner = Preconditions.notNull(runner, "Runner must not be null"); + this.testClassIgnoreAnnotation = Preconditions.notNull(ignoreAnnotation, + "Test class @Ignore annotation must not be null"); } @Override public Description getDescription() { - return runner.getDescription(); + Description originalDescription = runner.getDescription(); + + if (runner instanceof JUnit38ClassRunner) { + return junit38ClassRunnerDescriptionWithIgnoreAnnotation(originalDescription); + } + else if (originalDescription.getAnnotation(Ignore.class) == null) { + warnAboutMissingIgnoreAnnotation(originalDescription); + } + + return originalDescription; } @Override @@ -46,4 +69,26 @@ public void run(RunNotifier notifier) { public Runner getDecoratedRunner() { return runner; } + + /** + * {@link JUnit38ClassRunner} does not add class-level annotations to the runner description, + * which results in an inconsistent behavior when combined with the vintage engine: the runner description + * will be marked as started because the runner told so, but it will alos be reported as skipped by IgnoringRunnerDecorator + * which detected the @Ignore annotation on the test Java class. + */ + private Description junit38ClassRunnerDescriptionWithIgnoreAnnotation(Description runnerDescription) { + List effectiveAnnotations = new ArrayList<>(runnerDescription.getAnnotations()); + effectiveAnnotations.add(testClassIgnoreAnnotation); + + Description updatedRunnerDescription = Description.createTestDescription(runnerDescription.getClassName(), + runnerDescription.getMethodName(), effectiveAnnotations.toArray(new Annotation[0])); + + runnerDescription.getChildren().forEach(updatedRunnerDescription::addChild); + return updatedRunnerDescription; + } + + private void warnAboutMissingIgnoreAnnotation(Description originalDescription) { + logger.warn(() -> "Custom test runner '" + runner.getClass().getName() + + "' did not add an @Ignore annotation to the runner description " + originalDescription); + } } diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java index 781b7ead1b9d..638066a1be9e 100644 --- a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java +++ b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java @@ -902,7 +902,7 @@ void executesIgnoredJUnit3TestCase() { var suiteClass = IgnoredJUnit3TestCase.class; execute(suiteClass).allEvents().assertEventsMatchExactly( // event(engine(), started()), // - event(container(suiteClass), skippedWithReason(__ -> true)), // + event(container(suiteClass), skippedWithReason("respected by Vintage engine")), // event(engine(), finishedSuccessfully())); } diff --git a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java index 6f16db32e5c3..21c125221154 100644 --- a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java +++ b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java @@ -18,7 +18,7 @@ /** * @since 4.12 */ -@Ignore +@Ignore("respected by Vintage engine") public class IgnoredJUnit3TestCase extends TestCase { public void test() { From 5bf067f49fb0e1df5ee7eef73c1d8d84d8aa339c Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 5 Sep 2023 12:31:39 +0200 Subject: [PATCH 3/7] Revert "Manually add `@Ignore` annotation to the `JUnit38ClassRunner` root description" This reverts commit ea3e7df11c9c9c3ed93eaa571b1ab858c8776136. --- ...fensiveAllDefaultPossibilitiesBuilder.java | 11 ++--- .../FilterableIgnoringRunnerDecorator.java | 5 +- .../discovery/IgnoringRunnerDecorator.java | 49 +------------------ .../VintageTestEngineExecutionTests.java | 2 +- .../samples/junit3/IgnoredJUnit3TestCase.java | 2 +- 5 files changed, 11 insertions(+), 58 deletions(-) diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java index ee47d1ec8e01..0da94f689fde 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java @@ -55,12 +55,11 @@ class DefensiveAllDefaultPossibilitiesBuilder extends AllDefaultPossibilitiesBui @Override public Runner runnerForClass(Class testClass) throws Throwable { Runner runner = super.runnerForClass(testClass); - Ignore ignoreAnnotation = testClass.getAnnotation(Ignore.class); - if (ignoreAnnotation != null) { + if (testClass.getAnnotation(Ignore.class) != null) { if (runner == null) { return new IgnoredClassRunner(testClass); } - return decorateIgnoredTestClass(runner, ignoreAnnotation); + return decorateIgnoredTestClass(runner); } return runner; } @@ -73,11 +72,11 @@ public Runner runnerForClass(Class testClass) throws Throwable { * override its runtime behavior (i.e. skip execution) but return its * regular {@link org.junit.runner.Description}. */ - private Runner decorateIgnoredTestClass(Runner runner, Ignore ignoreAnnotation) { + private Runner decorateIgnoredTestClass(Runner runner) { if (runner instanceof Filterable) { - return new FilterableIgnoringRunnerDecorator(runner, ignoreAnnotation); + return new FilterableIgnoringRunnerDecorator(runner); } - return new IgnoringRunnerDecorator(runner, ignoreAnnotation); + return new IgnoringRunnerDecorator(runner); } @Override diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java index 29306be25caa..636ef2554802 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/FilterableIgnoringRunnerDecorator.java @@ -10,7 +10,6 @@ package org.junit.vintage.engine.discovery; -import org.junit.Ignore; import org.junit.platform.commons.util.Preconditions; import org.junit.runner.Runner; import org.junit.runner.manipulation.Filter; @@ -24,8 +23,8 @@ */ class FilterableIgnoringRunnerDecorator extends IgnoringRunnerDecorator implements Filterable { - FilterableIgnoringRunnerDecorator(Runner runner, Ignore ignoreAnnotation) { - super(runner, ignoreAnnotation); + FilterableIgnoringRunnerDecorator(Runner runner) { + super(runner); Preconditions.condition(runner instanceof Filterable, () -> "Runner must be an instance of Filterable: " + runner.getClass().getName()); } diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java index a53fd9cb2566..3b5597e51169 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/IgnoringRunnerDecorator.java @@ -10,20 +10,11 @@ package org.junit.vintage.engine.discovery; -import java.lang.annotation.Annotation; -import java.util.ArrayList; -import java.util.List; - -import org.junit.Ignore; -import org.junit.internal.runners.JUnit38ClassRunner; -import org.junit.platform.commons.logging.Logger; -import org.junit.platform.commons.logging.LoggerFactory; import org.junit.platform.commons.util.Preconditions; import org.junit.runner.Description; import org.junit.runner.Runner; import org.junit.runner.notification.RunNotifier; import org.junit.vintage.engine.descriptor.RunnerDecorator; -import org.junit.vintage.engine.descriptor.RunnerTestDescriptor; /** * Decorator for Runners that will be ignored completely. @@ -35,29 +26,15 @@ */ class IgnoringRunnerDecorator extends Runner implements RunnerDecorator { - private static final Logger logger = LoggerFactory.getLogger(RunnerTestDescriptor.class); - protected final Runner runner; - private final Ignore testClassIgnoreAnnotation; - IgnoringRunnerDecorator(Runner runner, Ignore ignoreAnnotation) { + IgnoringRunnerDecorator(Runner runner) { this.runner = Preconditions.notNull(runner, "Runner must not be null"); - this.testClassIgnoreAnnotation = Preconditions.notNull(ignoreAnnotation, - "Test class @Ignore annotation must not be null"); } @Override public Description getDescription() { - Description originalDescription = runner.getDescription(); - - if (runner instanceof JUnit38ClassRunner) { - return junit38ClassRunnerDescriptionWithIgnoreAnnotation(originalDescription); - } - else if (originalDescription.getAnnotation(Ignore.class) == null) { - warnAboutMissingIgnoreAnnotation(originalDescription); - } - - return originalDescription; + return runner.getDescription(); } @Override @@ -69,26 +46,4 @@ public void run(RunNotifier notifier) { public Runner getDecoratedRunner() { return runner; } - - /** - * {@link JUnit38ClassRunner} does not add class-level annotations to the runner description, - * which results in an inconsistent behavior when combined with the vintage engine: the runner description - * will be marked as started because the runner told so, but it will alos be reported as skipped by IgnoringRunnerDecorator - * which detected the @Ignore annotation on the test Java class. - */ - private Description junit38ClassRunnerDescriptionWithIgnoreAnnotation(Description runnerDescription) { - List effectiveAnnotations = new ArrayList<>(runnerDescription.getAnnotations()); - effectiveAnnotations.add(testClassIgnoreAnnotation); - - Description updatedRunnerDescription = Description.createTestDescription(runnerDescription.getClassName(), - runnerDescription.getMethodName(), effectiveAnnotations.toArray(new Annotation[0])); - - runnerDescription.getChildren().forEach(updatedRunnerDescription::addChild); - return updatedRunnerDescription; - } - - private void warnAboutMissingIgnoreAnnotation(Description originalDescription) { - logger.warn(() -> "Custom test runner '" + runner.getClass().getName() - + "' did not add an @Ignore annotation to the runner description " + originalDescription); - } } diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java index 638066a1be9e..781b7ead1b9d 100644 --- a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java +++ b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java @@ -902,7 +902,7 @@ void executesIgnoredJUnit3TestCase() { var suiteClass = IgnoredJUnit3TestCase.class; execute(suiteClass).allEvents().assertEventsMatchExactly( // event(engine(), started()), // - event(container(suiteClass), skippedWithReason("respected by Vintage engine")), // + event(container(suiteClass), skippedWithReason(__ -> true)), // event(engine(), finishedSuccessfully())); } diff --git a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java index 21c125221154..6f16db32e5c3 100644 --- a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java +++ b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java @@ -18,7 +18,7 @@ /** * @since 4.12 */ -@Ignore("respected by Vintage engine") +@Ignore public class IgnoredJUnit3TestCase extends TestCase { public void test() { From 25ca326459a9bb09da27ed02f5170f335c5cdabc Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 5 Sep 2023 12:43:54 +0200 Subject: [PATCH 4/7] Track skip reason in RunnerTestDescriptor --- .../engine/descriptor/RunnerTestDescriptor.java | 7 +++++++ .../vintage/engine/execution/RunListenerAdapter.java | 11 ++++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java index 0ad3c5f9b6a0..af1013dc1d0b 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java @@ -21,6 +21,7 @@ import java.util.function.Consumer; import org.apiguardian.api.API; +import org.junit.Ignore; import org.junit.platform.commons.JUnitException; import org.junit.platform.commons.logging.Logger; import org.junit.platform.commons.logging.LoggerFactory; @@ -43,12 +44,14 @@ public class RunnerTestDescriptor extends VintageTestDescriptor { private final Set rejectedExclusions = new HashSet<>(); private Runner runner; + private final Optional skipReason; private boolean wasFiltered; private List filters = new ArrayList<>(); public RunnerTestDescriptor(UniqueId uniqueId, Class testClass, Runner runner) { super(uniqueId, runner.getDescription(), testClass.getSimpleName(), ClassSource.from(testClass)); this.runner = runner; + this.skipReason = Optional.ofNullable(testClass.getAnnotation(Ignore.class)).map(Ignore::value); } @Override @@ -155,6 +158,10 @@ private Runner getRunnerToReport() { return (runner instanceof RunnerDecorator) ? ((RunnerDecorator) runner).getDecoratedRunner() : runner; } + public Optional getSkipReason() { + return skipReason; + } + private static class ExcludeDescriptionFilter extends Filter { private final Description description; diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java index 9cefb8faee30..88ea197e5604 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java @@ -49,7 +49,7 @@ class RunListenerAdapter extends RunListener { @Override public void testRunStarted(Description description) { - if (description.isSuite() && description.getAnnotation(Ignore.class) == null) { + if (description.isSuite() && !testRun.getRunnerTestDescriptor().getSkipReason().isPresent()) { fireExecutionStarted(testRun.getRunnerTestDescriptor(), EventType.REPORTED); } } @@ -65,7 +65,8 @@ public void testSuiteStarted(Description description) { @Override public void testIgnored(Description description) { - testIgnored(lookupOrRegisterNextTestDescriptor(description), determineReasonForIgnoredTest(description)); + TestDescriptor testDescriptor = lookupOrRegisterNextTestDescriptor(description); + testIgnored(testDescriptor, determineReasonForIgnoredTest(description).orElse("")); } @Override @@ -176,9 +177,9 @@ private void testIgnored(TestDescriptor testDescriptor, String reason) { fireExecutionSkipped(testDescriptor, reason); } - private String determineReasonForIgnoredTest(Description description) { - Ignore ignoreAnnotation = description.getAnnotation(Ignore.class); - return Optional.ofNullable(ignoreAnnotation).map(Ignore::value).orElse(""); + private Optional determineReasonForIgnoredTest(Description description) { + Optional reason = Optional.ofNullable(description.getAnnotation(Ignore.class)).map(Ignore::value); + return reason.isPresent() ? reason : testRun.getRunnerTestDescriptor().getSkipReason(); } private void dynamicTestRegistered(TestDescriptor testDescriptor) { From 37bb89e126b56c0a13d6dac9b772eabee66ef77e Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 5 Sep 2023 12:51:26 +0200 Subject: [PATCH 5/7] Add another test case --- .../VintageTestEngineExecutionTests.java | 16 ++++++++++++++- .../samples/junit3/IgnoredJUnit3TestCase.java | 2 +- .../JUnit4SuiteWithIgnoredJUnit3TestCase.java | 20 +++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/JUnit4SuiteWithIgnoredJUnit3TestCase.java diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java index 781b7ead1b9d..cd33b52e2269 100644 --- a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java +++ b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java @@ -10,6 +10,7 @@ package org.junit.vintage.engine; +import static java.util.function.Predicate.isEqual; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assumptions.assumeTrue; import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass; @@ -59,6 +60,7 @@ import org.junit.vintage.engine.samples.junit3.IgnoredJUnit3TestCase; import org.junit.vintage.engine.samples.junit3.JUnit3ParallelSuiteWithSubsuites; import org.junit.vintage.engine.samples.junit3.JUnit3SuiteWithSubsuites; +import org.junit.vintage.engine.samples.junit3.JUnit4SuiteWithIgnoredJUnit3TestCase; import org.junit.vintage.engine.samples.junit3.PlainJUnit3TestCaseWithSingleTestWhichFails; import org.junit.vintage.engine.samples.junit4.CompletelyDynamicTestCase; import org.junit.vintage.engine.samples.junit4.EmptyIgnoredTestCase; @@ -902,7 +904,19 @@ void executesIgnoredJUnit3TestCase() { var suiteClass = IgnoredJUnit3TestCase.class; execute(suiteClass).allEvents().assertEventsMatchExactly( // event(engine(), started()), // - event(container(suiteClass), skippedWithReason(__ -> true)), // + event(container(suiteClass), skippedWithReason(isEqual("testing"))), // + event(engine(), finishedSuccessfully())); + } + + @Test + void executesJUnit4SuiteWithIgnoredJUnit3TestCase() { + var suiteClass = JUnit4SuiteWithIgnoredJUnit3TestCase.class; + var testClass = IgnoredJUnit3TestCase.class; + execute(suiteClass).allEvents().assertEventsMatchExactly( // + event(engine(), started()), // + event(container(suiteClass), started()), // + event(container(testClass), skippedWithReason(isEqual("testing"))), // + event(container(suiteClass), finishedSuccessfully()), // event(engine(), finishedSuccessfully())); } diff --git a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java index 6f16db32e5c3..64015bf57d52 100644 --- a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java +++ b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/IgnoredJUnit3TestCase.java @@ -18,7 +18,7 @@ /** * @since 4.12 */ -@Ignore +@Ignore("testing") public class IgnoredJUnit3TestCase extends TestCase { public void test() { diff --git a/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/JUnit4SuiteWithIgnoredJUnit3TestCase.java b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/JUnit4SuiteWithIgnoredJUnit3TestCase.java new file mode 100644 index 000000000000..ba89153ace0c --- /dev/null +++ b/junit-vintage-engine/src/testFixtures/java/org/junit/vintage/engine/samples/junit3/JUnit4SuiteWithIgnoredJUnit3TestCase.java @@ -0,0 +1,20 @@ +/* + * Copyright 2015-2023 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.vintage.engine.samples.junit3; + +import org.junit.runner.RunWith; +import org.junit.runners.Suite; +import org.junit.runners.Suite.SuiteClasses; + +@RunWith(Suite.class) +@SuiteClasses({ IgnoredJUnit3TestCase.class }) +public class JUnit4SuiteWithIgnoredJUnit3TestCase { +} From 103b29dd49a331c9d40c4094928064b69a5eb220 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 5 Sep 2023 13:48:23 +0200 Subject: [PATCH 6/7] Fall back to read Ignore annotation from test class --- .../descriptor/RunnerTestDescriptor.java | 11 ++++----- .../discovery/ClassSelectorResolver.java | 5 ++-- ...fensiveAllDefaultPossibilitiesBuilder.java | 6 ++++- .../engine/execution/RunListenerAdapter.java | 24 +++++++++++++++---- .../engine/execution/TestRunTests.java | 6 +++-- 5 files changed, 35 insertions(+), 17 deletions(-) diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java index af1013dc1d0b..03ae08dd0a6c 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java @@ -21,7 +21,6 @@ import java.util.function.Consumer; import org.apiguardian.api.API; -import org.junit.Ignore; import org.junit.platform.commons.JUnitException; import org.junit.platform.commons.logging.Logger; import org.junit.platform.commons.logging.LoggerFactory; @@ -44,14 +43,14 @@ public class RunnerTestDescriptor extends VintageTestDescriptor { private final Set rejectedExclusions = new HashSet<>(); private Runner runner; - private final Optional skipReason; + private final boolean ignored; private boolean wasFiltered; private List filters = new ArrayList<>(); - public RunnerTestDescriptor(UniqueId uniqueId, Class testClass, Runner runner) { + public RunnerTestDescriptor(UniqueId uniqueId, Class testClass, Runner runner, boolean ignored) { super(uniqueId, runner.getDescription(), testClass.getSimpleName(), ClassSource.from(testClass)); this.runner = runner; - this.skipReason = Optional.ofNullable(testClass.getAnnotation(Ignore.class)).map(Ignore::value); + this.ignored = ignored; } @Override @@ -158,8 +157,8 @@ private Runner getRunnerToReport() { return (runner instanceof RunnerDecorator) ? ((RunnerDecorator) runner).getDecoratedRunner() : runner; } - public Optional getSkipReason() { - return skipReason; + public boolean isIgnored() { + return ignored; } private static class ExcludeDescriptionFilter extends Filter { diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/ClassSelectorResolver.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/ClassSelectorResolver.java index a2d1e7d4c80c..14445f155b4e 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/ClassSelectorResolver.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/ClassSelectorResolver.java @@ -26,7 +26,6 @@ import org.junit.platform.engine.discovery.UniqueIdSelector; import org.junit.platform.engine.support.discovery.SelectorResolver; import org.junit.runner.Runner; -import org.junit.runners.model.RunnerBuilder; import org.junit.vintage.engine.descriptor.RunnerTestDescriptor; /** @@ -34,7 +33,7 @@ */ class ClassSelectorResolver implements SelectorResolver { - private static final RunnerBuilder RUNNER_BUILDER = new DefensiveAllDefaultPossibilitiesBuilder(); + private static final DefensiveAllDefaultPossibilitiesBuilder RUNNER_BUILDER = new DefensiveAllDefaultPossibilitiesBuilder(); private final ClassFilter classFilter; @@ -76,7 +75,7 @@ private Resolution resolveTestClass(Class testClass, Context context) { private RunnerTestDescriptor createRunnerTestDescriptor(TestDescriptor parent, Class testClass, Runner runner) { UniqueId uniqueId = parent.getUniqueId().append(SEGMENT_TYPE_RUNNER, testClass.getName()); - return new RunnerTestDescriptor(uniqueId, testClass, runner); + return new RunnerTestDescriptor(uniqueId, testClass, runner, RUNNER_BUILDER.isIgnored(runner)); } } diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java index 0da94f689fde..b35dc20077c4 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/DefensiveAllDefaultPossibilitiesBuilder.java @@ -64,6 +64,10 @@ public Runner runnerForClass(Class testClass) throws Throwable { return runner; } + boolean isIgnored(Runner runner) { + return runner instanceof IgnoredClassRunner || runner instanceof IgnoringRunnerDecorator; + } + /** * Instead of checking for the {@link Ignore} annotation and returning an * {@link IgnoredClassRunner} from {@link IgnoredBuilder}, we've let the @@ -72,7 +76,7 @@ public Runner runnerForClass(Class testClass) throws Throwable { * override its runtime behavior (i.e. skip execution) but return its * regular {@link org.junit.runner.Description}. */ - private Runner decorateIgnoredTestClass(Runner runner) { + private IgnoringRunnerDecorator decorateIgnoredTestClass(Runner runner) { if (runner instanceof Filterable) { return new FilterableIgnoringRunnerDecorator(runner); } diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java index 88ea197e5604..58144877d04a 100644 --- a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java +++ b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java @@ -20,6 +20,7 @@ import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.TestExecutionResult; import org.junit.platform.engine.UniqueId; +import org.junit.platform.engine.support.descriptor.ClassSource; import org.junit.runner.Description; import org.junit.runner.Result; import org.junit.runner.notification.Failure; @@ -49,7 +50,7 @@ class RunListenerAdapter extends RunListener { @Override public void testRunStarted(Description description) { - if (description.isSuite() && !testRun.getRunnerTestDescriptor().getSkipReason().isPresent()) { + if (description.isSuite() && !testRun.getRunnerTestDescriptor().isIgnored()) { fireExecutionStarted(testRun.getRunnerTestDescriptor(), EventType.REPORTED); } } @@ -66,7 +67,8 @@ public void testSuiteStarted(Description description) { @Override public void testIgnored(Description description) { TestDescriptor testDescriptor = lookupOrRegisterNextTestDescriptor(description); - testIgnored(testDescriptor, determineReasonForIgnoredTest(description).orElse("")); + String reason = determineReasonForIgnoredTest(testDescriptor, description).orElse(""); + testIgnored(testDescriptor, reason); } @Override @@ -177,9 +179,21 @@ private void testIgnored(TestDescriptor testDescriptor, String reason) { fireExecutionSkipped(testDescriptor, reason); } - private Optional determineReasonForIgnoredTest(Description description) { - Optional reason = Optional.ofNullable(description.getAnnotation(Ignore.class)).map(Ignore::value); - return reason.isPresent() ? reason : testRun.getRunnerTestDescriptor().getSkipReason(); + private Optional determineReasonForIgnoredTest(TestDescriptor testDescriptor, Description description) { + Optional reason = getReason(description.getAnnotation(Ignore.class)); + if (reason.isPresent()) { + return reason; + } + // Workaround for some runners (e.g. JUnit38ClassRunner) don't include the @Ignore annotation + // in the description, so we read it from the test class directly + return testDescriptor.getSource() // + .filter(ClassSource.class::isInstance) // + .map(source -> ((ClassSource) source).getJavaClass()) // + .flatMap(testClass -> getReason(testClass.getAnnotation(Ignore.class))); + } + + private static Optional getReason(Ignore annotation) { + return Optional.ofNullable(annotation).map(Ignore::value); } private void dynamicTestRegistered(TestDescriptor testDescriptor) { diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/execution/TestRunTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/execution/TestRunTests.java index 69272fd511db..918e32dffa96 100644 --- a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/execution/TestRunTests.java +++ b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/execution/TestRunTests.java @@ -32,7 +32,8 @@ class TestRunTests { void returnsEmptyOptionalForUnknownDescriptions() throws Exception { Class testClass = PlainJUnit4TestCaseWithSingleTestWhichFails.class; var runnerId = engineId().append(SEGMENT_TYPE_RUNNER, testClass.getName()); - var runnerTestDescriptor = new RunnerTestDescriptor(runnerId, testClass, new BlockJUnit4ClassRunner(testClass)); + var runnerTestDescriptor = new RunnerTestDescriptor(runnerId, testClass, new BlockJUnit4ClassRunner(testClass), + false); var unknownDescription = createTestDescription(testClass, "dynamicTest"); var testRun = new TestRun(runnerTestDescriptor); @@ -45,7 +46,8 @@ void returnsEmptyOptionalForUnknownDescriptions() throws Exception { void registersDynamicTestDescriptors() throws Exception { Class testClass = PlainJUnit4TestCaseWithSingleTestWhichFails.class; var runnerId = engineId().append(SEGMENT_TYPE_RUNNER, testClass.getName()); - var runnerTestDescriptor = new RunnerTestDescriptor(runnerId, testClass, new BlockJUnit4ClassRunner(testClass)); + var runnerTestDescriptor = new RunnerTestDescriptor(runnerId, testClass, new BlockJUnit4ClassRunner(testClass), + false); var dynamicTestId = runnerId.append(SEGMENT_TYPE_DYNAMIC, "dynamicTest"); var dynamicDescription = createTestDescription(testClass, "dynamicTest"); var dynamicTestDescriptor = new VintageTestDescriptor(dynamicTestId, dynamicDescription, null); From 2d50bbd02bd7481c4a5e193f0f01d3d3c3e4f802 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 5 Sep 2023 13:54:57 +0200 Subject: [PATCH 7/7] Add entry to release notes --- .../src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc index f9e994b0bd79..f3186db2b66a 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc @@ -47,7 +47,7 @@ JUnit repository on GitHub. ==== Bug Fixes -* ❓ +* Fix reporting of JUnit 3 test classes with `@Ignored` annotation ==== Deprecations and Breaking Changes