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

JUnit rule for flaky test retry #1680

Merged
merged 19 commits into from Aug 6, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
38 changes: 21 additions & 17 deletions build.gradle
Expand Up @@ -12,8 +12,6 @@ subprojects {
apply plugin: 'idea'
apply plugin: 'io.franzbecker.gradle-lombok'
apply plugin: 'com.github.johnrengelman.shadow'
apply from: "$rootDir/gradle/publishing.gradle"
apply from: "$rootDir/gradle/bintray.gradle"

group = "org.testcontainers"

Expand All @@ -35,9 +33,28 @@ subprojects {
}
}

project.tasks.sourceJar.from(delombok)
repositories {
jcenter()
}

// specific modules should be excluded from publication
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 way to disable publication of these modules. Not sure if it's the best.

I've tested this manually using ./gradlew -x test publishToMavenLocal and verified that these three modules are no longer created under ~/.m2/repository/org/testcontainers

We should obviously be cautious to check the same applies to the bintray publication as well.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK artifacts.removeAll() will do the trick (no publishing if there are no artifacts to publish)

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly can't figure out how to make this work - my Gradle fu is clearly not strong enough. Unless there's something wrong with my approach I'd prefer to keep with something that I can understand!

if ( ! ["test-support", "jdbc-test", "docs-examples"].contains(it.name) ) {
apply from: "$rootDir/gradle/publishing.gradle"
apply from: "$rootDir/gradle/bintray.gradle"

project.tasks.sourceJar.from(delombok)

publishing {
publications {
mavenJava(MavenPublication) { publication ->
artifacts.removeAll { it.classifier == null }
artifact project.tasks.shadowJar
}
}
}

task release(dependsOn: bintrayUpload)
task release(dependsOn: bintrayUpload)
}

test {
defaultCharacterEncoding = "UTF-8"
Expand All @@ -49,10 +66,6 @@ subprojects {
}
}

repositories {
jcenter()
}

shadowJar {
configurations = []
classifier = null
Expand Down Expand Up @@ -86,15 +99,6 @@ subprojects {
}
}

publishing {
publications {
mavenJava(MavenPublication) { publication ->
artifacts.removeAll { it.classifier == null }
artifact project.tasks.shadowJar
}
}
}

