Report failed flaky tests as skip#11931
Conversation
There was a problem hiding this comment.
More details
The PR safely implements downgrading failed flaky tests to "skip" status when continueOnFailure=true, maintaining proper ordering and guard conditions to prevent double-tagging. Code is well-documented, handles the environment variable safely, and preserves existing behavior for normal test runs.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 7d30705 · What is Autotest? · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
🟢 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. |
| /// {@link #tagSyntheticFailures()} are left untouched. | ||
| void tagFailuresAsSkipped() { | ||
| for (var testcase : testcases()) { | ||
| if (hasFinalStatusProperty(testcase) || !isFailed(testcase)) { |
There was a problem hiding this comment.
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 😃 .
There was a problem hiding this comment.
Let’s get it right now, it will save us time late.
I pushed follow up changes: 94dbdf3
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
@cbeauchesne Is there a way to check the results are following your expectations or do I need to merge it first?
There was a problem hiding this comment.
Including normally passing tests?
Yes. It's surprising, but the idea behind CONTINUE_ON_FAILURE=true is : whatever the test will say, it'll be ignored.
On the long run, we'd like to monitor such test, because if ever they constantly succeed, it does not make sense to ignore their outcome, and we'd like to remove them form this quarantine. It's the purpose of final_status to store this information. The difference is subtile, but it'll allow us to treat those "healthy but quarantined" tests at scale.
There was a problem hiding this comment.
@cbeauchesne Is there a way to check the results are following your expectations or do I need to merge it first?
Yes, the two job that have this flag are test_flaky and test_flaky_inst.
There was a problem hiding this comment.
ah, it's because it runs only on master :
Line 1007 in 4242339
You can do a dummy commit to run it on your branch, my link above will tell if it's fine
There was a problem hiding this comment.
On the long run, we'd like to monitor such test,
The Java Guild does. We have to widgets for:
You can do a dummy commit to run it on your branch
Thanks, I’ll give it a try. That will take some time to run / take care of.
There was a problem hiding this comment.
This looks to be working. Reverting the CI changes then merging. Thanks for the link!
566c143 to
94dbdf3
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
8d9ad4d
into
master
What Does This Do
This PR updates the test result collector to mark the failed flaky tests as skip.
Motivation
Avoid reporting "failures" to the Test Optimization product.
Additional Notes
It additionally fixes a typo for the release notes tag documentation.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APMLP-1267