CAMEL-24290: ci - report tests that only passed after a retry

[ammachado]

Description

Part of CAMEL-24290 (improve CI to support larger workload). This is the visibility step: it makes an existing, currently invisible cost measurable before anything is changed about how tests run.

The problem

Surefire retries failing tests. surefire.rerunFailingTestsCount defaults to 2 in the full profile of parent/pom.xml (activated by !quickly), and both CI systems pass it again explicitly (Jenkinsfile:17, install-mvnd/action.yml:71).

A test that fails and then passes within those attempts is a recovered flake. The build goes green, nothing appears in the console output, and the retry leaves no trace. We are already paying up to 3x the runtime of every flaky test, and the retry is precisely the thing that stops anyone noticing.

parse_errors.sh cannot cover this, for two independent reasons:

1. It parses the .txt reports, which contain no flake data at all (verified: grep -c "Flake" on a surefire 3.5.6 .txt report with a known flake returns 0).
2. It only runs inside the if [[ ${ret} -ne 0 ]] branch, and a recovered flake exits 0.

The change

collect-flakes.py runs on the always-path of incremental-build.sh. It walks the reactor for target/{surefire,failsafe}-reports/TEST-*.xml and reports every <testcase> carrying <flakyFailure>/<flakyError>. Tests with <rerunFailure>/<rerunError> failed every attempt and already fail the build, so they are deliberately excluded.

The walk prunes rather than globbing: after a full build every module's target/ holds thousands of class and generated-source directories, none of which can hold a report, so **/target/... would descend into all of them.

Two outputs:

• A section appended to the PR comment and the job summary, naming module, test, attempt count and first failure message. Nothing is emitted when no test was retried, so clean PRs are unaffected.
• flakes.json, uploaded as flakes-java-<version> on PRs and flakes-main-java-<version> on main. .mvn/develocity.xml publishes build scans only when authenticated, so fork PRs produce no Develocity data. This artifact is the only per-PR record of flakiness we can accumulate.

The section names its JDK

The PR-comment artifact uploads with overwrite: true across the JDK matrix, justified by the existing comment in pr-build-main.yml: the content is identical across entries because the same modules are tested. Flake data is the one part where that does not hold. If JDK 17 flakes and JDK 25 does not, whichever entry finishes last decides what the comment shows.

Rather than restructure the comment/artifact strategy in this PR, the section names the entry it came from (flake-label on the action, --label on the script, also recorded in flakes.json):

:repeat: **1 test passed only after a retry on JDK 25** (1 retried attempt)

Last writer still wins, so the comment shows one JDK's flakes. What the label buys is that a shown section is attributable, and that an empty section can no longer be silently confused between "no flakes" and "the other JDK overwrote it". The per-JDK flakes-* artifacts remain the complete record, and the label in flakes.json lets aggregation distinguish a test that only flakes on one JDK from one that flakes everywhere.

Deliberate non-goals

No time figure is reported. Surefire records no per-attempt timing, and <testcase time> reflects only the final successful attempt. Estimating cost from it would systematically understate timeout-driven flakes, which are the common kind. Counts and test names are honest; a fabricated minutes-lost number would poison the evidence this is meant to gather.

Nothing about test execution changes. No test is skipped, quarantined, or retried differently. This PR only reports.

Notes for reviewers

• The script declares its dependency inline via PEP 723 and is run with uv run (PEP 723 describes inline dependency declaration similar to jbang). uv is installed by the action (astral-sh/setup-uv, pinned by SHA) so the action stays self-contained for downstream repos that reuse it. Both the uv version and the Python version it resolves against are pinned explicitly (version: "0.12.5", python-version: "3.11") — the action's own SHA pin does not fix what uv itself installs at run time, since version defaults to "latest" absent a pyproject.toml/uv.toml pin.
• XML is parsed with defusedxml rather than the stdlib, with forbid_dtd=True passed explicitly. defusedxml forbids entity declarations by default but not a bare <!DOCTYPE .. SYSTEM ..>, so the default alone does not enforce "Surefire never emits a DOCTYPE, therefore any report carrying one did not come from the build". testdata/TEST-doctype-no-entities-rejected.xml pins this.
• A pre-parse byte scan for <!DOCTYPE is not sufficient either: it misses a UTF-16 document, where the marker is interleaved with NUL bytes, and the entity then expands normally. testdata/TEST-utf16-doctype-rejected.xml pins that case. (The stdlib-only alternative of installing pyexpat entity handlers is unavailable: ET.XMLParser.parser no longer exists on current CPython.)
• Flaky attempts are read in document order. Collecting per tag (findall("flakyFailure") + findall("flakyError")) returns them grouped by tag, so the "First failure" column named the wrong attempt whenever the two kinds mixed, hiding the timeout that is typically the real cause.
• Failure messages are HTML-escaped. GitHub's renderer treats expected: <true> but was: <false> as raw HTML and strips it, which emptied the column for exactly the assertion messages that produce most flake reports.
• The step cannot fail a job. Parse errors are logged and skipped, and main() always returns 0, because by that point the build verdict is already decided.
• TEST-utf16-doctype-rejected.xml shows as binary in the diff. That is expected for UTF-16 with a BOM.

