From 3e13dd871ea057b5bb659ce5ccfd0d034e3951d9 Mon Sep 17 00:00:00 2001 From: Luke Daley Date: Fri, 20 Nov 2020 16:31:51 +1000 Subject: [PATCH] Handle TestNG lifecycle method failures Fixes #78 --- .../internal/executer/RetryTestExecuter.java | 7 +- .../executer/RetryTestResultProcessor.java | 13 ++- .../BaseJunitTestFrameworkStrategy.java | 2 +- .../framework/TestFrameworkStrategy.java | 6 +- .../framework/TestNgClassVisitor.java | 104 ++++++++++++------ .../TestNgTestFrameworkStrategy.java | 65 ++++++++--- .../testframework/TestNGFuncTest.groovy | 13 ++- 7 files changed, 147 insertions(+), 63 deletions(-) diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java index 97236a9a..f37888cc 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java @@ -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; diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java index 103e0cb6..c6829cb3 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java @@ -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; @@ -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; } @@ -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 { diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java index d0a3ae50..e1cb2209 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java @@ -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); } diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java index caf5c13a..fdefe070 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java @@ -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) { @@ -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); diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgClassVisitor.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgClassVisitor.java index 8a75a211..ae7a0fb3 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgClassVisitor.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgClassVisitor.java @@ -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; @@ -30,50 +32,82 @@ import static org.objectweb.asm.Opcodes.ASM7; -final class TestNgClassVisitor extends TestsReader.Visitor { +final class TestNgClassVisitor extends TestsReader.Visitor { - private final Map> dependsOn = new HashMap<>(); - private final Map> dependedOn = new HashMap<>(); + private static final List 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> dependsOn = new HashMap<>(); + private final Map> dependedOn = new HashMap<>(); + private final Set lifecycleMethods = new HashSet<>(); + + private String superClass; + + Set dependsOn(String method) { + Set dependentChain = new HashSet<>(); + + List 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 dependsOn(String method) { - Set dependentChain = new HashSet<>(); + return dependentChain; + } - List 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 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); } @@ -81,7 +115,9 @@ public TestNGMethodVisitor() { @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; } @@ -89,6 +125,8 @@ public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { private final class TestNGTestAnnotationVisitor extends AnnotationVisitor { + private final TestNGTestDependsOnAnnotationVisitor dependsOnAnnotationVisitor = new TestNGTestDependsOnAnnotationVisitor(); + public TestNGTestAnnotationVisitor() { super(ASM7); } @@ -96,7 +134,7 @@ public TestNGTestAnnotationVisitor() { @Override public AnnotationVisitor visitArray(String name) { if ("dependsOnMethods".equals(name)) { - return new TestNGTestDependsOnAnnotationVisitor(); + return dependsOnAnnotationVisitor; } return null; } @@ -110,7 +148,7 @@ 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<>(); } @@ -118,7 +156,7 @@ public void visit(String name, Object value) { return acc; }); - dependedOn.compute((String) value, (m, acc) -> { + classInfo.dependedOn.compute((String) value, (m, acc) -> { if (acc == null) { acc = new ArrayList<>(); } diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java index 6b75a28f..deffca5e 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java @@ -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> 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 @@ -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 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 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 getClassInfo(TestsReader testsReader, String className) { + return classInfoCache.computeIfAbsent(className, ignored -> { + Optional 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("\\[[^)]+](\\([^)]*\\))+$", ""); } diff --git a/plugin/src/test/groovy/org/gradle/testretry/testframework/TestNGFuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/testframework/TestNGFuncTest.groovy index f74b425e..fcfbb416 100644 --- a/plugin/src/test/groovy/org/gradle/testretry/testframework/TestNGFuncTest.groovy +++ b/plugin/src/test/groovy/org/gradle/testretry/testframework/TestNGFuncTest.groovy @@ -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 @@ -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