Repository navigation
Support test automation exclusion annotations in JUnit4 - #12940
calvinbayer wants to merge 5 commits into
Conversation
calvinbayer
added
type: feature
CI Visibility Test Environment - sbt-scalatestJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - nebula-release-pluginJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - netflix-zuulJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - reactive-streams-jvmJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - pass4sJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - heliboardJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - sonar-kotlinJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - jolokiaJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - okhttpJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
CI Visibility Test Environment - spring_bootJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
CI Visibility Test Environment - sonar-javaJob Status: 🟢 success
Baseline: median of Tests run to verify that CI Visibility behavior has not regressed in the current PR. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Protected JUnit4 tests forced to run by TIA’s unskippable category can lose their forced-run tag and metric because reporting ignores the new exclusions.
🤖 Bits Code Review · Commit 193ac29
8939084 to
8028762
Compare
|
@autotest Review |
There was a problem hiding this comment.
More details
Source and test inspection support that protected JUnit4 tests retain blocking failures and backend assignment tags while preserving framework-owned skips and retries.
🤖 Bits Code Review · Commit 8028762
8028762 to
4c3a3ac
Compare
|
@autotest Review |
There was a problem hiding this comment.
More details
Static review found no reportable issue with preserving JUnit4 skip semantics or separating backend assignment tags from excluded automation. Runtime behavior remains unverified because tests were not run.
🤖 Bits Code Review · Commit 4c3a3ac
ygree
left a comment
ygree
left a comment
There was a problem hiding this comment.
Can a test annotated with @PreventFailureSuppression pass after an automatic retry when used without @NoAutoRetry? If so, @NoQuarantineOrDisable might be a clearer name since it better describes the prevented behaviors.
|
@ygree Yes. With The policy selection keeps these controls independent. Your naming suggestion makes that distinction clearer; I’ll leave the API rename open for discussion. |
daniel-mohedano
left a comment
daniel-mohedano
left a comment
There was a problem hiding this comment.
left some comments and nits, but overall it's looking good, thanks for taking care of these changes 👍
| @Override | ||
| public SkipReason skipReason(TestIdentifier test) { |
There was a problem hiding this comment.
The changes moved over the @Nullable here:
| @Override | |
| public SkipReason skipReason(TestIdentifier test) { | |
| @Nullable | |
| @Override | |
| public SkipReason skipReason(TestIdentifier test) { |
| @Override | ||
| public SkipReason skipReason(TestIdentifier test) { |
There was a problem hiding this comment.
Same as in Proxy
| @Override | |
| public SkipReason skipReason(TestIdentifier test) { | |
| @Nullable | |
| @Override | |
| public SkipReason skipReason(TestIdentifier test) { |
| default SkipReason skipReason( | ||
| TestIdentifier test, TestSourceData source, Collection<String> tags) { | ||
| return skipReason(test); | ||
| } |
There was a problem hiding this comment.
Both Proxy and Headless already implement the method so we can make it abstract and also add back the @Nullable.
| } | |
| @Nullable | |
| SkipReason skipReason( | |
| TestIdentifier test, TestSourceData source, Collection<String> tags); |
| if (TestExclusions.noAutoRetry(testSourceData, categories)) { | ||
| test.setTag("test.test_management.no_auto_retry", true); | ||
| } | ||
| if (TestExclusions.preventFailureSuppression(testSourceData, categories)) { | ||
| test.setTag("test.test_management.prevent_failure_suppression", true); | ||
| } |
There was a problem hiding this comment.
Let's add both new tags to datadog.trace.bootstrap.instrumentation.api.Tags
| @Override | ||
| public SkipReason skipReason(TestIdentifier test) { |
There was a problem hiding this comment.
The override is missing the @Nullable here too
| @Override | |
| public SkipReason skipReason(TestIdentifier test) { | |
| @Nullable | |
| @Override | |
| public SkipReason skipReason(TestIdentifier test) { |
| # Test automation exclusions | ||
|
|
||
| Use the `dd-trace-api` artifact to declare test-local Datadog policy exclusions. |
There was a problem hiding this comment.
I think we only hold developer documentation in docs/, so I'd be inclined to remove this doc and explain the behavior of each annotation in its Javadoc. That way, customers using the public API also have all the context through the annotation.
| @@ -0,0 +1,38 @@ | |||
| package datadog.trace.api.civisibility.config; | |||
There was a problem hiding this comment.
nit: i'd put this under datadog.trace.api.civisibility.execution instead
| import java.util.Collection; | ||
|
|
||
| /** Resolves test-local declarations without retaining application classes between runs. */ | ||
| public final class TestExclusions { |
There was a problem hiding this comment.
If we end up removing the public API class, i'd be inclined to rename this to TestAutomationExclusion.
Also, to avoid building the Strings constantly, I think we could make this work with an enum, something like:
public enum TestAutomationExclusion {
NO_AUTO_RETRY(NoAutoRetry.TAG, NoAutoRetry.class.getName()),
PREVENT_FAILURE_SUPPRESSION(PreventFailureSuppression.TAG, PreventFailureSuppression.class.getName());
private final String tag;
private final String annotationName;
TestAutomationExclusion(String tag, String annotationName) {
this.tag = tag;
this.annotationName = annotationName;
}
public boolean isDeclared(@Nonnull TestSourceData source, @Nullable Collection<String> tags) {
...
}
...
}The call sites would end up looking something like:
if (isAttemptToFix(test) && !NO_AUTO_RETRY.isDeclared(source, tags)) {
return null;
}| if (annotation | ||
| .annotationType() | ||
| .getName() | ||
| .equals("datadog.trace.api.civisibility." + annotationName)) { |
There was a problem hiding this comment.
why do we need this name-matching instead of checking for the actual annotation? In TestEventsHandlerImpl we already check for annotations with a class literal.
private static boolean skipTrace(final Class<?> testClass) {
return testClass != null && testClass.getAnnotation(DisableTestTrace.class) != null;
}Should this be updated too?
| @Nullable | ||
| SkipReason skipReason(TestIdentifier test); | ||
|
|
||
| default SkipReason skipReason( |
There was a problem hiding this comment.
I'd argue for removing the default and making it abstract, plus implementing the method in NoOpTestEventsHandler, so it's easier to follow the logic. We should also add the @Nullable back
| default SkipReason skipReason( | |
| @Nullable | |
| SkipReason skipReason( |
This branch has not been deployed
3 participants
What Does This Do
Add public
@NoAutoRetryand@PreventFailureSuppressionannotations and JUnit4 support. Protected tests run despite Datadog disabling, preserve blocking failures under quarantine, and can exclude ATR, EFD, and Attempt to Fix while retaining backend assignment tags.Motivation
Let developers opt individual critical tests out of Datadog automation. Framework skips, TIA, parameterized source declarations, and explicit framework retries keep their normal semantics. Tag-based frameworks can use public
TestAutomationExclusionsconstants.Additional Notes
First PR in a per-framework Java stack; shared policy/API/reporting live here. No refactor commit was needed.
Validation:
./gradlew spotlessApply; focused existingExecutionStrategyTest; newAutomationExclusionsTest; JUnit4 compile and instrumentedJUnit4Test.test automation exclusions*all passed. The instrumented regression proves one execution, blocking failure, both annotation tags, retained disabled/quarantined/attempt-to-fix tags, and no attempt-to-fix completion tag.