Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
7 changes: 5 additions & 2 deletions .gitlab/collect-result/CollectResults.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@

class CollectResults {
public static void main(String[] args) throws Exception {
// Detect flaky jobs
var continueOnFailure = Boolean.parseBoolean(System.getenv("CONTINUE_ON_FAILURE"));
// Run collector
var collector =
new ResultCollector(
Path.of("results"),
Path.of("workspace"),
List.of(Path.of("workspace"), Path.of("buildSrc")));

List.of(Path.of("workspace"), Path.of("buildSrc")),
continueOnFailure);
collector.collect();
}
}
26 changes: 25 additions & 1 deletion .gitlab/collect-result/JUnitReport.java
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,26 @@ void tagFinalStatuses() {
}
}

/// Downgrades failing testcases to `test.final_status=skip` for jobs that tolerate failures.
///
/// The flaky test jobs (`test_flaky`, `test_flaky_inst`) run with `CONTINUE_ON_FAILURE=true`, so
/// Gradle test failures are swallowed and a failing flaky test does not fail the job. Reporting
/// those failures to Test Optimization as `fail` produces false-positive failure notifications
/// and skews SLIs, so we record them as `skip` instead. Passing and skipped tests keep their
/// natural status, so the tests stay visible in Test Optimization.
///
/// **Must run before {@link #tagFinalStatuses()}** so the natural `fail` status is never assigned
/// to these testcases. Testcases already tagged by {@link #tagRetriedAttempts()} or
/// {@link #tagSyntheticFailures()} are left untouched.
void tagFailuresAsSkipped() {
for (var testcase : testcases()) {
if (hasFinalStatusProperty(testcase) || !isFailed(testcase)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From a semantic point of view, we should treat all tests in that job as final_status=skip. The reason is that this status is really meant to describe the impact of the CI, regardless of its status.

For green CI, it does not change anything, as only test failures are looked. But the day we'll start to look over the test based to check tests that are ignored while been always green, it'll be be the canonical way of looking them : status=pass AND final_status=skip.

Though, as the present ticket is about the impact on Green CI, feel free to ignore my comment 😃 .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let’s get it right now, it will save us time late.
I pushed follow up changes: 94dbdf3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should treat all tests in that job as final_status=skip

Including normally passing tests?
From what I understand here you're saying it should behave the way CI is behaving: the ENV is to "continue on failure", so that if a failure halts CI, all previously passing tests would report as skipped? Correct?


I don't quite follow your second statement/comment. Why would we want to look at passed tests that have final_status=skip except to exclude such cases from SLAs (false positives)?

@mhdatie mhdatie Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see Bruce already addressed the first part to report them all as skipped 94dbdf3

Edit: My webpage was stale so this comment came in late 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cbeauchesne Is there a way to check the results are following your expectations or do I need to merge it first?

continue;
}
addFinalStatusProperty(testcase, "skip", MissingPropertiesPlacement.FIRST_CHILD);
}
}

void write(Path xmlFile) throws Exception {
Files.createDirectories(xmlFile.getParent());
var tmpFile = Files.createTempFile(xmlFile.getParent(), "collect-results-", ".xml");
Expand Down Expand Up @@ -239,7 +259,7 @@ private static boolean propertiesHasFinalStatusProperty(Element properties) {
}

private static String finalStatus(Element testcase) {
if (hasChildElement(testcase, "failure") || hasChildElement(testcase, "error")) {
if (isFailed(testcase)) {
return "fail";
}
if (hasChildElement(testcase, "skipped")) {
Expand All @@ -248,6 +268,10 @@ private static String finalStatus(Element testcase) {
return "pass";
}

private static boolean isFailed(Element testcase) {
return hasChildElement(testcase, "failure") || hasChildElement(testcase, "error");
}

private static Element firstChildElement(Element parent, String tagName) {
var children = parent.getChildNodes();
for (var i = 0; i < children.getLength(); i++) {
Expand Down
14 changes: 13 additions & 1 deletion .gitlab/collect-result/ResultCollector.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@ final class ResultCollector {
private final Path resultsDir;
private final Path workspaceDir;
private final List<Path> searchDirs;
private final boolean continueOnFailure;
private final SourceFileResolver sourceFileResolver;

ResultCollector(Path resultsDir, Path workspaceDir, List<Path> searchDirs) {
ResultCollector(
Path resultsDir, Path workspaceDir, List<Path> searchDirs, boolean continueOnFailure) {
this.resultsDir = resultsDir;
this.workspaceDir = workspaceDir;
this.searchDirs = searchDirs;
this.continueOnFailure = continueOnFailure;
this.sourceFileResolver = new SourceFileResolver(workspaceDir);
}

Expand All @@ -32,6 +35,11 @@ void collect() throws Exception {
return;
}

if (continueOnFailure) {
System.out.println(
"CONTINUE_ON_FAILURE=true: reporting failed tests as skip");
}

System.out.println("Saving test results:");
for (var sourceXml : findXmlFiles(testResultDirs)) {
collect(sourceXml);
Expand All @@ -51,6 +59,10 @@ private void collect(Path sourceXml) throws Exception {
report.tagRetriedAttempts();
reportChangedBeforeFinalStatus |= report.normalizeStableTestNames();
report.tagSyntheticFailures();
// On flaky jobs, downgrade failures to skip before assigning natural statuses (APMLP-1267).
if (continueOnFailure) {
report.tagFailuresAsSkipped();
}
report.tagFinalStatuses();
report.write(targetXml);

Expand Down
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ docs/ Developer documentation (see below)

- Title: imperative verb sentence describing user-visible change (e.g. "Fix span sampling rule parsing")
- Labels: always add `tag: ai generated` and at least one `comp:` or `inst:` label + one `type:` label
- Use `tag: no release note` for internal/refactoring changes
- Use `tag: no release notes` for internal/refactoring changes
- Open as draft first, convert to ready when reviewable

## Bootstrap constraints (critical)
Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ Both pull requests and issues should be labelled with at least a component or an

Labels are used to not only categorize but also alter the continuous integration behavior:

* `tag: no release note` to exclude a pull request from the next release changelog. Use it when changes are not relevant to the users like:
* `tag: no release notes` to exclude a pull request from the next release changelog. Use it when changes are not relevant to the users like:
* Internal features changes
* Refactoring pull requests
* CI and build tools improvements
Expand Down
Loading