add a timeout to junit test (#35767)

Today there's no clear timeout for tests. The only one that exists is at the github action level, and it has to be conservative at 1h. This change introduces a timeout per test method. The default value is set at 5 minutes per method, and can be overriden on a per method basis via the use of the junit @timeout annotation. We're also fixing a regression in the test ContainerFactory that was sharing less container than was possible. Finally, we're introducing a MdcScope.doNothing to allow using MdcScope in try blocks without triggering a compiler warning
airbytehq · Mar 2, 2024 · dc35e13 · dc35e13
1 parent 7063ea3
commit dc35e13
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 6 deletions.
diff --git a/airbyte-cdk/java/airbyte-cdk/README.md b/airbyte-cdk/java/airbyte-cdk/README.md
@@ -166,9 +166,10 @@ MavenLocal debugging steps:
 
 | Version | Date       | Pull Request                                               | Subject                                                                                                                                                        |
 |:--------|:-----------|:-----------------------------------------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| 0.23.12 | 2024-03-01 | [\#35767](https://github.com/airbytehq/airbyte/pull/35767) | introducing a timeout for java tests.                                                                                                                          |
 | 0.23.11 | 2024-03-01 | [\#35313](https://github.com/airbytehq/airbyte/pull/35313) | Preserve timezone offset in CSV writer for destinations                                                                                                        |
 | 0.23.10 | 2024-03-01 | [\#35303](https://github.com/airbytehq/airbyte/pull/35303) | Migration framework with DestinationState for softReset                                                                                                        |
-| 0.23.9  | 2024-03-01 | [\#35720](https://github.com/airbytehq/airbyte/pull/35720) | various improvements for tests TestDataHolder                                                                                                                  |
+| 0.23.9  | 2024-02-29 | [\#35720](https://github.com/airbytehq/airbyte/pull/35720) | various improvements for tests TestDataHolder                                                                                                                  |
 | 0.23.8  | 2024-02-28 | [\#35529](https://github.com/airbytehq/airbyte/pull/35529) | Refactor on state iterators                                                                                                                                    |
 | 0.23.7  | 2024-02-28 | [\#35376](https://github.com/airbytehq/airbyte/pull/35376) | Extract typereduper migrations to separte method                                                                                                               |
 | 0.23.6  | 2024-02-26 | [\#35647](https://github.com/airbytehq/airbyte/pull/35647) | Add a getNamespace into TestDataHolder                                                                                                                         |

diff --git a/airbyte-cdk/java/airbyte-cdk/core/src/main/resources/version.properties b/airbyte-cdk/java/airbyte-cdk/core/src/main/resources/version.properties
@@ -1 +1 @@
-version=0.23.11
+version=0.23.12
diff --git a/...dk/core/src/testFixtures/java/io/airbyte/cdk/extensions/LoggingInvocationInterceptor.java b/...dk/core/src/testFixtures/java/io/airbyte/cdk/extensions/LoggingInvocationInterceptor.java
@@ -16,6 +16,9 @@
 import java.util.regex.Pattern;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.api.Timeout.ThreadMode;
 import org.junit.jupiter.api.extension.DynamicTestInvocationContext;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.InvocationInterceptor;
@@ -33,6 +36,8 @@
  */
 public class LoggingInvocationInterceptor implements InvocationInterceptor {
 
+  private static final Duration DEFAULT_TIMEOUT = Duration.ofMinutes(5);
+
   private static final class LoggingInvocationInterceptorHandler implements InvocationHandler {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(LoggingInvocationInterceptor.class);
@@ -68,7 +73,13 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
       LOGGER.info("Junit starting {}", logLineSuffix);
       try {
         Instant start = Instant.now();
-        Object retVal = invocation.proceed();
+        final Object retVal;
+        Duration timeout = getTimeout(invocationContext);
+        if (timeout != null) {
+          retVal = Assertions.assertTimeoutPreemptively(timeout, invocation::proceed);
+        } else {
+          retVal = invocation.proceed();
+        }
         long elapsedMs = Duration.between(start, Instant.now()).toMillis();
         LOGGER.info("Junit completed {} in {} ms", logLineSuffix, elapsedMs);
         return retVal;
@@ -93,6 +104,26 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
       }
     }
 
+    private static Duration getTimeout(ReflectiveInvocationContext<Method> invocationContext) {
+      Duration timeout = DEFAULT_TIMEOUT;
+      if (invocationContext.getExecutable()instanceof Method m) {
+        Timeout timeoutAnnotation = m.getAnnotation(Timeout.class);
+        if (timeoutAnnotation == null) {
+          timeoutAnnotation = invocationContext.getTargetClass().getAnnotation(Timeout.class);
+        }
+        if (timeoutAnnotation != null) {
+          if (timeoutAnnotation.threadMode() == ThreadMode.SAME_THREAD) {
+            return null;
+          }
+          timeout = Duration.ofMillis(timeoutAnnotation.unit().toMillis(timeoutAnnotation.value()));
+        }
+      }
+      if (timeout.compareTo(Duration.ofHours(1)) > 0) {
+        return DEFAULT_TIMEOUT;
+      }
+      return timeout;
+    }
+
   }
 
   private final InvocationInterceptor proxy = (InvocationInterceptor) Proxy.newProxyInstance(

diff --git a/...ava/airbyte-cdk/core/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java b/...ava/airbyte-cdk/core/src/testFixtures/java/io/airbyte/cdk/testutils/ContainerFactory.java
@@ -35,7 +35,9 @@ public abstract class ContainerFactory<C extends GenericContainer<?>> {
 
   private record ContainerKey<C extends GenericContainer<?>> (Class<? extends ContainerFactory> clazz,
                                                               DockerImageName imageName,
-                                                              List<? extends NamedContainerModifier<C>> methods) {};
+                                                              List<? extends NamedContainerModifier<C>> methods) {}
+
+  ;
 
   private static class ContainerOrException {
 
@@ -70,7 +72,7 @@ GenericContainer<?> container() {
 
   }
 
-  private final ConcurrentMap<ContainerKey<C>, ContainerOrException> SHARED_CONTAINERS = new ConcurrentHashMap<>();
+  private static final ConcurrentMap<ContainerKey<?>, ContainerOrException> SHARED_CONTAINERS = new ConcurrentHashMap<>();
   private static final AtomicInteger containerId = new AtomicInteger(0);
 
   private final MdcScope.Builder getTestContainerLogMdcBuilder(DockerImageName imageName,
@@ -112,7 +114,7 @@ public final C shared(String imageName, List<? extends NamedContainerModifier<C>
     // Container creation can be exceedingly slow.
     // Furthermore, we need to handle exceptions raised during container creation.
     ContainerOrException containerOrError = SHARED_CONTAINERS.computeIfAbsent(containerKey,
-        key -> new ContainerOrException(() -> createAndStartContainer(key.imageName(), key.methods())));
+        key -> new ContainerOrException(() -> createAndStartContainer(key.imageName(), ((ContainerKey<C>) key).methods())));
     // Instead, the container creation (if applicable) is deferred to here.
     return (C) containerOrError.container();
   }

diff --git a/...nations/src/test/java/io/airbyte/cdk/integrations/destination/s3/csv/S3CsvWriterTest.java b/...nations/src/test/java/io/airbyte/cdk/integrations/destination/s3/csv/S3CsvWriterTest.java
@@ -45,8 +45,12 @@
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.api.Timeout.ThreadMode;
 import org.mockito.MockedConstruction;
 
+@Timeout(value = 1,
+         threadMode = ThreadMode.SAME_THREAD)
 class S3CsvWriterTest {
 
   public static final ConfiguredAirbyteStream CONFIGURED_STREAM = new ConfiguredAirbyteStream()