Fix reporting of ignored JUnit 3 test classes (#3427)

Prior to this commit, `@Ignore`-annotated JUnit 3 test classes where
reported as started and then as skipped due to their `Description` not
including class-level annotations in some case.

Co-authored-by: Marc Philipp <[email protected]>

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
 

junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/RunnerTestDescriptor.java

@@ -43,12 +43,14 @@ public class RunnerTestDescriptor extends VintageTestDescriptor {
 
 	private final Set<Description> rejectedExclusions = new HashSet<>();
 	private Runner runner;
+	private final boolean ignored;
 	private boolean wasFiltered;
 	private List<Filter> 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.ignored = ignored;
 	}
 
 	@Override
@@ -155,6 +157,10 @@ private Runner getRunnerToReport() {
 		return (runner instanceof RunnerDecorator) ? ((RunnerDecorator) runner).getDecoratedRunner() : runner;
 	}
 
+	public boolean isIgnored() {
+		return ignored;
+	}
+
 	private static class ExcludeDescriptionFilter extends Filter {
 
 		private final Description description;

junit-vintage-engine/src/main/java/org/junit/vintage/engine/discovery/ClassSelectorResolver.java

@@ -26,15 +26,14 @@
 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;
 
 /**
  * @since 4.12
  */
 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));
 	}
 
 }

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);
 		}

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() && description.getAnnotation(Ignore.class) == null) {
+		if (description.isSuite() && !testRun.getRunnerTestDescriptor().isIgnored()) {
 			fireExecutionStarted(testRun.getRunnerTestDescriptor(), EventType.REPORTED);
 		}
 	}
@@ -65,7 +66,9 @@ public void testSuiteStarted(Description description) {
 
 	@Override
 	public void testIgnored(Description description) {
-		testIgnored(lookupOrRegisterNextTestDescriptor(description), determineReasonForIgnoredTest(description));
+		TestDescriptor testDescriptor = lookupOrRegisterNextTestDescriptor(description);
+		String reason = determineReasonForIgnoredTest(testDescriptor, description).orElse("<unknown>");
+		testIgnored(testDescriptor, reason);
 	}
 
 	@Override
@@ -176,9 +179,21 @@ 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("<unknown>");
+	private Optional<String> determineReasonForIgnoredTest(TestDescriptor testDescriptor, Description description) {
+		Optional<String> 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<String> getReason(Ignore annotation) {
+		return Optional.ofNullable(annotation).map(Ignore::value);
 	}
 
 	private void dynamicTestRegistered(TestDescriptor testDescriptor) {

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;
@@ -56,8 +57,10 @@
 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.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;
@@ -896,6 +899,27 @@ void executesRegularSpockFeatureMethod() {
 			event(engine(), finishedSuccessfully()));
 	}
 
+	@Test
+	void executesIgnoredJUnit3TestCase() {
+		var suiteClass = IgnoredJUnit3TestCase.class;
+		execute(suiteClass).allEvents().assertEventsMatchExactly( //
+			event(engine(), started()), //
+			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()));
+	}
+
 	private static EngineExecutionResults execute(Class<?> testClass) {
 		return execute(request(testClass));
 	}

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);

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("testing")
+public class IgnoredJUnit3TestCase extends TestCase {
+
+	public void test() {
+		Assert.fail("this test should be ignored");
+	}
+
+}

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 {
+}