Skip to content

Commit

Permalink
Make error message for cyclic graphs in test discovery more actionable
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@tribbloid @marcphilipp
tribbloid and marcphilipp committed Apr 28, 2023
1 parent f79e38b commit d62feaa
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 23 deletions.
68 changes: 55 additions & 13 deletions ...uncher/src/main/java/org/junit/platform/launcher/core/EngineDiscoveryResultValidator.java
View file
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
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]""");
}

}

0 comments on commit d62feaa

Please sign in to comment.