-
Notifications
You must be signed in to change notification settings - Fork 362
Add JUnit support for @Flaky conditions #12620
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
base: master
Are you sure you want to change the base?
Changes from all commits
f2f7b4d
e59ec12
af699d8
4254dcf
2d53f0b
6526407
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,16 +103,12 @@ tasks.named("check") { | |
| } | ||
|
|
||
| tasks.withType<Test>().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") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We no longer add We needed to remove this because The only change is that a raw
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we include a Forbidden API for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Forbidden API can only forbid the tag in general and not flaky specification (unless I'm missing something), so I clarified one of the comments instead to note that |
||
| } | ||
|
|
||
| // 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) { | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { | ||
|
sarahchen6 marked this conversation as resolved.
|
||
| @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<String> logLines = new ArrayList<>(); | ||
| assertTrue( | ||
| app.waitForLogLine( | ||
| line -> { | ||
| logLines.add(line); | ||
| return "FIN".equals(line); | ||
| })); | ||
| Predicate<String> loadedResource = | ||
| compile("Loading sample.app.Resource[$]Test[1-3] from TestLoader").asPredicate(); | ||
| Predicate<String> 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<String> { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sort of Predicate that returns |
||
| @Override | ||
| public boolean test(String suite) { | ||
| return JavaVirtualMachine.isIbm(); | ||
| } | ||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<String> { | ||
| @Override | ||
| public boolean test(String suite) { | ||
| return JavaVirtualMachine.isIbm(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<? extends Predicate<String>> 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); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-PskipFlakyTestsno longer excludes flaky tests at JUnit discovery time. The old code called(options as? JUnitPlatformOptions)?.excludeTags("flaky")in skip mode, which skipped the whole class at discovery. That's been dropped in favor ofFlakyJUnitExtension's per-methodExecutionCondition, which only disables the individual@Testmethod — not the container — when@Flakyis method-level rather than class-level.For classes like
CustomSystemLoaderSmokeTest/SampleTraceSmokeTest(annotated@Flakyonly on the test method), running./gradlew test -PskipFlakyTestswill still trigger the class's@RegisterExtensionbeforeAll— launching the smoke-app subprocess and waiting on it — before the method itself is finally disabled. That defeats the point of-PskipFlakyTests, which was meant to avoid exactly that expensive setup.Worth adding an equivalent discovery-time (or container-level) skip path for
-PskipFlakyTests, e.g. extendingFlakyJUnitFilter/the extension to also check for method-level@Flakyand disable at the container level, or restoring anexcludeTagsmechanism that JUnit5's tag-based filtering can still apply.