Make error message for cyclic graphs in test discovery more actionable #3052
Conversation
...-launcher/src/main/java/org/junit/platform/launcher/core/EngineDiscoveryResultValidator.java
Outdated
Show resolved
Hide resolved
...-launcher/src/main/java/org/junit/platform/launcher/core/EngineDiscoveryResultValidator.java
Outdated
Show resolved
Hide resolved
...-launcher/src/main/java/org/junit/platform/launcher/core/EngineDiscoveryResultValidator.java
Outdated
Show resolved
Hide resolved
|
Good to go. Can we merge and close it? |
tribbloid
force-pushed
the
verbose-cyclic-msg/squash1
branch
from
c0baf2d to
1a66b58
Compare
| () -> String.format("The discover() method for TestEngine with ID '%s' returned a cyclic graph: " | ||
| + "Cycle found at unique ID '%s'", |
There was a problem hiding this comment.
I think it would be even more helpful to print all unique IDs in the cycle or at least the two between which the cycle was detected (detectInnerClassCycle in ReflectionUtils could be used for inspiration).
@tribbloid Would you have time to look into that?
There was a problem hiding this comment.
Absolutely, that's why I opted to use List instead of Optional in the first place.
I just don't know if it can halt every time, should we add a some kind of depth check?
There was a problem hiding this comment.
Here's an article outlining how to find the shortest cycle in an undirected graph (tests should be a tree, i.e. an acyclic undirected graph): https://www.baeldung.com/cs/shortest-cycle-undirected-graph
There was a problem hiding this comment.
@marcphilipp thanks a lot, I'll add the code to print the first detected path of the cycle (using node.parent()).
But I'm reluctant to change the algorithm from breadth first (the status quo) to depth first (one used in your link), the search will hang for diverging tree without loop (e.g. if you generate a UUID suffix for each new sub-test). Existing code probably chose breadth first for this reason (and some other reasons)
There was a problem hiding this comment.
OK the latest version now print 2 paths that leads to the same node. There are some strange styling errors which shouldn't affect implementation.
All 3 test cases now contains examples of the error messages
There was a problem hiding this comment.
LGTM. Let me know if it looks good or only requires minor revision.
There was a problem hiding this comment.
Now the styling error is also fixed. Only get some deprecation error under JVM 20 that is irrelevant to the scope of this change.
Can we close it now?
tribbloid
force-pushed
the
verbose-cyclic-msg/squash1
branch
3 times, most recently
from
8d6f3e5 to
639ea69
Compare
tribbloid
force-pushed
the
verbose-cyclic-msg/squash1
branch
from
639ea69 to
cec4762
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions. |
|
This pull request has been automatically closed due to inactivity. If you are still interested in contributing this, please ensure that it is rebased against the latest branch (usually |
|
@marcphilipp I don't see any objection on this patch, can we merge and close it any time soon? It has already become stale once |
|
@tribbloid Sorry for the delay! We've deactivated Stalebot for PRs now. This PR needs a proper review from someone on the team. |
marcphilipp
changed the title
|
@tribbloid Thank you! 👍 |
Overview
#3051 error message now contains a list of detected cyclic test UIDs
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
getCyclicUIDs function will return a list containing the first UID that emits a cycle (can be changed to all UIDs, albeit slower)
may still need test case(s)