Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make error message for cyclic graphs in test discovery more actionable #3052

Merged
merged 8 commits into from
Apr 28, 2023
Merged

Make error message for cyclic graphs in test discovery more actionable #3052

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f1ca56
add more verbose error message for cyclic UID in discovery
tribbloid Oct 5, 2022
068c5bb
getFirstCyclicUID now returns optional
tribbloid Oct 6, 2022
cec4762
Cyclic graph error now reports 2 paths that leads to the cycle
tribbloid Nov 11, 2022
e9aa670
spotlessApply
tribbloid Nov 11, 2022
b2835fe
avoided import .*
tribbloid Nov 11, 2022
787b50b
Merge branch 'main' into verbose-cyclic-msg/squash1
marcphilipp Apr 28, 2023
60b8330
Polishing
marcphilipp Apr 28, 2023
f1951eb
Use platform-agnostic line separators
marcphilipp Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 55 additions & 13 deletions ...uncher/src/main/java/org/junit/platform/launcher/core/EngineDiscoveryResultValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,15 @@

package org.junit.platform.launcher.core;

import static java.util.stream.Collectors.joining;

import java.util.ArrayDeque;
import java.util.HashSet;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Queue;
import java.util.Set;

import org.junit.platform.commons.util.Preconditions;
import org.junit.platform.engine.TestDescriptor;
Expand All @@ -37,30 +42,67 @@ void validate(TestEngine testEngine, TestDescriptor root) {
() -> String.format(
"The discover() method for TestEngine with ID '%s' must return a non-null root TestDescriptor.",
testEngine.getId()));
Preconditions.condition(isAcyclic(root),
() -> String.format("The discover() method for TestEngine with ID '%s' returned a cyclic graph.",
testEngine.getId()));
Optional<String> cyclicGraphInfo = getCyclicGraphInfo(root);
Preconditions.condition(!cyclicGraphInfo.isPresent(),
() -> String.format("The discover() method for TestEngine with ID '%s' returned a cyclic graph; %s",
testEngine.getId(), cyclicGraphInfo.get()));
}

/**
* @return {@code true} if the tree does <em>not</em> contain a cycle; else {@code false}.
* @return non-empty {@link Optional} if the tree contains a cycle
*/
boolean isAcyclic(TestDescriptor root) {
Set<UniqueId> visited = new HashSet<>();
visited.add(root.getUniqueId());
private Optional<String> getCyclicGraphInfo(TestDescriptor root) {

Map<UniqueId, Optional<UniqueId>> visited = new HashMap<>();
visited.put(root.getUniqueId(), Optional.empty());

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!
TestDescriptor parent = queue.remove();
for (TestDescriptor child : parent.getChildren()) {
UniqueId uid = child.getUniqueId();
if (visited.containsKey(uid)) { // id already known: cycle detected!

List<UniqueId> path1 = findPath(visited, uid);
List<UniqueId> path2 = findPath(visited, parent.getUniqueId());
path2.add(uid);

return Optional.of(String.format("%s exists in at least two paths:%n(1) %s%n(2) %s", uid,
formatted(path1), formatted(path2)));
}
else {
visited.put(uid, Optional.of(parent.getUniqueId()));
}
if (child.isContainer()) {
queue.add(child);
}
}
}
return true;
return Optional.empty();
}

private String formatted(List<UniqueId> path) {
return path.stream().map(UniqueId::toString).collect(joining(" -> "));
}

private static List<UniqueId> findPath(Map<UniqueId, Optional<UniqueId>> visited, UniqueId target) {
List<UniqueId> path = new ArrayList<>();
path.add(target);
UniqueId current = target;

while (visited.containsKey(current)) {
Optional<UniqueId> backTraced = visited.get(current);
if (backTraced.isPresent()) {
path.add(0, backTraced.get());
current = backTraced.get();
}
else {
break;
}
}
return path;
}

}
42 changes: 32 additions & 10 deletions ...s/src/test/java/org/junit/platform/launcher/core/EngineDiscoveryResultValidatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,38 @@

package org.junit.platform.launcher.core;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import org.junit.jupiter.api.Test;
import org.junit.platform.commons.PreconditionViolationException;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.TestEngine;
import org.junit.platform.engine.UniqueId;
import org.junit.platform.fakes.TestDescriptorStub;
import org.junit.platform.fakes.TestEngineStub;

/**
* @since 1.3
*/
class EngineDiscoveryResultValidatorTests {

private final TestEngine testEngine = new TestEngineStub("my-engine");
private final EngineDiscoveryResultValidator validator = new EngineDiscoveryResultValidator();

@Test
void detectCycleWithDoubleRoot() {

var root = new TestDescriptorStub(UniqueId.forEngine("root"), "root");
assertTrue(validator.isAcyclic(root));
validator.validate(testEngine, root);

root.addChild(root);
assertFalse(validator.isAcyclic(root));
assertThatThrownBy(() -> validator.validate(testEngine, root)) //
.isInstanceOf(PreconditionViolationException.class) //
.hasMessage(
"""
The discover() method for TestEngine with ID 'my-engine' returned a cyclic graph; [engine:root] exists in at least two paths:
(1) [engine:root]
(2) [engine:root] -> [engine:root]""");
}

@Test
Expand All @@ -42,10 +52,16 @@ void detectCycleWithDoubleGroup() {
TestDescriptor group2 = new TestDescriptorStub(rootId.append("group", "2"), "2");
root.addChild(group1);
root.addChild(group2);
assertTrue(validator.isAcyclic(root));
validator.validate(testEngine, root);

group2.addChild(group1);
assertFalse(validator.isAcyclic(root));
assertThatThrownBy(() -> validator.validate(testEngine, root)) //
.isInstanceOf(PreconditionViolationException.class) //
.hasMessage(
"""
The discover() method for TestEngine with ID 'my-engine' returned a cyclic graph; [engine:root]/[group:1] exists in at least two paths:
(1) [engine:root] -> [engine:root]/[group:1]
(2) [engine:root] -> [engine:root]/[group:2] -> [engine:root]/[group:1]""");
}

@Test
Expand All @@ -56,14 +72,20 @@ void detectCycleWithDoubleTest() {
TestDescriptor group2 = new TestDescriptorStub(rootId.append("group", "2"), "2");
root.addChild(group1);
root.addChild(group2);
TestDescriptor test1 = new TestDescriptorStub(rootId.append("test", "1"), "1-1");
TestDescriptor test2 = new TestDescriptorStub(rootId.append("test", "2"), "2-2");
TestDescriptor test1 = new TestDescriptorStub(group1.getUniqueId().append("test", "1"), "1-1");
TestDescriptor test2 = new TestDescriptorStub(group2.getUniqueId().append("test", "2"), "2-2");
group1.addChild(test1);
group2.addChild(test2);
assertTrue(validator.isAcyclic(root));
validator.validate(testEngine, root);

group2.addChild(test1);
assertFalse(validator.isAcyclic(root));
assertThatThrownBy(() -> validator.validate(testEngine, root)) //
.isInstanceOf(PreconditionViolationException.class) //
.hasMessage(
"""
The discover() method for TestEngine with ID 'my-engine' returned a cyclic graph; [engine:root]/[group:1]/[test:1] exists in at least two paths:
(1) [engine:root] -> [engine:root]/[group:1] -> [engine:root]/[group:1]/[test:1]
(2) [engine:root] -> [engine:root]/[group:2] -> [engine:root]/[group:1]/[test:1]""");
}

}