MUnitRunner invokes fireTestStarted even when a test is skipped causing JUnit compatibility issues

[rpalcolea]

Hi folks,

We have seen some usage of munit with Scala based gradle projects lately and part of our work is to enable Gradle Test Distribution for them. In a nutshell, this feature requires junit platform.

One of our users reported an issue when running tests that use munit.

After some digging, I came up with a reproducer in https://github.com/rpalcolea/scala-munit-test-distribution

Given a test like this:

test("myTest2".ignore) {
    assertEquals(true, true)
  }

Using munit + org.junit.vintage:junit-vintage-engine + Gradle test features , results in:

Caused by: java.lang.NullPointerException
        at org.gradle.api.internal.tasks.testing.operations.TestListenerBuildOperationAdapter.completed(TestListenerBuildOperationAdapter.java:72)
        at jdk.internal.reflect.GeneratedMethodAccessor100.invoke(Unknown Source)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        

After some digging with the help of @marcphilipp, we found that the problem comes from

munit/munit/shared/src/main/scala/munit/MUnitRunner.scala

Lines 282 to 286 in 0ec2259

	notifier.fireTestStarted(description)
	if (test.tags(Ignore)) {
	notifier.fireTestIgnored(description)
	return Future.successful(false)
	}

notifier.fireTestStarted(description)
    if (test.tags(Ignore)) {
      notifier.fireTestIgnored(description)
      return Future.successful(false)
    }

Ideally, fireTestStarted should not be called before fireTestIgnored. That’s a violation of JUnit 4's RunListener interface. Any call to testStarted must be followed by a call to testFinished. For ignored tests only testIgnored must be called.

It seems like a small change to do