Verification

• 18 unit tests, run in pr-ci-scripts-validation.yml (path filter extended to .github/actions/incremental-build/**).
• Fixtures derived from real surefire 3.5.6 / JUnit 5.14.4 output rather than hand-written.
• Exercised end to end against a synthetic two-module reactor, and the shell function was executed in both GITHUB_STEP_SUMMARY set and unset states.
• The walk was checked for equivalence with the previous glob across Path, str and relative . roots, at root, one-level and three-level module depths.

Target

• I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

• If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

CAMEL-24290. Note the JIRA issue is currently Unassigned and this PR does not claim it; it implements one bullet from the ticket's list.

Apache Camel coding standards and style

• I checked that each commit in the pull request has a meaningful subject line and body.

• I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

Not run, and not applicable: this PR touches only .github/** (Python, YAML, shell, Markdown). No Java sources, POMs, or generated files are involved, so no auto-generated content can change. pr-build-main.yml itself carries paths-ignore: .github/** for exactly this reason.

AI-assisted contributions

• If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., Co-authored-by trailers) and the PR description identifies the AI tool used.

Written with Claude Code (Claude Opus 5 and Claude Sonnet 5, across commits). Every commit carries a Co-Authored-By trailer.

---

Claude Code on behalf of @ammachado

[github-actions]

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

• First-time contributors require MANUAL approval for the GitHub Actions to run
• You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
• You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
• Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

[apupier]

I think it would be easier to maintain and easier to leverage reported flaky tests by reusing the same tool than for Jenkins. Which is develocity.

[davsclaus]

This is a rules-and-conventions review from OSS Helper — it does not replace CodeRabbit, Sourcery, SonarCloud, or a dedicated static analyzer.

Overall this is a well-scoped, well-tested CI visibility change. To verify rather than just read it, I checked out the branch, ran all 18 unit tests locally (all pass), confirmed the forbid_dtd=True XXE defense actually rejects the UTF-16-hidden-DOCTYPE fixture, confirmed reportRecoveredFlakes genuinely runs on the always-path of incremental-build.sh (after the build's exit code is captured, before the final exit $ret), confirmed the new astral-sh/setup-uv SHA pin matches the v10.0.1 tag, and confirmed both commits carry the Co-Authored-By trailer per project convention. JIRA CAMEL-24290 is Open/Unassigned and the PR's "implements one bullet, doesn't claim the umbrella ticket" framing matches that.

One correctness issue below (inline), plus two non-blocking notes:

Recommend confirming before merge: this is a fork PR, and the only checks that ran are pr-ci-scripts-validation.yml (isolated unit tests) plus the PR-id uploader/dependency-review. pr-build-main.yml — the workflow that actually exercises the new flake-label input, the new install-uv composite-action step, and the new "Upload recovered-flake report" step — has no run at all for this branch yet (checked via gh run list --workflow=pr-build-main.yml), most likely GitHub's approval gate for fork PRs touching .github/workflows/** / .github/actions/**. The unit tests validate collect-flakes.py's logic in isolation but can't prove the composite-action wiring (uv landing on PATH, FLAKE_LABEL threading through, the hashFiles('flakes.json') upload gate) works on a live runner. Worth a maintainer approving/triggering that run before merging.

Minor/optional: in pr-build-main.yml, the new "Upload recovered-flake report" step isn't gated on !matrix.experimental the way the "Save PR number and test comment" step is. Harmless today (the matrix has no experimental: true entry), but worth a thought if an experimental JDK entry is ever added.

Claude Code on behalf of Claus Ibsen (@davsclaus)

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

[ammachado]

I think it would be easier to maintain and easier to leverage reported flaky tests by reusing the same tool than for Jenkins. Which is develocity.

Thanks for the suggestion, @apupier. I checked whether Develocity could stand in for this instead of the custom flake report, and it can't cover the case this feature is actually for.

.mvn/develocity.xml gates publishing on <publishing><onlyIf>authenticated</onlyIf></publishing> - a build scan only reaches develocity.apache.org if the Maven process authenticates. I grepped every workflow and composite action under .github/ for DEVELOCITY, GRADLE_ENTERPRISE, and ACCESS_KEY and found nothing; no GitHub Actions run in this repo ever gets a Develocity key. pr-build-main.yml triggers on plain pull_request (not pull_request_target), and the only secret it uses is secrets.GITHUB_TOKEN. GitHub withholds repo secrets from pull_request runs on fork PRs by design, since fork code effectively controls the run, so switching triggers to expose a Develocity key would just trade this problem for a real exfiltration risk.

So for a fork PR (the majority of external Camel contributions), and the exact audience that can't see Jenkins internals and needs inline visibility most - no Develocity scan ever gets published. There's nothing to leverage there. flakes.json plus the PR comment are the only per-PR record that can exist for that case, which is basically the reasoning already in the original commit message for this change.

Where I think you're right: for merged builds on trusted ASF Jenkins infra, Develocity's cross-build flaky-test view is a better long-term signal than one job's retry report. That's an argument for wiring Develocity on the trusted side too, not a substitute for PR-time visibility on the fork side. I can file a follow-up for that if it's useful.

Claude Code on behalf of Adriano Machado (@ammachado)

This reply was generated by an AI agent and may contain inaccuracies. Please verify before relying on it.

[apupier]

I'm a bit sceptical to the fact that contributors will benefit to the "flaky but still passing" report, especially given that there are a lot of them today. Either they will look and can be confused because it is an existing flaky one, or they won't care as it is passing anyway.
The "flaky but failing" are already looked by almost nobody.

Anyway, if i'm alone to think that that's fine and we can go ahead after other pints clarified/tackled.

Can you try to have these jobs triggered before we merge the PR to ensure that it is not breaking the main branch please?

[apupier]

What is the advantage/requirement to use uv compared to the "official" Python setup action https://github.com/actions/setup-python ?

[ammachado]

actions/setup-python only installs a Python interpreter — it has no notion of a script's own dependencies. collect-flakes.py declares its one runtime dependency (defusedxml>=0.7.1) inline via PEP 723:

# /// script
# requires-python = ">=3.9"
# dependencies = ["defusedxml>=0.7.1"]
# ///

uv run --quiet collect-flakes.py (see incremental-build.sh:447) reads that block and materializes an ephemeral, cached venv with exactly that dependency for the single invocation — no separate pip install step, no committed requirements.txt, and no risk of the dependency drifting from what the script actually declares it needs. With setup-python we'd still need a manual pip install defusedxml step to keep in sync by hand. That's the tradeoff captured in the comment on lines 55-57.

Claude Code on behalf of Adriano Machado (@ammachado)

This reply was generated by an AI agent and may contain inaccuracies. Please verify before relying on it.

[ammachado]

Addressing the two points from this review:

CI not triggered: Root cause is pr-build-main.yml's pull_request trigger has paths-ignore: [.github/**, ...] — since this PR only touches files under .github/, it never fires via the normal pull_request event (not the first-time-contributor approval gate the earlier review guessed at). I triggered it manually via the documented workflow_dispatch escape hatch: https://github.com/apache/camel/actions/runs/32856072627

Value of the "flaky but still passing" report: Fair concern, and I don't have a strong counter beyond what's in the original commit message — for a first-time or occasional contributor whose PR fails, then passes on retry, seeing why inline (rather than having to know Jenkins exists, find the right job, and dig through console logs) lowers the bar to understanding "was this me or was this the test." I agree the "flaky but failing" case is the one nobody reads today; I'm not trying to fix that habit with this PR, just make the passing-after-retry case visible where a PR author will actually see it. Open to gating this behind something narrower (e.g. only show it if the flake count is small, or only on first-time-contributor PRs) if that addresses the noise concern — let me know if you'd like that adjustment before merge.

Claude Code on behalf of Adriano Machado (@ammachado)

This reply was generated by an AI agent and may contain inaccuracies. Please verify before relying on it.