Skip to content

Commit

Permalink
Handle TestNG lifecycle method failures
Browse files Browse the repository at this point in the history 
Fixes #78
  • Loading branch information
@ldaley
ldaley committed Nov 20, 2020
1 parent b093a11 commit 3e13dd8
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 63 deletions.
7 changes: 6 additions & 1 deletion plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ public void execute(JvmTestExecutionSpec spec, TestResultProcessor testResultPro

TestFrameworkStrategy testFrameworkStrategy = TestFrameworkStrategy.of(spec.getTestFramework());

RetryTestResultProcessor retryTestResultProcessor = new RetryTestResultProcessor(testFrameworkStrategy, testResultProcessor, maxFailures);
RetryTestResultProcessor retryTestResultProcessor = new RetryTestResultProcessor(
testFrameworkStrategy,
frameworkTemplate.testsReader,
testResultProcessor,
maxFailures
);

int retryCount = 0;
JvmTestExecutionSpec testExecutionSpec = spec;
Expand Down
13 changes: 10 additions & 3 deletions plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
final class RetryTestResultProcessor implements TestResultProcessor {

private final TestFrameworkStrategy testFrameworkStrategy;
private final TestsReader testsReader;
private final TestResultProcessor delegate;

private final int maxFailures;
Expand All @@ -42,8 +43,14 @@ final class RetryTestResultProcessor implements TestResultProcessor {

private Object rootTestDescriptorId;

RetryTestResultProcessor(TestFrameworkStrategy testFrameworkStrategy, TestResultProcessor delegate, int maxFailures) {
RetryTestResultProcessor(
TestFrameworkStrategy testFrameworkStrategy,
TestsReader testsReader,
TestResultProcessor delegate,
int maxFailures
) {
this.testFrameworkStrategy = testFrameworkStrategy;
this.testsReader = testsReader;
this.delegate = delegate;
this.maxFailures = maxFailures;
}
Expand Down Expand Up @@ -78,8 +85,8 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
}

if (isClassDescriptor(descriptor)) {
previousRoundFailedTests.remove(descriptor.getClassName(), n -> {
if (testFrameworkStrategy.isSyntheticFailure(n)) {
previousRoundFailedTests.remove(className, n -> {
if (testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, n)) {
emitFakePassedEvent(descriptor, testCompleteEvent, n);
return true;
} else {
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ abstract class BaseJunitTestFrameworkStrategy implements TestFrameworkStrategy {
);

@Override
public boolean isSyntheticFailure(String testName) {
public boolean isLifecycleFailureTest(TestsReader testsReader, String className, String testName) {
return ERROR_SYNTHETIC_TEST_NAMES.contains(testName);
}

Expand Down
6 changes: 5 additions & 1 deletion ...src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@
import org.gradle.api.internal.tasks.testing.testng.TestNGTestFramework;
import org.gradle.testretry.internal.executer.TestFrameworkTemplate;
import org.gradle.testretry.internal.executer.TestNames;
import org.gradle.testretry.internal.executer.TestsReader;
import org.gradle.util.GradleVersion;

/**
* Instances are scoped to a test task execution and are reused between rounds.
*/
public interface TestFrameworkStrategy {

static TestFrameworkStrategy of(TestFramework testFramework) {
Expand All @@ -41,7 +45,7 @@ static boolean gradleVersionIsAtLeast(String version) {
return GradleVersion.current().getBaseVersion().compareTo(GradleVersion.version(version)) >= 0;
}

boolean isSyntheticFailure(String testName);
boolean isLifecycleFailureTest(TestsReader testsReader, String className, String testName);

TestFramework createRetrying(TestFrameworkTemplate template, TestNames failedTests);

Expand Down
104 changes: 71 additions & 33 deletions ...in/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgClassVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import org.objectweb.asm.AnnotationVisitor;
import org.objectweb.asm.MethodVisitor;

import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
Expand All @@ -30,73 +32,109 @@

import static org.objectweb.asm.Opcodes.ASM7;

final class TestNgClassVisitor extends TestsReader.Visitor<TestNgClassVisitor> {
final class TestNgClassVisitor extends TestsReader.Visitor<TestNgClassVisitor.ClassInfo> {

private final Map<String, List<String>> dependsOn = new HashMap<>();
private final Map<String, List<String>> dependedOn = new HashMap<>();
private static final List<String> LIFECYCLE_ANNOTATION_DESCRIPTORS = Arrays.asList(
"Lorg/testng/annotations/BeforeClass;",
"Lorg/testng/annotations/BeforeTest;",
"Lorg/testng/annotations/AfterTest;",
"Lorg/testng/annotations/AfterClass;"
);

private String currentMethod;

@Override
public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) {
this.currentMethod = name;
return new TestNGMethodVisitor();
}
private final ClassInfo classInfo = new ClassInfo();
private final TestNGMethodVisitor methodVisitor = new TestNGMethodVisitor();

@Override
public TestNgClassVisitor getResult() {
return this;
}
final static class ClassInfo {

private final Map<String, List<String>> dependsOn = new HashMap<>();
private final Map<String, List<String>> dependedOn = new HashMap<>();
private final Set<String> lifecycleMethods = new HashSet<>();

private String superClass;

Set<String> dependsOn(String method) {
Set<String> dependentChain = new HashSet<>();

List<String> search = Collections.singletonList(method);
while (!search.isEmpty()) {
search = search.stream()
.flatMap(upstream -> dependsOn.getOrDefault(upstream, Collections.emptyList()).stream())
.filter(upstream -> !dependentChain.contains(upstream))
.collect(Collectors.toList());
dependentChain.addAll(search);
}

search = Collections.singletonList(method);
while (!search.isEmpty()) {
search = search.stream()
.flatMap(downstream -> dependedOn.getOrDefault(downstream, Collections.emptyList()).stream())
.filter(downstream -> !dependentChain.contains(downstream))
.collect(Collectors.toList());
dependentChain.addAll(search);
}

Set<String> dependsOn(String method) {
Set<String> dependentChain = new HashSet<>();
return dependentChain;
}

List<String> search = Collections.singletonList(method);
while (!search.isEmpty()) {
search = search.stream()
.flatMap(upstream -> dependsOn.getOrDefault(upstream, Collections.emptyList()).stream())
.filter(upstream -> !dependentChain.contains(upstream))
.collect(Collectors.toList());
dependentChain.addAll(search);
@Nullable
public String getSuperClass() {
return superClass;
}

search = Collections.singletonList(method);
while (!search.isEmpty()) {
search = search.stream()
.flatMap(downstream -> dependedOn.getOrDefault(downstream, Collections.emptyList()).stream())
.filter(downstream -> !dependentChain.contains(downstream))
.collect(Collectors.toList());
dependentChain.addAll(search);
public Set<String> getLifecycleMethods() {
return lifecycleMethods;
}
}

return dependentChain;
@Override
public ClassInfo getResult() {
return classInfo;
}

@Override
public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) {
this.currentMethod = name;
return methodVisitor;
}

@Override
public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) {
classInfo.superClass = superName.replace('/', '.');
}

private final class TestNGMethodVisitor extends MethodVisitor {

private final TestNGTestAnnotationVisitor testAnnotationVisitor = new TestNGTestAnnotationVisitor();

public TestNGMethodVisitor() {
super(ASM7);
}

@Override
public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
if (descriptor.contains("org/testng/annotations/Test")) {
return new TestNGTestAnnotationVisitor();
return testAnnotationVisitor;
} else if (LIFECYCLE_ANNOTATION_DESCRIPTORS.contains(descriptor)) {
classInfo.lifecycleMethods.add(currentMethod);
}
return null;
}
}

private final class TestNGTestAnnotationVisitor extends AnnotationVisitor {

private final TestNGTestDependsOnAnnotationVisitor dependsOnAnnotationVisitor = new TestNGTestDependsOnAnnotationVisitor();

public TestNGTestAnnotationVisitor() {
super(ASM7);
}

@Override
public AnnotationVisitor visitArray(String name) {
if ("dependsOnMethods".equals(name)) {
return new TestNGTestDependsOnAnnotationVisitor();
return dependsOnAnnotationVisitor;
}
return null;
}
Expand All @@ -110,15 +148,15 @@ public TestNGTestDependsOnAnnotationVisitor() {

@Override
public void visit(String name, Object value) {
dependsOn.compute(currentMethod, (m, acc) -> {
classInfo.dependsOn.compute(currentMethod, (m, acc) -> {
if (acc == null) {
acc = new ArrayList<>();
}
acc.add((String) value);
return acc;
});

dependedOn.compute((String) value, (m, acc) -> {
classInfo.dependedOn.compute((String) value, (m, acc) -> {
if (acc == null) {
acc = new ArrayList<>();
}
Expand Down
65 changes: 47 additions & 18 deletions ...in/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,34 @@

import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

final class TestNgTestFrameworkStrategy implements TestFrameworkStrategy {

private static final Logger LOGGER = LoggerFactory.getLogger(TestNgTestFrameworkStrategy.class);

private final Map<String, Optional<TestNgClassVisitor.ClassInfo>> classInfoCache = new HashMap<>();

@Override
public boolean isSyntheticFailure(String testName) {
return false;
public boolean isLifecycleFailureTest(TestsReader testsReader, String className, String testName) {
return getClassInfo(testsReader, className)
.map(classInfo -> isLifecycleMethod(testsReader, testName, classInfo))
.orElse(false);
}

private boolean isLifecycleMethod(TestsReader testsReader, String testName, TestNgClassVisitor.ClassInfo classInfo) {
if (classInfo.getLifecycleMethods().contains(testName)) {
return true;
} else {
String superClass = classInfo.getSuperClass();
if (superClass == null || superClass.equals("java.lang.Object")) {
return false;
} else {
return isLifecycleFailureTest(testsReader, superClass, testName);
}
}
}

@Override
Expand Down Expand Up @@ -93,26 +111,37 @@ private void addFilters(TestsReader testsReader, TestNames failedTests, TestFilt
failedTests.stream().forEach(entry -> {
String className = entry.getKey();
entry.getValue().forEach(test -> {
Optional<TestNgClassVisitor> classVisitor;
try {
classVisitor = testsReader.readTestClassDirClass(className, TestNgClassVisitor::new);
} catch (Throwable t) {
LOGGER.warn("Unable to determine if class " + className + " has TestNG dependent tests", t);
classVisitor = Optional.empty();
Optional<TestNgClassVisitor.ClassInfo> classInfoOpt = getClassInfo(testsReader, className);
if (classInfoOpt.isPresent()) {
TestNgClassVisitor.ClassInfo classInfo = classInfoOpt.get();
if (isLifecycleMethod(testsReader, test, classInfo)) {
filters.clazz(className);
} else {
String parameterlessName = stripParameters(test);
filters.test(className, parameterlessName);
classInfo.dependsOn(parameterlessName)
.forEach(methodName -> filters.test(className, methodName));
}
} else {
filters.clazz(className);
}

String parameterlessName = stripParameters(test);
filters.test(className, parameterlessName);

classVisitor
.ifPresent(visitor ->
visitor.dependsOn(parameterlessName)
.forEach(methodName -> filters.test(className, methodName))
);
});
});
}

private Optional<TestNgClassVisitor.ClassInfo> getClassInfo(TestsReader testsReader, String className) {
return classInfoCache.computeIfAbsent(className, ignored -> {
Optional<TestNgClassVisitor.ClassInfo> classInfoOpt;
try {
classInfoOpt = testsReader.readTestClassDirClass(className, TestNgClassVisitor::new);
} catch (Throwable t) {
LOGGER.warn("Unable to determine if class " + className + " has TestNG dependent tests", t);
classInfoOpt = Optional.empty();
}
return classInfoOpt;
});
}

private static String stripParameters(String testMethodName) {
return testMethodName.replaceAll("\\[[^)]+](\\([^)]*\\))+$", "");
}
Expand Down
13 changes: 7 additions & 6 deletions plugin/src/test/groovy/org/gradle/testretry/testframework/TestNGFuncTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class TestNGFuncTest extends AbstractFrameworkFuncTest {
}

@Unroll
def "does not handle failure in #lifecycle (gradle version #gradleVersion)"() {
def "handles failure in #lifecycle (gradle version #gradleVersion)"() {
given:
buildFile << """
test.retry.maxRetries = 1
Expand All @@ -52,18 +52,19 @@ class TestNGFuncTest extends AbstractFrameworkFuncTest {
"""

when:
def result = gradleRunner(gradleVersion as String).buildAndFail()
def result = gradleRunner(gradleVersion as String).build()

then:
result.output.count('lifecycle FAILED') >= 1
result.output.contains("org.gradle.test-retry was unable to retry")
result.output.contains("acme.SuccessfulTests#lifecycle")
result.output.count('lifecycle FAILED') == 1
result.output.count('lifecycle PASSED') == 1
!result.output.contains("org.gradle.test-retry was unable to retry")

where:
[gradleVersion, lifecycle] << GroovyCollections.combinations((Iterable) [
GRADLE_VERSIONS_UNDER_TEST,
['BeforeClass', 'BeforeTest', 'AfterClass', 'AfterTest', 'BeforeSuite', 'AfterSuite']
['BeforeClass', 'BeforeTest', 'AfterClass', 'AfterTest']
])
// Note: we don't handle BeforeSuite AfterSuite
}

@Unroll
Expand Down

0 comments on commit 3e13dd8

Please sign in to comment.