dependencies {
testCompile 'ch.qos.logback:logback-classic:1.2.3'
}
Expand Down
2 changes: 1 addition & 1 deletion core/build.gradle
Expand Up @@ -113,7 +113,7 @@ dependencies {
testCompile files('testlib/repo/fakejar/fakejar/0/fakejar-0.jar')

testCompile 'org.assertj:assertj-core:3.12.2'

testCompile project(':test-support')

jarFileTestCompileOnly "org.projectlombok:lombok:${lombok.version}"
jarFileTestCompile 'junit:junit:4.12'
Expand Down
Expand Up @@ -14,6 +14,8 @@
import org.rnorth.ducttape.unreliables.Unreliables;
import org.testcontainers.containers.Container;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.containers.startupcheck.OneShotStartupCheckStrategy;
import org.testcontainers.testsupport.FlakyTestJUnit4RetryRule;
import org.testcontainers.utility.Base58;
import org.testcontainers.utility.TestEnvironment;

Expand Down Expand Up @@ -128,28 +130,8 @@ public static void setupContent() throws FileNotFoundException {
.withExtraHost("somehost", "192.168.1.10")
.withCommand("/bin/sh", "-c", "while true; do cat /etc/hosts | nc -l -p 80; done");

// @Test
// public void simpleRedisTest() {
// String ipAddress = redis.getContainerIpAddress();
// Integer port = redis.getMappedPort(REDIS_PORT);
//
// // Use Redisson to obtain a List that is backed by Redis
// Config redisConfig = new Config();
// redisConfig.useSingleServer().setAddress(ipAddress + ":" + port);
//
// Redisson redisson = Redisson.create(redisConfig);
//
// List<String> testList = redisson.getList("test");
// testList.add("foo");
// testList.add("bar");
// testList.add("baz");
//
// List<String> testList2 = redisson.getList("test");
// assertEquals("The list contains the expected number of items (redis is working!)", 3, testList2.size());
// assertTrue("The list contains an item that was put in (redis is working!)", testList2.contains("foo"));
// assertTrue("The list contains an item that was put in (redis is working!)", testList2.contains("bar"));
// assertTrue("The list contains an item that was put in (redis is working!)", testList2.contains("baz"));
// }
@Rule
public FlakyTestJUnit4RetryRule retry = new FlakyTestJUnit4RetryRule();
rnorth marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void testIsRunning() {
Expand Down Expand Up @@ -404,7 +386,8 @@ public void addExposedPortAfterWithExposedPortsTest() {
@Test
public void sharedMemorySetTest() {
try (GenericContainer containerWithSharedMemory = new GenericContainer()
.withSharedMemorySize(42L * FileUtils.ONE_MB)) {
.withSharedMemorySize(42L * FileUtils.ONE_MB)
.withStartupCheckStrategy(new OneShotStartupCheckStrategy())) {

containerWithSharedMemory.start();

Expand Down
2 changes: 2 additions & 0 deletions modules/couchbase/build.gradle
Expand Up @@ -3,4 +3,6 @@ description = "Testcontainers :: Couchbase"
dependencies {
compile project(':testcontainers')
compile 'com.couchbase.client:java-client:2.7.7'

testCompile project(':test-support')
}
Expand Up @@ -9,7 +9,10 @@
import com.couchbase.client.java.view.View;
import com.google.common.collect.Lists;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.testcontainers.testsupport.Flaky;
import org.testcontainers.testsupport.FlakyTestJUnit4RetryRule;

import java.util.List;

Expand All @@ -25,7 +28,11 @@ public abstract class BaseCouchbaseContainerTest extends AbstractCouchbaseTest {

private static final String DOCUMENT = "{\"name\":\"toto\"}";

@Rule
public FlakyTestJUnit4RetryRule retry = new FlakyTestJUnit4RetryRule();

@Test
@Flaky(githubIssueUrl = "https://github.com/testcontainers/testcontainers-java/issues/1453", reviewDate = "2019-10-01")
public void shouldInsertDocument() {
RawJsonDocument expected = RawJsonDocument.create(ID, DOCUMENT);
getBucket().upsert(expected);
Expand All @@ -34,6 +41,7 @@ public void shouldInsertDocument() {
}

@Test
@Flaky(githubIssueUrl = "https://github.com/testcontainers/testcontainers-java/issues/1453", reviewDate = "2019-10-01")
public void shouldExecuteN1ql() {
getBucket().query(N1qlQuery.simple("INSERT INTO " + TEST_BUCKET + " (KEY, VALUE) VALUES ('" + ID + "', " + DOCUMENT + ")"));

Expand All @@ -46,6 +54,7 @@ public void shouldExecuteN1ql() {
}

@Test
@Flaky(githubIssueUrl = "https://github.com/testcontainers/testcontainers-java/issues/1453", reviewDate = "2019-10-01")
public void shouldCreateView() {
View view = DefaultView.create(VIEW_NAME, VIEW_FUNCTION);
DesignDocument document = DesignDocument.create(VIEW_NAME, Lists.newArrayList(view));
Expand Down
1 change: 1 addition & 0 deletions modules/jdbc-test/build.gradle
Expand Up @@ -28,4 +28,5 @@ dependencies {
testCompile 'commons-dbutils:commons-dbutils:1.6'

testCompile 'com.googlecode.junit-toolbox:junit-toolbox:2.4'
testCompile project(':test-support')
}
@@ -1,18 +1,26 @@
package org.testcontainers.jdbc;

import org.junit.Rule;
import org.junit.Test;
import org.testcontainers.containers.Container;
import org.testcontainers.containers.JdbcDatabaseContainer;
import org.testcontainers.testsupport.Flaky;
import org.testcontainers.testsupport.FlakyTestJUnit4RetryRule;

import java.io.IOException;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import org.junit.Test;
import org.testcontainers.containers.Container;
import org.testcontainers.containers.JdbcDatabaseContainer;

import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;

public class DatabaseDriverTmpfsTest {

@Rule
public FlakyTestJUnit4RetryRule retry = new FlakyTestJUnit4RetryRule();

@Test
@Flaky(githubIssueUrl = "https://github.com/testcontainers/testcontainers-java/issues/1687", reviewDate = "2019-10-01")
public void tmpfs() throws IOException, InterruptedException, SQLException {
final String jdbcUrl = "jdbc:tc:postgresql:9.6.8://hostname/databasename?TC_TMPFS=/testtmpfs:rw";
try (Connection ignored = DriverManager.getConnection(jdbcUrl)) {
Expand Down
2 changes: 2 additions & 0 deletions settings.gradle
Expand Up @@ -41,3 +41,5 @@ file('modules').eachDir { dir ->

include "docs-examples"
project(":docs-examples").projectDir = file("docs/examples")
include 'test-support'
rnorth marked this conversation as resolved.
Show resolved Hide resolved

4 changes: 4 additions & 0 deletions test-support/build.gradle
@@ -0,0 +1,4 @@
dependencies {
implementation 'junit:junit:4.12'
implementation 'org.slf4j:slf4j-api:1.7.26'
}
@@ -0,0 +1,34 @@
package org.testcontainers.testsupport;

import java.lang.annotation.Retention;
import java.lang.annotation.Target;

import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

/**
* Annotation for test methods that should be retried in the event of failure. See {@link FlakyTestJUnit4RetryRule} for
* more details.
*/
@Retention(RUNTIME)
@Target({METHOD})
public @interface Flaky {

/**
* @return a URL for a GitHub issue where this flaky test can be discussed, and where actions to resolve it can be
* coordinated.
*/
String githubIssueUrl();

/**
* @return a date at which this should be reviewed, in {@link java.time.format.DateTimeFormatter#ISO_LOCAL_DATE}
* format (e.g. {@code 2020-12-03}). Now + 3 months is suggested. Once this date has passed, retries will no longer
* be applied.
*/
String reviewDate();
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about letting them fail explicitly instead of not retrying?
Which would mean, we either have to remove the annotation or prolong it?

Or does this create too much maintenance? I think the maintenance should be the same, just without explicitly failing, we will miss the flaky tests for some time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect it could end up being a PITA for contributors, to be honest!


/**
* @return the total number of times to try running this test (default 3)
*/
int maxTries() default 3;
}
@@ -0,0 +1,93 @@
package org.testcontainers.testsupport;

import lombok.extern.slf4j.Slf4j;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.MultipleFailureException;
import org.junit.runners.model.Statement;

import java.time.LocalDate;
import java.time.format.DateTimeParseException;
import java.util.ArrayList;
import java.util.List;

/**
* <p>
* JUnit 4 @Rule that implements retry for flaky tests (tests that suffer from sporadic random failures).
* </p>
* <p>
* This rule should be used in conjunction with the @{@link Flaky} annotation. When this Rule is applied to a test
* class, any test method with this annotation will be invoked up to 3 times or until it succeeds.
* </p>
* <p>
* Tests should <em>not</em> be marked @{@link Flaky} for a long period of time. Every usage should be
* accompanied by a GitHub issue URL, and should be subject to review at a suitable point in the (near) future.
* Should the review date pass without the test's instability being fixed, the retry behaviour will cease to have an
* effect and the test will be allowed to sporadically fail again.
* </p>
*/
@Slf4j
public class FlakyTestJUnit4RetryRule implements TestRule {

@Override
public Statement apply(Statement base, Description description) {

final Flaky annotation = description.getAnnotation(Flaky.class);

rnorth marked this conversation as resolved.
Show resolved Hide resolved
if (annotation != null) {
if (annotation.githubIssueUrl().trim().length() == 0) {
throw new IllegalArgumentException("A GitHub issue URL must be set for usages of the @Flaky annotation");
}

final int maxTries = annotation.maxTries();
rnorth marked this conversation as resolved.
Show resolved Hide resolved

final LocalDate reviewDate;
try {
reviewDate = LocalDate.parse(annotation.reviewDate());
} catch (DateTimeParseException e) {
throw new IllegalArgumentException("@Flaky reviewDate could not be parsed. Please provide a date in yyyy-mm-dd format");
}

// the annotation should only have an effect before the review date, to encourage review and resolution
if ( LocalDate.now().isBefore(reviewDate) ) {
return new RetryingStatement(base, description, maxTries);
}
}

// otherwise leave the statement as-is
return base;
}

private static class RetryingStatement extends Statement {
private final Statement base;
private final Description description;
private final int maxTries;

RetryingStatement(Statement base, Description description, int maxTries) {
this.base = base;
this.description = description;
this.maxTries = maxTries;
}

@Override
public void evaluate() {

int attempts = 0;
final List<Throwable> causes = new ArrayList<>();

while (++attempts <= maxTries) {
try {
base.evaluate();
return;
} catch (Throwable throwable) {
log.warn("Retrying @Flaky-annotated test: {}", description.getDisplayName());
causes.add(throwable);
}
}

throw new IllegalStateException(
"@Flaky-annotated test failed despite retries.",
new MultipleFailureException(causes));
}
}
}