diff --git a/buildSrc/src/main/kotlin/dd-trace-java.configure-tests.gradle.kts b/buildSrc/src/main/kotlin/dd-trace-java.configure-tests.gradle.kts index 04862485528..1e98ee77b52 100644 --- a/buildSrc/src/main/kotlin/dd-trace-java.configure-tests.gradle.kts +++ b/buildSrc/src/main/kotlin/dd-trace-java.configure-tests.gradle.kts @@ -103,16 +103,12 @@ tasks.named("check") { } tasks.withType().configureEach { - // Flaky tests management for JUnit 5 - (options as? JUnitPlatformOptions)?.apply { - if (skipFlakyTestsProvider.isPresent) { - excludeTags("flaky") - } else if (runFlakyTestsProvider.isPresent) { - includeTags("flaky") - } + // Keep suites without test-utils out of flaky-only runs. Runtime extensions refine this tag. + if (!skipFlakyTestsProvider.isPresent && runFlakyTestsProvider.isPresent) { + (options as? JUnitPlatformOptions)?.includeTags("flaky") } - // Set system property flag that is checked from tests to determine if they should be skipped or run + // Let the JUnit and Spock extensions evaluate @Flaky conditions before selecting tests. if (skipFlakyTestsProvider.isPresent) { jvmArgs("-Drun.flaky.tests=false") } else if (runFlakyTestsProvider.isPresent) { diff --git a/dd-smoke-tests/custom-systemloader/src/test/groovy/datadog/smoketest/CustomSystemLoaderSmokeTest.groovy b/dd-smoke-tests/custom-systemloader/src/test/groovy/datadog/smoketest/CustomSystemLoaderSmokeTest.groovy deleted file mode 100644 index 0cf70b42f41..00000000000 --- a/dd-smoke-tests/custom-systemloader/src/test/groovy/datadog/smoketest/CustomSystemLoaderSmokeTest.groovy +++ /dev/null @@ -1,54 +0,0 @@ -package datadog.smoketest - -import datadog.environment.JavaVirtualMachine -import datadog.trace.test.util.Flaky - -import static java.util.concurrent.TimeUnit.SECONDS - -class CustomSystemLoaderSmokeTest extends AbstractSmokeTest { - private static final int TIMEOUT_SECS = 30 - - @Override - def logLevel() { - "debug" - } - - @Override - ProcessBuilder createProcessBuilder() { - String appJar = System.getProperty("datadog.smoketest.systemloader.shadowJar.path") - assert new File(appJar).isFile() - - List command = new ArrayList<>() - command.add(javaPath()) - command.addAll(defaultJavaProperties) - command.add("-Djava.system.class.loader=datadog.smoketest.systemloader.TestLoader") - command.addAll((String[]) ["-jar", appJar]) - - ProcessBuilder processBuilder = new ProcessBuilder(command) - processBuilder.directory(new File(buildDirectory)) - - return processBuilder - } - - @Flaky(value = 'Race condition with IMB. Check APMAPI-1194', condition = () -> JavaVirtualMachine.isIbm()) - def "resource types loaded by custom system class-loader are transformed"() { - when: - testedProcess.waitFor(TIMEOUT_SECS, SECONDS) - - then: - testedProcess.exitValue() == 0 - int loadedResources = 0 - int transformedResources = 0 - forEachLogLine { - String it -> - if (it =~ /Loading sample.app.Resource[$]Test[1-3] from TestLoader/) { - loadedResources++ - } - if (it =~ /Transformed.*class=sample.app.Resource[$]Test[1-3].*classloader=datadog.smoketest.systemloader.TestLoader/) { - transformedResources++ - } - } - loadedResources == 3 - transformedResources == 3 - } -} diff --git a/dd-smoke-tests/custom-systemloader/src/test/java/datadog/smoketest/CustomSystemLoaderSmokeTest.java b/dd-smoke-tests/custom-systemloader/src/test/java/datadog/smoketest/CustomSystemLoaderSmokeTest.java new file mode 100644 index 00000000000..3a0d91a9bec --- /dev/null +++ b/dd-smoke-tests/custom-systemloader/src/test/java/datadog/smoketest/CustomSystemLoaderSmokeTest.java @@ -0,0 +1,61 @@ +package datadog.smoketest; + +import static datadog.smoketest.backend.AgentBackend.testAgent; +import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.regex.Pattern.compile; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.environment.JavaVirtualMachine; +import datadog.trace.test.util.Flaky; +import java.io.File; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Predicate; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +class CustomSystemLoaderSmokeTest { + @RegisterExtension + static final SmokeCliApp app = + SmokeCliApp.named("custom-systemloader") + .jar(System.getProperty("datadog.smoketest.systemloader.shadowJar.path")) + .jvmArgs("-Djava.system.class.loader=datadog.smoketest.systemloader.TestLoader") + .workingDirectory(new File(System.getProperty("datadog.smoketest.builddir"))) + .debugLogs() + .backend(testAgent()) + // The app may exit before sending telemetry. + .skipTelemetryCheck() + .build(); + + @Test + @DisplayName("resource types loaded by custom system class-loader are transformed") + @Flaky(value = "Race condition with IBM. Check APMAPI-1194", condition = IbmJvm.class) + void resourceTypesLoadedByCustomSystemClassLoaderAreTransformed() { + app.assertCompletesWithValue(30, SECONDS, 0); + + List logLines = new ArrayList<>(); + assertTrue( + app.waitForLogLine( + line -> { + logLines.add(line); + return "FIN".equals(line); + })); + Predicate loadedResource = + compile("Loading sample.app.Resource[$]Test[1-3] from TestLoader").asPredicate(); + Predicate transformedResource = + compile( + "Transformed.*class=sample.app.Resource[$]Test[1-3].*classloader=datadog.smoketest.systemloader.TestLoader") + .asPredicate(); + assertEquals(3, logLines.stream().filter(loadedResource).count()); + assertEquals(3, logLines.stream().filter(transformedResource).count()); + } + + static class IbmJvm implements Predicate { + @Override + public boolean test(String suite) { + return JavaVirtualMachine.isIbm(); + } + } +} diff --git a/dd-smoke-tests/sample-trace/src/test/groovy/datadog/smoketest/SampleTraceSmokeTest.groovy b/dd-smoke-tests/sample-trace/src/test/groovy/datadog/smoketest/SampleTraceSmokeTest.groovy deleted file mode 100644 index 791f9aed1c2..00000000000 --- a/dd-smoke-tests/sample-trace/src/test/groovy/datadog/smoketest/SampleTraceSmokeTest.groovy +++ /dev/null @@ -1,37 +0,0 @@ -package datadog.smoketest - -import datadog.environment.JavaVirtualMachine -import datadog.trace.test.util.Flaky - -class SampleTraceSmokeTest extends AbstractSmokeTest { - - @Override - ProcessBuilder createProcessBuilder() { - List command = new ArrayList<>() - command.add(javaPath()) - // tests tracer as a jar sending sample traces instead of a javaagent - command.addAll((String[]) [ - "-Ddd.trace.agent.port=${server.address.port}", - '-jar', - "${shadowJarPath}", - 'sampleTrace', - '-c', - '10', - '-i', - '0.1' - ]) - ProcessBuilder processBuilder = new ProcessBuilder(command) - processBuilder.directory(new File(buildDirectory)) - } - - @Flaky(condition = () -> JavaVirtualMachine.isIbm()) - def 'sample traces are sent'() { - when: - waitForTraceCount(10) - testedProcess.waitFor() - - then: - testedProcess.exitValue() == 0 - traceCount.get() == 10 - } -} diff --git a/dd-smoke-tests/sample-trace/src/test/java/datadog/smoketest/SampleTraceSmokeTest.java b/dd-smoke-tests/sample-trace/src/test/java/datadog/smoketest/SampleTraceSmokeTest.java new file mode 100644 index 00000000000..e033069dcbc --- /dev/null +++ b/dd-smoke-tests/sample-trace/src/test/java/datadog/smoketest/SampleTraceSmokeTest.java @@ -0,0 +1,51 @@ +package datadog.smoketest; + +import static datadog.smoketest.backend.AgentBackend.testAgent; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import datadog.environment.JavaVirtualMachine; +import datadog.smoketest.backend.AgentBackend; +import datadog.trace.test.util.Flaky; +import java.io.File; +import java.util.function.Predicate; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +class SampleTraceSmokeTest { + private static final AgentBackend BACKEND = testAgent(); + + @RegisterExtension + static final SmokeCliApp app = + SmokeCliApp.named("sample-trace") + .jar(System.getProperty("datadog.smoketest.agent.shadowJar.path")) + // tests tracer as a jar sending sample traces instead of a javaagent + .noAgent() + .backend(BACKEND) + .placeholder("agent.host", () -> BACKEND.url().getHost()) + .placeholder("agent.port", () -> Integer.toString(BACKEND.port())) + .jvmArgs( + "-Ddd.agent.host=${agent.host}", + "-Ddd.trace.agent.port=${agent.port}", + "-Ddd.test.agent.session.token=" + BACKEND.sessionToken()) + .args("sampleTrace", "-c", "10", "-i", "0.1") + .workingDirectory(new File(System.getProperty("datadog.smoketest.builddir"))) + .build(); + + @Test + @DisplayName("sample traces are sent") + @Flaky(condition = IbmJvm.class) + void sampleTracesAreSent() { + app.traces().waitForTraceCount(10); + app.assertCompletesWithValue(30, SECONDS, 0); + assertEquals(10, app.traces().getTraces().size()); + } + + static class IbmJvm implements Predicate { + @Override + public boolean test(String suite) { + return JavaVirtualMachine.isIbm(); + } + } +} diff --git a/docs/how_to_test.md b/docs/how_to_test.md index a0bcefdb368..96d50a93f14 100644 --- a/docs/how_to_test.md +++ b/docs/how_to_test.md @@ -44,8 +44,33 @@ This mechanism exists to make sure either java agent state or static data are re ### Flaky Tests -If a test runs unreliably, or doesn't have a fully deterministic behavior, this will lead to recurrent unexpected errors in continuous integration. -In order to identify such tests and avoid the continuous integration to fail, they are marked as _flaky_ and must be annotated with the `@Flaky` annotation. +Mark unreliable test methods or classes with `@Flaky` in both JUnit and Spock. + +All tests run by default. Use `-PskipFlakyTests` to skip flaky tests or `-PrunFlakyTests` to run only flaky tests. + +If a test is flaky only in certain environments, use `condition`. In Java, supply a predicate class +with a no-argument constructor. Its `test` method returns `true` when the test is flaky: + +```java +import datadog.environment.JavaVirtualMachine; +import java.util.function.Predicate; + +@Test +@Flaky(condition = IbmJvm.class) +void testOnSupportedJvms() { + // ... +} + +static class IbmJvm implements Predicate { + @Override + public boolean test(String suite) { + return JavaVirtualMachine.isIbm(); + } +} +``` + +Use `suites = {"SomeSubclass"}` to limit the annotation to particular test classes, such as subclasses +that inherit a test. When both `suites` and `condition` are specified, both must match. > [!TIP] > In case your pull request checks failed due to some unexpected flaky tests, you can retry the continuous diff --git a/utils/test-utils/build.gradle.kts b/utils/test-utils/build.gradle.kts index c47732851d6..ebc34b09471 100644 --- a/utils/test-utils/build.gradle.kts +++ b/utils/test-utils/build.gradle.kts @@ -37,6 +37,7 @@ dependencies { compileOnly(project(":components:annotations")) compileOnly(libs.junit.jupiter) + compileOnly(libs.junit.platform.launcher) compileOnly(libs.logback.core) compileOnly(libs.logback.classic) diff --git a/utils/test-utils/gradle.lockfile b/utils/test-utils/gradle.lockfile index 78df65e886e..df3cd0a59a6 100644 --- a/utils/test-utils/gradle.lockfile +++ b/utils/test-utils/gradle.lockfile @@ -54,7 +54,7 @@ org.junit.jupiter:junit-jupiter-params:5.14.1=compileClasspath,testCompileClassp org.junit.jupiter:junit-jupiter:5.14.1=compileClasspath,testCompileClasspath,testRuntimeClasspath org.junit.platform:junit-platform-commons:1.14.1=compileClasspath,testCompileClasspath,testRuntimeClasspath org.junit.platform:junit-platform-engine:1.14.1=compileClasspath,testCompileClasspath,testRuntimeClasspath -org.junit.platform:junit-platform-launcher:1.14.1=testRuntimeClasspath +org.junit.platform:junit-platform-launcher:1.14.1=compileClasspath,testRuntimeClasspath org.junit:junit-bom:5.14.1=compileClasspath,testCompileClasspath,testRuntimeClasspath org.mockito:mockito-core:4.4.0=testRuntimeClasspath org.objenesis:objenesis:3.3=compileClasspath,testCompileClasspath,testRuntimeClasspath diff --git a/utils/test-utils/src/main/groovy/datadog/trace/test/util/Flaky.java b/utils/test-utils/src/main/groovy/datadog/trace/test/util/Flaky.java index c4625a3de38..e0f6072f9fb 100644 --- a/utils/test-utils/src/main/groovy/datadog/trace/test/util/Flaky.java +++ b/utils/test-utils/src/main/groovy/datadog/trace/test/util/Flaky.java @@ -6,27 +6,30 @@ import java.lang.annotation.Target; import java.util.function.Predicate; import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.extension.ExtendWith; /** * Use this annotation for suites or test cases that are flaky. When running in CI, these will be - * segregated to a separate job. + * split to a separate job. Apply this annotation instead of {@code @Tag("flaky")} directly. */ @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.TYPE, ElementType.METHOD}) @Tag("flaky") +@ExtendWith(FlakyJUnitExtension.class) public @interface Flaky { /** Reason why the test is flaky (optional). */ String value() default ""; /** - * Fully qualified name of the test suite classes where this test is flaky. Only required when the - * test is flaky only when run in a subclass. + * Names of the test suite classes where this test is flaky, typically subclasses that inherit the + * test. Spock uses simple class names; JUnit accepts simple or fully qualified class names. */ String[] suites() default {}; /** - * Closure with a predicate to test at runtime if the actual spec is flaky (e.g. check the JVM - * vendor), the parameter is the actual name of the spec under test + * Predicate class with a no-argument constructor that determines whether the test is flaky (e.g. + * check the JVM vendor). JUnit passes the concrete test class's simple name. Spock also supports + * Groovy closures. */ Class> condition() default True.class; diff --git a/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakyJUnitExtension.java b/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakyJUnitExtension.java new file mode 100644 index 00000000000..8d3838db237 --- /dev/null +++ b/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakyJUnitExtension.java @@ -0,0 +1,77 @@ +package datadog.trace.test.util; + +import static org.junit.platform.commons.support.AnnotationSupport.findAnnotation; + +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Constructor; +import java.lang.reflect.Method; +import java.util.function.Predicate; +import org.junit.jupiter.api.extension.ConditionEvaluationResult; +import org.junit.jupiter.api.extension.ExecutionCondition; +import org.junit.jupiter.api.extension.ExtensionConfigurationException; +import org.junit.jupiter.api.extension.ExtensionContext; + +/** Selects JUnit tests using the same flaky-test modes as {@link FlakySpockExtension}. */ +public final class FlakyJUnitExtension implements ExecutionCondition { + private static final String RUN_FLAKY_TESTS = "run.flaky.tests"; + + @Override + public ConditionEvaluationResult evaluateExecutionCondition(ExtensionContext context) { + if (!"false".equals(System.getProperty(RUN_FLAKY_TESTS)) + || !context.getTestClass().isPresent()) { + return ConditionEvaluationResult.enabled("Flaky tests are not skipped"); + } + Flaky flaky = findFlaky(context.getRequiredTestClass(), context.getTestMethod().orElse(null)); + if (flaky == null) { + return ConditionEvaluationResult.enabled("Test is not flaky"); + } + return ConditionEvaluationResult.disabled( + flaky.value().isEmpty() ? "Flaky test" : "Flaky test: " + flaky.value()); + } + + static Flaky findFlaky(Class testClass, Method method) { + for (Class enclosing = testClass; + enclosing != null; + enclosing = enclosing.getEnclosingClass()) { + for (Class current = enclosing; current != null; current = current.getSuperclass()) { + Flaky flaky = matchingAnnotation(current, testClass); + if (flaky != null) { + return flaky; + } + } + } + return method == null ? null : matchingAnnotation(method, testClass); + } + + private static Flaky matchingAnnotation(AnnotatedElement element, Class testClass) { + Flaky flaky = findAnnotation(element, Flaky.class).orElse(null); + if (flaky == null) { + return null; + } + if (flaky.suites().length > 0) { + boolean matches = false; + for (String suite : flaky.suites()) { + if (suite.equals(testClass.getSimpleName()) || suite.equals(testClass.getName())) { + matches = true; + break; + } + } + if (!matches) { + return null; + } + } + if (flaky.condition() == Flaky.True.class) { + return flaky; + } + try { + Constructor> constructor = + flaky.condition().getDeclaredConstructor(); + constructor.setAccessible(true); + return constructor.newInstance().test(testClass.getSimpleName()) ? flaky : null; + } catch (ReflectiveOperationException | RuntimeException e) { + throw new ExtensionConfigurationException( + "Could not evaluate @Flaky condition " + flaky.condition().getName() + " on " + element, + e); + } + } +} diff --git a/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakyJUnitFilter.java b/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakyJUnitFilter.java new file mode 100644 index 00000000000..517afb2b8d4 --- /dev/null +++ b/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakyJUnitFilter.java @@ -0,0 +1,29 @@ +package datadog.trace.test.util; + +import static datadog.trace.test.util.FlakyJUnitExtension.findFlaky; + +import org.junit.platform.engine.FilterResult; +import org.junit.platform.engine.TestDescriptor; +import org.junit.platform.engine.TestSource; +import org.junit.platform.engine.support.descriptor.MethodSource; +import org.junit.platform.launcher.PostDiscoveryFilter; + +/** Excludes non-flaky JUnit tests from flaky-only runs before execution. */ +public final class FlakyJUnitFilter implements PostDiscoveryFilter { + @Override + public FilterResult apply(TestDescriptor descriptor) { + if (!"true".equals(System.getProperty("run.flaky.tests")) + || !descriptor.getUniqueId().getEngineId().filter("junit-jupiter"::equals).isPresent()) { + return FilterResult.included("Flaky-only filtering does not apply"); + } + TestSource source = descriptor.getSource().orElse(null); + if (source instanceof MethodSource) { + MethodSource method = (MethodSource) source; + return findFlaky(method.getJavaClass(), method.getJavaMethod()) != null + ? FilterResult.included("Flaky test") + : FilterResult.excluded("Test is not flaky"); + } + // Keep containers until their methods have been filtered; JUnit prunes empty containers. + return FilterResult.included("Container may contain flaky tests"); + } +} diff --git a/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakySpockExtension.groovy b/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakySpockExtension.groovy index ac962da6a65..8420215f627 100644 --- a/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakySpockExtension.groovy +++ b/utils/test-utils/src/main/groovy/datadog/trace/test/util/FlakySpockExtension.groovy @@ -42,6 +42,11 @@ class FlakySpockExtension extends AbstractGlobalExtension { } } } + + if (shouldRunFlakyTestsOnly()) { + // Preserve selected features through the JUnit Platform's flaky tag filter. + spec.getAllFeatures().findAll { !it.excluded }.each { it.addTestTag("flaky") } + } } private static void skip(final node) { diff --git a/utils/test-utils/src/main/resources/META-INF/services/org.junit.platform.launcher.PostDiscoveryFilter b/utils/test-utils/src/main/resources/META-INF/services/org.junit.platform.launcher.PostDiscoveryFilter new file mode 100644 index 00000000000..a36aa1c3981 --- /dev/null +++ b/utils/test-utils/src/main/resources/META-INF/services/org.junit.platform.launcher.PostDiscoveryFilter @@ -0,0 +1 @@ +datadog.trace.test.util.FlakyJUnitFilter