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.

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

Handle class setup failures preventing previously failed methods from retrying #177

Merged
merged 6 commits into from
Feb 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -279,13 +279,13 @@ Other versions are likely to work as well, but are not tested.
|4.13.2

|JUnit5
|5.8.2
|5.9.2

|Spock
|2.0-groovy-3.0
|2.3-groovy-3.0

|TestNG
|7.4.0
|7.5
|===

=== Parameterized tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public void execute(JvmTestExecutionSpec spec, TestResultProcessor testResultPro

public void failWithNonRetriedTestsIfAny() {
if (extension.getSimulateNotRetryableTest() || hasNonRetriedTests()) {
throw new IllegalStateException("org.gradle.test-retry was unable to retry the following test methods, which is unexpected. Please file a bug report at https://github.com/gradle/test-retry-gradle-plugin/issues" +
throw new IllegalStateException("The following test methods could not be retried, which is unexpected. Please file a bug report at https://github.com/gradle/test-retry-gradle-plugin/issues" +
lastResult.nonRetriedTests.stream()
.flatMap(entry -> entry.getValue().stream().map(methodName -> " " + entry.getKey() + "#" + methodName))
.collect(Collectors.joining("\n", "\n", "\n")));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,21 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
addRetry(className, name);
}

// class-level lifecycle failures do not guarantee that all methods that failed in the previous round will be re-executed (e.g. due to class setup failure)
// in this case, we retry the entire class, so we ignore method-level failures for the next round
// we keep all lifecycle failures from previous round to make sure we report them as passed later on
if (isLifecycleFailure(className, name)) {
previousRoundFailedTests.remove(className, n -> {
if (isLifecycleFailure(className, n)) {
addRetry(className, n);
}
return true;
});
}

if (isClassDescriptor(descriptor)) {
previousRoundFailedTests.remove(className, n -> {
if (testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, n)) {
if (isLifecycleFailure(className, n)) {
emitFakePassedEvent(descriptor, testCompleteEvent, n);
return true;
} else {
Expand All @@ -114,6 +126,10 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
delegate.completed(testId, testCompleteEvent);
}

private boolean isLifecycleFailure(String className, String name) {
return testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, name);
}

private void addRetry(String className, String name) {
if (classRetryMatcher.retryWholeClass(className)) {
currentRoundFailedTests.addClass(className);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ abstract class AbstractPluginFuncTest extends Specification {
}

String markerFileExistsCheck(String id = "id") {
"Files.exists(Paths.get(\"build/marker.file.${StringEscapeUtils.escapeJava(id)}\"))"
"""Files.exists(Paths.get("build/marker.file.${StringEscapeUtils.escapeJava(id)}"))"""
}

String flakyAssertClass() {
Expand All @@ -78,6 +78,24 @@ abstract class AbstractPluginFuncTest extends Specification {
throw new java.io.UncheckedIOException(e);
}
}

public static void flakyAssertPassFailPass(String id) {
Path marker = Paths.get("build/marker.file." + id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Path marker = Paths.get("build/marker.file." + id);
Path marker = Paths.get("build/marker.file.$id");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

try {
if (Files.exists(marker)) {
int counter = Integer.parseInt(new String(Files.readAllBytes(marker)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need the groovy-nio as test dependency for the nice Path extension methods.

Suggested change
int counter = Integer.parseInt(new String(Files.readAllBytes(marker)));
int counter = Integer.parseInt(marker.text));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, it is a plain Java class written alongside test classes.

++counter;
Files.write(marker, Integer.toString(counter).getBytes());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Files.write(marker, Integer.toString(counter).getBytes());
marker.text = counter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗ This is a Java class that we create in the build workspace to simplify creation of test classes, so this won't work.

if (counter == 1) {
throw new RuntimeException("fail me!");
}
} else {
Files.write(marker, "0".getBytes());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Files.write(marker, "0".getBytes());
marker.text = 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

}
} catch (java.io.IOException e) {
throw new java.io.UncheckedIOException(e);
}
}
}
"""
}
Expand Down Expand Up @@ -129,7 +147,11 @@ abstract class AbstractPluginFuncTest extends Specification {
}

static String flakyAssert(String id = "id", int failures = 1) {
return "acme.FlakyAssert.flakyAssert(\"${StringEscapeUtils.escapeJava(id)}\", $failures);"
return """acme.FlakyAssert.flakyAssert("${StringEscapeUtils.escapeJava(id)}", $failures);"""
}

static String flakyAssertPassFailPass(String id = "id") {
return """acme.FlakyAssert.flakyAssertPassFailPass("${StringEscapeUtils.escapeJava(id)}");"""
}

@SuppressWarnings("GroovyAssignabilityCheck")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -590,4 +590,104 @@ class JUnit4FuncTest extends AbstractFrameworkFuncTest {
where:
gradleVersion << GRADLE_VERSIONS_UNDER_TEST
}

def "handles flaky setup that prevents the retries of initially failed methods (gradle version #gradleVersion)"() {
given:
buildFile << """
test.retry.maxRetries = 2
"""

and:
writeJavaTestSource """
package acme;

public class FlakySetupAndMethodTest {
@org.junit.BeforeClass
public static void setup() {
${flakyAssertPassFailPass("setup")}
}

@org.junit.Test
public void flakyTest() {
${flakyAssert("method")}
}

@org.junit.Test
public void successfulTest() {
}
}
"""

when:
def result = gradleRunner(gradleVersion).build()

then:
with(result.output) {
it.count('flakyTest FAILED') == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
it.count('flakyTest PASSED') == 1
it.count('successfulTest PASSED') == 2
}

where:
gradleVersion << GRADLE_VERSIONS_UNDER_TEST
}

def "handles setup failure after cleanup failure (gradle version #gradleVersion)"() {
given:
buildFile << """
test.retry.maxRetries = 2
"""

and:
writeJavaTestSource """
package acme;

public class FlakySetupAndCleanupTest {
@org.junit.BeforeClass
public static void setup() {
${flakyAssertPassFailPass("setup")}
}

@org.junit.AfterClass
public static void cleanup() {
${flakyAssert("cleanup")}
}

@org.junit.Test
public void flakyTest() {
${flakyAssert("method")}
}

@org.junit.Test
public void successfulTest() {
}
}
"""

when:
def result = gradleRunner(gradleVersion).build()

then:
def differentiatesBetweenSetupAndCleanupMethods = beforeClassErrorTestMethodName(gradleVersion) != afterClassErrorTestMethodName(gradleVersion)
with(result.output) {
it.count('flakyTest FAILED') == 1
it.count('flakyTest PASSED') == 1
it.count('successfulTest PASSED') == 2

if (differentiatesBetweenSetupAndCleanupMethods) {
it.count("${afterClassErrorTestMethodName(gradleVersion)} FAILED") == 1
it.count("${afterClassErrorTestMethodName(gradleVersion)} PASSED") == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
} else {
it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 2
it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
}
}

where:
gradleVersion << GRADLE_VERSIONS_UNDER_TEST
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ class JUnit4ViaJUnitVintageFuncTest extends JUnit4FuncTest {
return '''
dependencies {
testImplementation "junit:junit:4.13.2"
testImplementation "org.junit.jupiter:junit-jupiter-api:5.8.2"
testRuntimeOnly "org.junit.vintage:junit-vintage-engine:5.8.2"
testImplementation "org.junit.jupiter:junit-jupiter-api:5.9.2"
testRuntimeOnly "org.junit.vintage:junit-vintage-engine:5.9.2"
}

test {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,106 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
gradleVersion << GRADLE_VERSIONS_UNDER_TEST
}

def "handles flaky setup that prevents the retries of initially failed methods (gradle version #gradleVersion)"() {
given:
buildFile << """
test.retry.maxRetries = 2
"""

and:
writeJavaTestSource """
package acme;

public class FlakySetupAndMethodTest {
@org.junit.jupiter.api.BeforeAll
public static void setup() {
${flakyAssertPassFailPass("setup")}
}

@org.junit.jupiter.api.Test
public void flakyTest() {
${flakyAssert("method")}
}

@org.junit.jupiter.api.Test
public void successfulTest() {
}
}
"""

when:
def result = gradleRunner(gradleVersion).build()

then:
with(result.output) {
it.count('flakyTest() FAILED') == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
it.count('flakyTest() PASSED') == 1
it.count('successfulTest() PASSED') == 2
}

where:
gradleVersion << GRADLE_VERSIONS_UNDER_TEST
}

def "handles setup failure after cleanup failure (gradle version #gradleVersion)"() {
given:
buildFile << """
test.retry.maxRetries = 2
"""

and:
writeJavaTestSource """
package acme;

public class FlakySetupAndMethodTest {
@org.junit.jupiter.api.BeforeAll
public static void setup() {
${flakyAssertPassFailPass("setup")}
}

@org.junit.jupiter.api.AfterAll
public static void cleanup() {
${flakyAssert("cleanup")}
}

@org.junit.jupiter.api.Test
public void flakyTest() {
${flakyAssert("method")}
}

@org.junit.jupiter.api.Test
public void successfulTest() {
}
}
"""

when:
def result = gradleRunner(gradleVersion).build()

then:
def differentiatesBetweenSetupAndCleanupMethods = beforeClassErrorTestMethodName(gradleVersion) != afterClassErrorTestMethodName(gradleVersion)
with(result.output) {
it.count('flakyTest() FAILED') == 1
it.count('flakyTest() PASSED') == 1
it.count('successfulTest() PASSED') == 2

if (differentiatesBetweenSetupAndCleanupMethods) {
it.count("${afterClassErrorTestMethodName(gradleVersion)} FAILED") == 1
it.count("${afterClassErrorTestMethodName(gradleVersion)} PASSED") == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
} else {
it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 2
it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
}
}

where:
gradleVersion << GRADLE_VERSIONS_UNDER_TEST
}

String reportedTestName(String testName) {
testName + "()"
}
Expand All @@ -367,9 +467,9 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
protected String buildConfiguration() {
return """
dependencies {
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'
testImplementation 'org.junit.jupiter:junit-jupiter-params:5.8.2'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.2'
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.9.2'
testImplementation 'org.junit.jupiter:junit-jupiter-params:5.9.2'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.9.2'
}
test {
useJUnitPlatform()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class Spock2FuncTest extends SpockBaseJunit5FuncTest {
protected String buildConfiguration() {
return """
dependencies {
implementation 'org.spockframework:spock-core:2.0-groovy-3.0'
implementation 'org.spockframework:spock-core:2.3-groovy-3.0'
}
test {
useJUnitPlatform()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ abstract class SpockBaseJunit5FuncTest extends SpockFuncTest {
protected String buildConfiguration() {
return """
dependencies {
implementation 'org.spockframework:spock-core:2.0-groovy-3.0'
implementation 'org.spockframework:spock-core:2.3-groovy-3.0'
}
test {
useJUnitPlatform()
Expand Down
Loading