EngineDiscoveryResultValidator should produce an actionable error message for cyclic TestDescriptor structures
#3051
Comments
|
I'm not part of the JUnit team, but I've written a few test engines.
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. static UniqueId testId(String suiteId, String testName) {
return ENGINE_ID.append(SUITE_TYPE, suiteId).append("test", testName);
}I would suggest the flowing changes:
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: Note: I would also suggest using the
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. |
|
That said, having the error actuality include the cycle it detected would be helpful with debugging and reproduction of implementation problems! |
|
@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 |
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.
So ideally, if the test engine authors did their job well, no one would ever see this error. 😄 |
|
Thx @tribbloid for summoning me and @mpkorstanje for the suggestions.
Well it seems they did not, but let's fix it.
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.
|
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. |
|
Thanks! |
|
You're welcome. Feel free to tag me if you have any questions. |
sbrannen
changed the title
sbrannen
changed the title
EngineDiscoveryResultValidator should produce more verbose, actionable error message
marcphilipp
changed the title
EngineDiscoveryResultValidator should produce more verbose, actionable error messageEngineDiscoveryResultValidator should produce an actionable error message for cyclic TestDescriptor structures
The error message now includes two paths from the engine root to the found `TestDescriptor` in question. Resolves #3051. Co-authored-by: Marc Philipp <mail@marcphilipp.de>
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:
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
EngineDiscoveryResultValidatorshould be modified to push an error message if returning false:This would allow not only tests using ScalaTest runner to be corrected easily, but also tests using other flexible runners
The text was updated successfully, but these errors were encountered: