Skip to content

Commit

Permalink
Configure test retries for JUnit Acceptance Tests (airbytehq#11818)
Browse files Browse the repository at this point in the history 
* acceptance tests with retries

* kube acceptance tests with retries

* javadoc to say we prefer not retrying

* no retries for tests that don't run on k8s
  • Loading branch information
@git-phu
git-phu authored Apr 14, 2022
1 parent fbaf83e commit 37a510c
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 36 deletions.
55 changes: 34 additions & 21 deletions airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/AcceptanceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,21 +127,34 @@
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.api.condition.DisabledIfEnvironmentVariable;
import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable;
import org.junitpioneer.jupiter.RetryingTest;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.slf4j.MDC;
import org.testcontainers.containers.PostgreSQLContainer;
import org.testcontainers.utility.MountableFile;

/**
* We order tests such that earlier tests test more basic behavior that is relied upon in later
* tests. e.g. We test that we can create a destination before we test whether we can sync data to
* it.
* <p>
* Many of the tests here are disabled for Kubernetes. This is because they are already run as part
* of the Docker acceptance tests and there is little value re-running on Kubernetes, especially
* operations take much longer due to Kubernetes pod spin up times. We run a subset to sanity check
* basic Airbyte Kubernetes Sync features.
* <p>
* Some tests here use the {@link RetryingTest} annotation instead of the more common {@link Test}
* to allow multiple tries for a test to pass. This is because these tests sometimes fail
* transiently, and we haven't been able to fix that yet.
* <p>
* However, in general we should prefer using {@code @Test} instead and only resort to using
* {@code @RetryingTest} for tests that we can't get to pass reliably. New tests should thus default
* to using {@code @Test} if possible.
* <p>
* For this class we currently use {@code @RetryingTest} for all tests that can run on k8s.
*/
@SuppressWarnings({"rawtypes", "ConstantConditions"})
// We order tests such that earlier tests test more basic behavior that is relied upon in later
// tests.
// e.g. We test that we can create a destination before we test whether we can sync data to it.
//
// Many of the tests here are disabled for Kubernetes. This is because they are already run
// as part of the Docker acceptance tests and there is little value re-running on Kubernetes,
// especially operations take much longer due to Kubernetes pod spin up times.
// We run a subset to sanity check basic Airbyte Kubernetes Sync features.
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
public class AcceptanceTests {

Expand Down Expand Up @@ -420,7 +433,7 @@ public void testSourceCheckConnection() throws ApiException {
checkConnectionRead.getMessage());
}

@Test
@RetryingTest(3)
@Order(5)
public void testDiscoverSourceSchema() throws ApiException {
final UUID sourceId = createPostgresSource().getSourceId();
Expand Down Expand Up @@ -481,7 +494,7 @@ public void testCreateConnection() throws ApiException {
assertEquals(name, createdConnection.getName());
}

@Test
@RetryingTest(3)
@Order(7)
public void testManualSync() throws Exception {
final String connectionName = "test-connection";
Expand All @@ -499,7 +512,7 @@ public void testManualSync() throws Exception {
assertSourceAndDestinationDbInSync(false);
}

@Test
@RetryingTest(3)
@Order(8)
public void testCancelSync() throws Exception {
final SourceDefinitionRead sourceDefinition = createE2eSourceDefinition();
Expand Down Expand Up @@ -534,7 +547,7 @@ public void testCancelSync() throws Exception {
assertEquals(JobStatus.CANCELLED, resp.getJob().getStatus());
}

@Test
@RetryingTest(3)
@Order(9)
public void testIncrementalSync() throws Exception {
LOGGER.info("Starting testIncrementalSync()");
Expand Down Expand Up @@ -739,7 +752,7 @@ public void testIncrementalDedupeSync() throws Exception {
assertNormalizedDestinationContains(expectedNormalizedRecords);
}

@Test
@RetryingTest(3)
@Order(14)
public void testCheckpointing() throws Exception {
final SourceDefinitionRead sourceDefinition = createE2eSourceDefinition();
Expand Down Expand Up @@ -803,7 +816,7 @@ public void testCheckpointing() throws Exception {
assertEquals(0, connectionState.getState().get("column1").asInt() % 5);
}

@Test
@RetryingTest(3)
@Order(15)
public void testRedactionOfSensitiveRequestBodies() throws Exception {
// check that the source password is not present in the logs
Expand All @@ -827,7 +840,7 @@ public void testRedactionOfSensitiveRequestBodies() throws Exception {
}

// verify that when the worker uses backpressure from pipes that no records are lost.
@Test
@RetryingTest(3)
@Order(16)
public void testBackpressure() throws Exception {
final SourceDefinitionRead sourceDefinition = createE2eSourceDefinition();
Expand Down Expand Up @@ -890,7 +903,7 @@ public void testBackpressure() throws Exception {
// This test is disabled because it takes a couple minutes to run, as it is testing timeouts.
// It should be re-enabled when the @SlowIntegrationTest can be applied to it.
// See relevant issue: https://github.com/airbytehq/airbyte/issues/8397
@Test
@RetryingTest(3)
@Order(17)
@Disabled
public void testFailureTimeout() throws Exception {
Expand Down Expand Up @@ -948,7 +961,7 @@ public void testFailureTimeout() throws Exception {
}
}

@Test
@RetryingTest(3)
@Order(18)
@EnabledIfEnvironmentVariable(named = "CONTAINER_ORCHESTRATOR",
matches = "true")
Expand Down Expand Up @@ -1018,7 +1031,7 @@ public void testDowntimeDuringSync() throws Exception {
}
}

@Test
@RetryingTest(3)
@Order(19)
@EnabledIfEnvironmentVariable(named = "CONTAINER_ORCHESTRATOR",
matches = "true")
Expand Down Expand Up @@ -1047,7 +1060,7 @@ public void testCancelSyncWithInterruption() throws Exception {
assertEquals(JobStatus.CANCELLED, resp.getJob().getStatus());
}

@Test
@RetryingTest(3)
@Order(20)
@Timeout(value = 5,
unit = TimeUnit.MINUTES)
Expand Down Expand Up @@ -1091,7 +1104,7 @@ public void testCuttingOffPodBeforeFilesTransfer() throws Exception {
waitForSuccessfulJob(apiClient.getJobsApi(), connectionSyncRead.getJob());
}

@Test
@RetryingTest(3)
@Order(21)
@Timeout(value = 5,
unit = TimeUnit.MINUTES)
Expand Down Expand Up @@ -1152,7 +1165,7 @@ public void testCancelSyncWhenCancelledWhenWorkerIsNotRunning() throws Exception
assertEquals(JobStatus.CANCELLED, resp.get().getJob().getStatus());
}

@Test
@RetryingTest(3)
@Order(22)
public void testDeleteConnection() throws Exception {
final String connectionName = "test-connection";
Expand Down
42 changes: 27 additions & 15 deletions ...s/src/test-integration/java/io/airbyte/workers/process/KubePodProcessIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,24 @@
import org.junit.jupiter.api.RepeatedTest;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junitpioneer.jupiter.RetryingTest;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

// requires kube running locally to run. If using Minikube it requires MINIKUBE=true
// Must have a timeout on this class because it tests child processes that may misbehave; otherwise
// this can hang forever during failures.
/**
* requires kube running locally to run. If using Minikube it requires MINIKUBE=true
* <p>
* Must have a timeout on this class because it tests child processes that may misbehave; otherwise
* this can hang forever during failures.
* <p>
* Many of the tests here use the {@link RetryingTest} annotation instead of the more common
* {@link Test} to allow multiple tries for a test to pass. This is because these tests sometimes
* fail transiently, and we haven't been able to fix that yet.
* <p>
* However, in general we should prefer using {@code @Test} instead and only resort to using
* {@code @RetryingTest} for tests that we can't get to pass reliably. New tests should thus default
* to using {@code @Test} if possible.
*/
@Timeout(value = 6,
unit = TimeUnit.MINUTES)
public class KubePodProcessIntegrationTest {
Expand Down Expand Up @@ -115,7 +127,7 @@ public void teardown() throws Exception {
* once, and check that they are all running in hopes of identifying regressions that introduce
* flakiness.
*/
@Test
@RetryingTest(3)
public void testConcurrentRunning() throws Exception {
final var totalJobs = 5;

Expand Down Expand Up @@ -147,7 +159,7 @@ public void testConcurrentRunning() throws Exception {
assertEquals(totalJobs, successCount.get());
}

@Test
@RetryingTest(3)
public void testSuccessfulSpawning() throws Exception {
// start a finite process
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand All @@ -160,7 +172,7 @@ public void testSuccessfulSpawning() throws Exception {
assertEquals(0, process.exitValue());
}

@Test
@RetryingTest(3)
public void testLongSuccessfulSpawning() throws Exception {
// start a finite process
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand Down Expand Up @@ -232,7 +244,7 @@ public void testPortsReintroducedIntoPoolOnlyOnce() throws Exception {
portsTaken.forEach(KubePortManagerSingleton.getInstance()::offer);
}

@Test
@RetryingTest(3)
public void testSuccessfulSpawningWithQuotes() throws Exception {
// start a finite process
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand All @@ -247,7 +259,7 @@ public void testSuccessfulSpawningWithQuotes() throws Exception {
assertEquals(0, process.exitValue());
}

@Test
@RetryingTest(3)
public void testEnvMapSet() throws Exception {
// start a finite process
final Process process = getProcess("echo ENV_VAR_1=$ENV_VAR_1");
Expand All @@ -260,7 +272,7 @@ public void testEnvMapSet() throws Exception {
assertEquals(0, process.exitValue());
}

@Test
@RetryingTest(3)
public void testPipeInEntrypoint() throws Exception {
// start a process that has a pipe in the entrypoint
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand All @@ -273,7 +285,7 @@ public void testPipeInEntrypoint() throws Exception {
assertEquals(0, process.exitValue());
}

@Test
@RetryingTest(3)
@Timeout(20)
public void testDeletingPodImmediatelyAfterCompletion() throws Exception {
// start a process that requests
Expand All @@ -299,7 +311,7 @@ public void testDeletingPodImmediatelyAfterCompletion() throws Exception {
assertEquals(10, process.exitValue());
}

@Test
@RetryingTest(3)
public void testExitCodeRetrieval() throws Exception {
// start a process that requests
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand All @@ -312,7 +324,7 @@ public void testExitCodeRetrieval() throws Exception {
assertEquals(10, process.exitValue());
}

@Test
@RetryingTest(3)
public void testMissingEntrypoint() throws WorkerException, InterruptedException {
// start a process with an entrypoint that doesn't exist
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand All @@ -325,7 +337,7 @@ public void testMissingEntrypoint() throws WorkerException, InterruptedException
assertEquals(127, process.exitValue());
}

@Test
@RetryingTest(3)
public void testKillingWithoutHeartbeat() throws Exception {
// start an infinite process
final var availablePortsBefore = KubePortManagerSingleton.getInstance().getNumAvailablePorts();
Expand All @@ -343,7 +355,7 @@ public void testKillingWithoutHeartbeat() throws Exception {
assertNotEquals(0, process.exitValue());
}

@Test
@RetryingTest(3)
public void testExitValueWaitsForMainToTerminate() throws Exception {
// start a long running main process
final Process process = getProcess("sleep 2; exit 13;");
Expand All @@ -359,7 +371,7 @@ public void testExitValueWaitsForMainToTerminate() throws Exception {
assertEquals(13, process.exitValue());
}

@Test
@RetryingTest(3)
public void testCopyLargeFiles() throws Exception {
final int numFiles = 1;
final int numLinesPerFile = 200000;
Expand Down
2 changes: 2 additions & 0 deletions build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,8 @@ subprojects {
testImplementation 'org.mockito:mockito-junit-jupiter:4.0.0'
testImplementation 'org.assertj:assertj-core:3.21.0'

testImplementation 'org.junit-pioneer:junit-pioneer:1.6.2'

// adds owasp plugin
spotbugsPlugins 'com.h3xstream.findsecbugs:findsecbugs-plugin:1.11.0'
}
Expand Down

0 comments on commit 37a510c

Please sign in to comment.