Skip to content

Tag retried-test non-final attempts as skip in JUnit report #11443

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.

Sign up for GitHub

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

Draft
mhdatie wants to merge 1 commit into
base: master
Choose a base branch
from
+27 −0
Draft

Tag retried-test non-final attempts as skip in JUnit report #11443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .gitlab/collect-result/JUnitReport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,32 @@ void tagSyntheticFailures() {
}
}

/// Tags non-final attempts of a retried test so Test Optimization does not surface them as
/// real failures. The Develocity testRetry plugin re-runs failed tests and emits one
/// `<testcase>` per attempt sharing the same `(classname, name)`; CI ignores all but the
/// final attempt, so this method does the same by marking earlier attempts as `skip`.
///
/// Must run before [#tagFinalStatuses] so the existing per-testcase tagger does not
/// overwrite `skip` with `fail`.
///
/// See https://docs.gradle.com/develocity/gradle-plugin/current/#test_retry
void tagRetriedTests() {
var all = testcases();
for (var i = 0; i < all.size(); i++) {
var current = all.get(i);
var classname = current.getAttribute("classname");
var name = current.getAttribute("name");
for (var j = i + 1; j < all.size(); j++) {
Comment on lines +128 to +132
Copy link
Copy Markdown
Contributor Author

@mhdatie mhdatie May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will apply based on outcome of above convo #11443 (review)

var later = all.get(j);
if (classname.equals(later.getAttribute("classname"))
&& name.equals(later.getAttribute("name"))) {
addFinalStatusProperty(current, "skip", MissingPropertiesPlacement.APPEND_TO_TESTCASE);
break;
}
}
}
}

void tagFinalStatuses() {
for (var testcase : testcases()) {
if (hasFinalStatusProperty(testcase)) {
Expand Down
1 change: 1 addition & 0 deletions .gitlab/collect-result/ResultCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ private void collect(Path sourceXml) throws Exception {
var reportChangedBeforeFinalStatus = report.addFileAttribute(sourceFile);
reportChangedBeforeFinalStatus |= report.normalizeStableTestNames();
report.tagSyntheticFailures();
report.tagRetriedTests();
Copy link
Copy Markdown
Contributor

@bric3 bric3 May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This happens after normalizeStableTestNames();, so basically two different tests could end up with the same name, and one can get silently skipped.

E.g.

  • localhost:12345 and final_status=fail => after normalization : localhost:PORT and this one would incorrectly switch to final_status=skip
  • localhost:23456 end final_status=pass => after normalization : localhost:PORT

Copy link
Copy Markdown
Contributor Author

@mhdatie mhdatie May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch..

  • Do we normalize localhost failures and then just look at the count in Test Optim.?
  • What do you think of excluding the normalized cases from this operation?

I'm getting a tad concerned by the lack of visibility on retries.

Can you elaborate? It would be great to autodetect retries. Are these manually detected by us today? (cc @cbeauchesne) - I know that Flaky tests are labelled in Datadog and from there we can detect if they were retried (grouped by name)

Copy link
Copy Markdown
Contributor Author

@mhdatie mhdatie May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On catching retries proactively, I'm not sure if there are plans to auto-detect these retries by @DataDog/ci-app-libraries team.

I did some planning with Claude, and the suggestion was to:

  • Spend time understanding how detection can be implemented with Develocity's testRetry plugin (Gradle TestListener, JUnit 5 TestExecutionListener, etc)
  • Use CiVisibilityMetricCollector and CiVisibilityCountMetric since they have the metadata/tags to detect retries
  • Document ownership
Claude Details

Data model — trivial

Add a RetryReason.develocity enum value in internal-api/.../telemetry/tag/RetryReason.java. The existing TEST_EVENT_FINISHED count metric and Tags.TEST_RETRY_REASON span tag pick it up automatically.

Detection — the real work

Identifying that a given test execution is a Develocity retry requires one of:

  • A new instrumentation module hooking the plugin's retry executor class.
  • Extending the existing Gradle build-system instrumentation in agent-ci-visibility to detect the testRetry extension on the Test task and propagate a sysprop into forked test JVMs.
  • A same-session re-execution heuristic — fallback only, if neither of the above is viable.

Wiring

The resulting signal lands in TestEventsHandlerImpl.java:263, and must not override existing atr / efd / attemptToFix reasons.

Open decisions

  • Whether to also set IsRetry — probably yes.
  • Whether to set HasFailedAllRetries — probably no, since Develocity controls the retry budget out-of-process.

Validation

A smoke test under dd-smoke-tests/ with the plugin applied to a flaky test, plus a telemetry assertion that event_finished carries retry_reason:develocity_test_retry.

Ownership

RetryReason.java, TestEventsHandlerImpl.java, and anything under agent-ci-visibility/ are owned by @DataDog/ci-app-libraries — the change will require their review even if apm-lang-platform-java drives
it.

Recommendation

Do a short Phase-0 spike on the Develocity plugin's internals before committing to a detection approach.

Copy link
Copy Markdown
Contributor

@daniel-mohedano daniel-mohedano May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mhdatie! Unfortunately what Claude suggested here seems related to the testing instrumentation inside the tracer itself. In our case, it wouldn't apply because we don't instrument the tracer's tests with the tracer itself. The test reporting is entirely based on the JUnitXML report generated by the testing framework. I've discussed with the team and I'm not sure there would be an easy way of detecting the retries on our side during ingestion (there are several limitations like the ingestion order not necessarily following the execution order, which could then incorrectly tag retries) unless we were able to include some additional information in the report to inform this.

report.tagFinalStatuses();
report.write(targetXml);

Expand Down
Loading