`EngineDiscoveryResultValidator` should produce an actionable error message for cyclic TestDescriptor structures

[tribbloid]

This is the root cause of the problem reported in this ticket:

helmethair-co/scalatest-junit-runner#83

Due to the flexibility of ScalaTest, A nested suite may cause several test cases to use the same ID, ScalaTest native runner can run it, but JUnit cannot. Instead it will throw a very brief error like this:

failed to execute tests

org.gradle.api.internal.tasks.testing.TestSuiteExecutionException: Could not complete execution for Gradle Test Executor 1.
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:63)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: org.junit.platform.commons.JUnitException: TestEngine with ID 'scalatest' failed to discover tests
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:111)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:85)
	at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:92)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:75)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	... 18 more
Caused by: org.junit.platform.commons.PreconditionViolationException: The discover() method for TestEngine with ID 'scalatest' returned a cyclic graph.
	at org.junit.platform.commons.util.Preconditions.condition(Preconditions.java:296)
	at org.junit.platform.launcher.core.EngineDiscoveryResultValidator.validate(EngineDiscoveryResultValidator.java:40)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:104)
	... 25 more

This error information gave no explicit action to identify the root cause and fix it.

Ideally, the error information should tell the exact path of the tree and its ID that have caused the cycle.

Deliverables

• The following function under EngineDiscoveryResultValidator should be modified to push an error message if returning false:

	boolean isAcyclic(TestDescriptor root) {
		Set<UniqueId> visited = new HashSet<>();
		visited.add(root.getUniqueId());
		Queue<TestDescriptor> queue = new ArrayDeque<>();
		queue.add(root);
		while (!queue.isEmpty()) {
			for (TestDescriptor child : queue.remove().getChildren()) {
				if (!visited.add(child.getUniqueId())) {
					return false; // id already known: cycle detected!
				}
				if (child.isContainer()) {
					queue.add(child);
				}
			}
		}
		return true;
	}

This would allow not only tests using ScalaTest runner to be corrected easily, but also tests using other flexible runners

[mpkorstanje]

I'm not part of the JUnit team, but I've written a few test engines.

A nested suite may cause several test cases to use the same ID

JUnit 5 uses a tree of tests and containers where each has a unique ID. This ID allows individual tests to be uniquely selected - for example for rerunning a failed test. So under no circumstances should any two distinct tests have the same unique ID.

It is up to the Scala Test Engine to construct the tree in such a way that all tests have a unique ID regardless of how a user might structure their tests. As such this error suggests a problem in the implementation of the Scala Test Engine and doesn't have to be actionable by a user.

Looking at https://github.com/helmethair-co/scalatest-junit-runner/blob/main/src/main/java/co/helmethair/scalatest/descriptor/ScalatestDescriptor.java

    static UniqueId testId(String suiteId, String testName) {
        return ENGINE_ID.append(SUITE_TYPE, suiteId).append("test", testName);
    }

I would suggest the flowing changes:

1. Do not append to the ENGINE_ID but use the UniqueId provided with the TestEngine.discover method as the root. Test engines may be used by the JUnit Platform Suite Engine and may not be the root engine in their own discovery request.

2. Ensure that when a sequence of tests is used, this sequence is part of the UniqueId.

class DummySpec extends AnyFunSpec {

  case class Child() extends AnyFunSpec {
    it("test 1") {}
  }

  object C1 extends Child()
  object C2 extends Child()

  override val nestedSuites: immutable.IndexedSeq[Suite] = {

    immutable.IndexedSeq(C1, C2)
  }
}

So here I would expect the following identifiers:

engine:scala-test
engine:scala-test / suite:DummySpec
engine:scala-test / suite:DummySpec / nestedSuite:0 / test:test 1
engine:scala-test / suite:DummySpec / nestedSuite:1 / test:test 1

Note: I would also suggest using the DummySpecs FQN instead but I left that out for brevity.

3. Take care around user input:

class DummySpec extends AnyFunSpec {
  it("test 1") {}
  it("test 1") {}
}

Instead of using "test 1" as the identfier for both tests consider either referencing these tests by the order of their declaration (or finding another way to make them unique).

Note that while the ID must be unique, there is no such constraint on the name that will be shown to users. Additionally tests can be filtered by their name independently from their ID. So repeatedly naming tests "test 1" is perfectly fine as long as their IDs are unique.

[mpkorstanje]

That said, having the error actuality include the cycle it detected would be helpful with debugging and reproduction of implementation problems!

[tribbloid]

@mpkorstanje thanks a lot, I'm not in the core team of both projects either :)

My point is that error messages like this can confuse both test engine writers and QA engineers alike. A serial UID may be difficult in some context (e.g. in distributed computing, or retrying on volatile smoke tests). Anything that makes debugging easier will help fixing or circumventing such blockers.

Update 1

In fact, adding the error message is a trivial change, even I can do this. Will do this shortly

Update 2

PR draft submitted as #3052

[mpkorstanje]

A serial UID may be difficult in some context (e.g. in distributed computing, or retrying on volatile smoke tests).

I don't think that is necessary. There is plenty of "unique enough" structure available to map Scala tests to JUnit without issue. It's just that the current mapping implementation is extremely naive.

My point is that error messages like this can confuse both test engine writers and QA engineers alike.

So ideally, if the test engine authors did their job well, no one would ever see this error. 😄

[tribbloid]

I'm not qualified enough to judge the engine, but I'll try to get @giurim 's attention to see how feasible it is.

I prefer to have the best of both, human make mistakes: #3052

[giurim]

Thx @tribbloid for summoning me and @mpkorstanje for the suggestions.

So ideally, if the test engine authors did their job well, no one would ever see this error. 😄

Well it seems they did not, but let's fix it.

the current mapping implementation is extremely naive

True, will figure it out something. Suggestions/links to other test engine implementations are welcome, but will do my research. We can use the ID Scalatest have internally it is not descriptive at all, but unique.

Do not append to the ENGINE_ID but use the UniqueId provided with the TestEngine.discover method as the root. Test engines may be used by the JUnit Platform Suite Engine and may not be the root engine in their own discovery request.

I am not sure what do you mean, can you elaborate? WIll fix.

[mpkorstanje]

Suggestions/links to other test engine implementations are welcome, but will do my research. We can use the ID Scalatest have internally it is not descriptive at all, but unique.

IDs don't have to be descriptive. The IDs are not typically exposed to a user. Though it is certainly nice when debugging.

If the IDs are deterministic and somewhat stable under small changes to changes to the suite they're good candidate to use. This is important for IDEs and other tools that want to (re-)run a specific test. They'll select it by ID.

[giurim]

Thanks!

[mpkorstanje]

You're welcome. Feel free to tag me if you have any questions.