From 9f7b550480b9d2b888496bd7957b9a5d30fb1889 Mon Sep 17 00:00:00 2001 From: Adriano Machado <60320+ammachado@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:45:22 -0400 Subject: [PATCH 1/6] CAMEL-24290: ci - report tests that only passed after a retry Surefire retries failing tests: surefire.rerunFailingTestsCount defaults to 2 in the `full` profile of parent/pom.xml, and both CI systems pass it again explicitly. A test that fails and then passes within those attempts leaves the build green and leaves nothing in the console output, so today the retry is invisible. parse_errors.sh cannot cover this for two independent reasons: it parses the .txt reports, which carry no flake data at all, and it only runs inside the build-failure branch, while a recovered flake exits 0. Add collect-flakes.py, invoked on the always-path of incremental-build.sh. It walks **/target/{surefire,failsafe}-reports/TEST-*.xml and reports every carrying /. Tests with failed every attempt and already fail the build, so they are excluded. Two outputs: a section appended to the PR comment and job summary, and flakes.json uploaded as a workflow artifact. Develocity publishes build scans only when authenticated, so fork PRs produce none; that artifact is the only per-PR record of flakiness available. No time figure is reported. Surefire records no per-attempt timing and reflects only the final successful attempt, so a minutes-lost number would understate timeout-driven flakes specifically. XML is parsed with defusedxml, declared inline via PEP 723 and resolved by uv. A pre-parse byte scan for --- .github/CI-ARCHITECTURE.md | 40 +++ .github/actions/incremental-build/action.yaml | 7 + .../incremental-build/collect-flakes.py | 221 +++++++++++++++ .../incremental-build/incremental-build.sh | 35 +++ .../incremental-build/test_collect_flakes.py | 264 ++++++++++++++++++ .../testdata/TEST-doctype-rejected.xml | 13 + .../testdata/TEST-recovered-flake.xml | 25 ++ .../testdata/TEST-two-attempt-flake.xml | 18 ++ .../testdata/TEST-utf16-doctype-rejected.xml | Bin 0 -> 724 bytes .github/workflows/pr-build-main.yml | 9 + .../workflows/pr-ci-scripts-validation.yml | 13 + 11 files changed, 645 insertions(+) create mode 100644 .github/actions/incremental-build/collect-flakes.py create mode 100644 .github/actions/incremental-build/test_collect_flakes.py create mode 100644 .github/actions/incremental-build/testdata/TEST-doctype-rejected.xml create mode 100644 .github/actions/incremental-build/testdata/TEST-recovered-flake.xml create mode 100644 .github/actions/incremental-build/testdata/TEST-two-attempt-flake.xml create mode 100644 .github/actions/incremental-build/testdata/TEST-utf16-doctype-rejected.xml diff --git a/.github/CI-ARCHITECTURE.md b/.github/CI-ARCHITECTURE.md index 22acc2a1a3a5a..69d2e3d817965 100644 --- a/.github/CI-ARCHITECTURE.md +++ b/.github/CI-ARCHITECTURE.md @@ -102,11 +102,51 @@ The script also: - Detects tests disabled in CI (`@DisabledIfSystemProperty(named = "ci.env.name")`) - Applies an exclusion list for generated/meta modules - Checks for excluded modules with associated integration tests (via `manual-it-mapping.txt`) and advises contributors to run them manually +- Reports recovered flaky tests (see below) - Generates a unified PR comment with all test information - Parses Maven reactor output from `incremental-test.log` and reports **per-module elapsed time**, total reactor duration, and the top 20 slowest modules (see `reactor_timing.sh`) Unit tests for reactor timing parsing live in `reactor_timing_test.sh`. +#### Recovered flake reporting (`collect-flakes.py`) + +Surefire retries failing tests: `surefire.rerunFailingTestsCount` defaults to `2` +in the `full` profile of `parent/pom.xml`, and both CI systems pass it again +explicitly. A test that fails and then passes within those attempts is a +**recovered flake**. The build stays green and nothing appears in the console +output, so without this step the retry is invisible. + +`collect-flakes.py` runs on the always-path (a recovered flake means exit code 0, +so it cannot live in the failure branch where `parse_errors.sh` runs). It walks +`**/target/{surefire,failsafe}-reports/TEST-*.xml` and reports every `` +carrying ``/`` children. Tests with +``/`` failed every attempt and already fail the build, +so they are deliberately excluded. + +Two outputs: + +- A section appended to the PR comment and the job summary, naming the module, + test, attempt count and first failure message. Nothing is emitted when no test + was retried. +- `flakes.json`, uploaded as the `flakes-java-` artifact. Develocity's + flaky-test data does not cover fork PRs (`.mvn/develocity.xml` publishes build + scans only when authenticated), so this artifact is the only per-PR record. + +Notes: + +- **No time figure is reported.** Surefire records no per-attempt timing, and + `` reflects only the final successful attempt. Estimating cost + from it would understate timeout-driven flakes, which are the common kind. +- The script declares its dependencies inline via + [PEP 723](https://peps.python.org/pep-0723/) and must be run with `uv run`; + plain `python3` ignores the metadata block. `uv` is installed by the action. +- XML is parsed with `defusedxml`. A pre-parse byte scan for `=3.9" +# dependencies = ["defusedxml>=0.7.1"] +# /// + +"""Collect recovered flaky tests from surefire/failsafe XML reports. + +A recovered flake is a test that failed at least once and then passed within +the attempts allowed by ``rerunFailingTestsCount``. Surefire records those +attempts as ````/```` and reports the build as +successful, so without this script they leave no trace in CI output at all. + +Tests that failed every attempt are recorded as ````/ +````. They already fail the build and are deliberately not +collected here. + +Run with ``uv run collect-flakes.py`` so the PEP-723 dependency block above is +honoured. Plain ``python3 collect-flakes.py`` ignores it and will fail on the +defusedxml import. +""" + +import argparse +import json +import sys +from dataclasses import dataclass, replace +from pathlib import Path +from xml.etree.ElementTree import ParseError + +import defusedxml.ElementTree as ET +from defusedxml.common import DefusedXmlException + +# Surefire records a failed-then-passed attempt under these tags. +FLAKY_TAGS = ("flakyFailure", "flakyError") + +# Maven writes unit-test reports under target/surefire-reports and +# integration-test reports under target/failsafe-reports. +REPORT_DIRS = ("surefire-reports", "failsafe-reports") + + +@dataclass(frozen=True) +class Flake: + classname: str + test: str + failed_attempts: int + message: str + module: str = "" + + +def parse_report(path): + """Return the recovered flakes recorded in a single surefire/failsafe report. + + Raises ValueError if the document tries an entity-expansion or external-entity + attack. Surefire never emits a DOCTYPE, so any report that declares entities + did not come from the build. + """ + raw = Path(path).read_bytes() + try: + root = ET.fromstring(raw) + except DefusedXmlException as exc: + raise ValueError( + f"refusing hostile XML report {path}: {type(exc).__name__}" + ) from exc + + flakes = [] + for testcase in root.iter("testcase"): + attempts = [el for tag in FLAKY_TAGS for el in testcase.findall(tag)] + if not attempts: + continue + flakes.append( + Flake( + classname=testcase.get("classname", ""), + test=testcase.get("name", ""), + failed_attempts=len(attempts), + message=attempts[0].get("message", ""), + ) + ) + return flakes + + +def collect(root): + """Walk a reactor and return every recovered flake, labelled by module. + + An unreadable or hostile report is skipped with a warning rather than + aborting: this runs on the always-path of a build whose result is already + determined, and must never be the reason a job fails. + """ + root = Path(root) + flakes = [] + for reports_dir in REPORT_DIRS: + for report in sorted(root.glob(f"**/target/{reports_dir}/TEST-*.xml")): + # ...//target//TEST-*.xml + module = report.parent.parent.parent.relative_to(root).as_posix() + try: + parsed = parse_report(report) + except (ParseError, ValueError, OSError) as exc: + print(f"skipping unreadable report {report}: {exc}", file=sys.stderr) + continue + flakes.extend(replace(flake, module=module) for flake in parsed) + return flakes + + +def _ordered(flakes): + return sorted(flakes, key=lambda f: (f.module, f.classname, f.test)) + + +def to_payload(flakes): + """Build the machine-readable summary uploaded as a workflow artifact. + + Aggregating this across PRs is what turns anecdotes about flaky tests into + the evidence needed to decide which ones to quarantine. + """ + ordered = _ordered(flakes) + return { + "total_flakes": len(ordered), + "total_retried_attempts": sum(f.failed_attempts for f in ordered), + "flakes": [ + { + "module": f.module, + "classname": f.classname, + "test": f.test, + "failed_attempts": f.failed_attempts, + "message": f.message, + } + for f in ordered + ], + } + + +def _cell(text): + """Make a value safe to drop into a markdown table cell.""" + return " ".join(text.split()).replace("|", "\\|") or "(no message)" + + +def render_markdown(flakes): + """Render the PR-comment section, or an empty string when nothing was retried.""" + ordered = _ordered(flakes) + if not ordered: + return "" + + attempts = sum(f.failed_attempts for f in ordered) + noun = "test" if len(ordered) == 1 else "tests" + lines = [ + "", + f":repeat: **{len(ordered)} {noun} passed only after a retry** " + f"({attempts} retried attempts)", + "", + f"
Recovered flaky tests ({len(ordered)})", + "", + "| Module | Test | Failed attempts | First failure |", + "| --- | --- | --- | --- |", + ] + for f in ordered: + simple_class = f.classname.rsplit(".", 1)[-1] + lines.append( + f"| `{f.module}` | `{simple_class}.{f.test}` | {f.failed_attempts} " + f"| {_cell(f.message)} |" + ) + lines += [ + "", + "> :information_source: These tests did **not** fail the build. Surefire " + "retried them and they passed.", + "> Retries are enabled project-wide by `surefire.rerunFailingTestsCount` " + "in `parent/pom.xml`.", + "", + "
", + ] + return "\n".join(lines) + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("root", help="reactor root to scan for test reports") + parser.add_argument( + "--comment-file", + help="markdown file to append the flake section to (left untouched when " + "nothing was retried)", + ) + parser.add_argument("--json-out", help="path to write the machine-readable summary") + parser.add_argument( + "--step-summary", + help="path to append the section to as well, typically $GITHUB_STEP_SUMMARY", + ) + args = parser.parse_args(argv) + + flakes = collect(args.root) + section = render_markdown(flakes) + + for target in (args.comment_file, args.step_summary): + if target and section: + with open(target, "a", encoding="utf-8") as handle: + handle.write(section + "\n") + + if args.json_out: + Path(args.json_out).write_text( + json.dumps(to_payload(flakes), indent=2) + "\n", encoding="utf-8" + ) + + print(f"recovered flaky tests: {len(flakes)}") + # Never fail the job: the build verdict is already decided by this point. + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/actions/incremental-build/incremental-build.sh b/.github/actions/incremental-build/incremental-build.sh index 947c6af672adc..6ebf4be239d54 100755 --- a/.github/actions/incremental-build/incremental-build.sh +++ b/.github/actions/incremental-build/incremental-build.sh @@ -418,6 +418,38 @@ checkManualItTests() { fi } +# ── Recovered flake reporting ────────────────────────────────────────── +# +# Surefire retries failing tests (rerunFailingTestsCount, set project-wide in +# parent/pom.xml). A test that fails and then passes leaves the build green and +# leaves nothing in the console output, so it is invisible unless the XML +# reports are read. parse_errors.sh cannot cover this: it reads .txt reports, +# which carry no flake data, and it only runs when the build failed. +# +# Appends a section to the PR comment and writes flakes.json for aggregation +# across PRs. Never fails the build: by this point the verdict is already set. +reportRecoveredFlakes() { + local comment_file="$1" + local script_dir + script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + + if ! command -v uv >/dev/null 2>&1; then + echo "uv not found, skipping recovered-flake reporting" + return + fi + + local step_summary_args=() + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + step_summary_args=(--step-summary "$GITHUB_STEP_SUMMARY") + fi + + uv run --quiet "${script_dir}/collect-flakes.py" . \ + --comment-file "$comment_file" \ + --json-out flakes.json \ + "${step_summary_args[@]}" \ + || echo "WARNING: recovered-flake reporting failed, continuing" +} + # ── Scalpel shadow comparison ────────────────────────────────────────── # Write Scalpel shadow comparison section to the PR comment. @@ -964,6 +996,9 @@ main() { local comment_file="incremental-test-comment.md" writeComment "$comment_file" "$pl" "$grep_dep_module_ids" "$grep_changed_props" "$testedDependents" "$extraModules" + # Recovered flakes: a green build can still have retried its way there + reportRecoveredFlakes "$comment_file" + # Scalpel shadow comparison (observation only — after separator) # Filter reactor_ids through EXCLUSION_LIST so the comparison is # apples-to-apples: both sides exclude the same meta/generated modules. diff --git a/.github/actions/incremental-build/test_collect_flakes.py b/.github/actions/incremental-build/test_collect_flakes.py new file mode 100644 index 0000000000000..05419442e8bfb --- /dev/null +++ b/.github/actions/incremental-build/test_collect_flakes.py @@ -0,0 +1,264 @@ +#!/usr/bin/env python3 +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""Tests for collect-flakes.py. Run with: python3 -m unittest discover""" + +import contextlib +import importlib.util +import io +import json +import shutil +import tempfile +import unittest +from pathlib import Path + +HERE = Path(__file__).parent +TESTDATA = HERE / "testdata" + +_spec = importlib.util.spec_from_file_location("collect_flakes", HERE / "collect-flakes.py") +collector = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(collector) + + +class ParseReportTest(unittest.TestCase): + """A recovered flake is a test that failed at least once and then passed. + + Surefire records it as . A test that failed every attempt is + recorded as and is NOT a flake: it already fails the build. + Conflating the two is the mistake this test exists to catch. + """ + + def test_reports_only_the_test_that_passed_on_retry(self): + flakes = collector.parse_report(TESTDATA / "TEST-recovered-flake.xml") + + self.assertEqual( + ["flakyPassesOnRetry"], + [f.test for f in flakes], + "expected only the recovered flake; alwaysFails failed every attempt " + "and alwaysPasses never failed", + ) + + +class FlakeAttributionTest(unittest.TestCase): + """A flake report is only actionable if it names the test and says why it + failed. Counting attempts separates a once-in-a-blue-moon flake from one + that needed every retry surefire allowed. + """ + + def test_captures_class_and_failure_message(self): + (flake,) = collector.parse_report(TESTDATA / "TEST-recovered-flake.xml") + + self.assertEqual("org.apache.camel.component.probe.ProbeTest", flake.classname) + self.assertEqual("deliberate first-attempt failure", flake.message) + + def test_counts_every_failed_attempt_not_just_the_first(self): + (flake,) = collector.parse_report(TESTDATA / "TEST-two-attempt-flake.xml") + + self.assertEqual( + 2, + flake.failed_attempts, + "the test failed twice before passing; reporting 1 would hide how " + "close it came to failing the build", + ) + + +class CollectTest(unittest.TestCase): + """Walking the reactor has to cover unit tests (surefire) and integration + tests (failsafe), and attribute each flake to the module that owns it. A + bare class name is not enough to file a ticket against. + """ + + @staticmethod + def _place(reports_dir, fixture): + reports_dir.mkdir(parents=True) + shutil.copy(TESTDATA / fixture, reports_dir / fixture) + + def test_finds_surefire_and_failsafe_reports_and_labels_the_owning_module(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + self._place( + root / "components/camel-foo/target/surefire-reports", + "TEST-recovered-flake.xml", + ) + self._place( + root / "components/camel-bar/target/failsafe-reports", + "TEST-two-attempt-flake.xml", + ) + + found = {(f.module, f.test) for f in collector.collect(root)} + + self.assertEqual( + { + ("components/camel-bar", "connectsEventually"), + ("components/camel-foo", "flakyPassesOnRetry"), + }, + found, + ) + + def test_a_corrupt_report_is_skipped_rather_than_losing_the_whole_run(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + good = root / "components/camel-foo/target/surefire-reports" + self._place(good, "TEST-recovered-flake.xml") + (root / "components/camel-bad/target/surefire-reports").mkdir(parents=True) + ( + root / "components/camel-bad/target/surefire-reports/TEST-truncated.xml" + ).write_text(" + + + +]> + + + + + diff --git a/.github/actions/incremental-build/testdata/TEST-recovered-flake.xml b/.github/actions/incremental-build/testdata/TEST-recovered-flake.xml new file mode 100644 index 0000000000000..e3dd382f8c9c3 --- /dev/null +++ b/.github/actions/incremental-build/testdata/TEST-recovered-flake.xml @@ -0,0 +1,25 @@ + + + + + + + org.opentest4j.AssertionFailedError: <elided> + at ProbeTest.java + + + + org.opentest4j.AssertionFailedError: <elided> + at ProbeTest.java + + + + + + + org.opentest4j.AssertionFailedError: <elided> + at ProbeTest.java + + + + \ No newline at end of file diff --git a/.github/actions/incremental-build/testdata/TEST-two-attempt-flake.xml b/.github/actions/incremental-build/testdata/TEST-two-attempt-flake.xml new file mode 100644 index 0000000000000..79c1fe6a3872d --- /dev/null +++ b/.github/actions/incremental-build/testdata/TEST-two-attempt-flake.xml @@ -0,0 +1,18 @@ + + + + + + java.net.ConnectException: Connection refused + at org.apache.camel.component.probe.SlowTest.connectsEventually(SlowTest.java:44) + + + + java.net.ConnectException: Connection refused + at org.apache.camel.component.probe.SlowTest.connectsEventually(SlowTest.java:44) + + + + diff --git a/.github/actions/incremental-build/testdata/TEST-utf16-doctype-rejected.xml b/.github/actions/incremental-build/testdata/TEST-utf16-doctype-rejected.xml new file mode 100644 index 0000000000000000000000000000000000000000..5dffb5b254f354054da895c6396dbf878c1fb7de GIT binary patch literal 724 zcmb`Fu};H45JacWR~#wma7aT#NEBfsXn+DF5TYj2J3Lv=LJ zj$&QtnB1){l~DUab;Y|G+`gc2-~m|C%$h5wq5Z*9&LA~ zFKFC2XxQ(zOXnwR3R318X1x1jYlpbCi8bJX@dVqvL|(t$o=9VQnEMiDaF`I=E=uZq Te!oFW@7Kh_J!|pyURmZ7)YW Date: Sun, 23 Aug 2026 22:23:25 -0400 Subject: [PATCH 2/6] CAMEL-24290: ci - harden recovered-flake reporting after review Correctness: - Pass forbid_dtd=True explicitly. defusedxml only forbids entity *declarations* by default, so a bare parsed cleanly even though the docstring, the test class name and the docs all claimed such a report is refused. - Read flaky attempts in document order. Collecting per tag returned the first flakyFailure even when a flakyError came first, so the "First failure" column named the wrong attempt, hiding the timeout that is typically the real cause. - HTML-escape failure messages. GitHub treats "expected: but was: " as raw HTML and strips it, emptying the column for exactly the assertion messages that produce most flake reports. JDK matrix attribution: - Name the JDK in the section and record it in flakes.json (flake-label action input, --label on the script). The PR-comment artifact uploads with overwrite: true on the grounds that content is identical across the matrix; flake data is the one part that is not, so an unattributed section left the reader unable to tell which JDK a flake came from. - Upload flakes.json from main-build.yml as flakes-main-java-. The action already produced it there and it was being discarded, even though main-branch flakes are the least noisy signal available. Other: - Replace the **/target glob with a pruned os.walk. After a full build every module's target/ holds thousands of class and generated-source directories that cannot contain a report. - Write the "Tested modules" header before anything else appends to the job summary, so the reader gets what was built before what happened while building it. - Fix "1 retried attempts" and a test docstring naming a command that fails with ModuleNotFoundError. Adds three tests (18 total) plus fixtures for attempt ordering and for a DOCTYPE that declares no entities of its own. Co-Authored-By: Claude Opus 5 (1M context) --- .github/CI-ARCHITECTURE.md | 24 +++- .github/actions/incremental-build/action.yaml | 5 + .../incremental-build/collect-flakes.py | 108 +++++++++++++----- .../incremental-build/incremental-build.sh | 39 ++++--- .../incremental-build/test_collect_flakes.py | 69 ++++++++++- .../TEST-doctype-no-entities-rejected.xml | 9 ++ .../TEST-error-then-failure-flake.xml | 19 +++ .github/workflows/main-build.yml | 10 ++ .github/workflows/pr-build-main.yml | 7 ++ 9 files changed, 240 insertions(+), 50 deletions(-) create mode 100644 .github/actions/incremental-build/testdata/TEST-doctype-no-entities-rejected.xml create mode 100644 .github/actions/incremental-build/testdata/TEST-error-then-failure-flake.xml diff --git a/.github/CI-ARCHITECTURE.md b/.github/CI-ARCHITECTURE.md index 69d2e3d817965..25d43a6ee679a 100644 --- a/.github/CI-ARCHITECTURE.md +++ b/.github/CI-ARCHITECTURE.md @@ -128,21 +128,33 @@ Two outputs: - A section appended to the PR comment and the job summary, naming the module, test, attempt count and first failure message. Nothing is emitted when no test was retried. -- `flakes.json`, uploaded as the `flakes-java-` artifact. Develocity's - flaky-test data does not cover fork PRs (`.mvn/develocity.xml` publishes build - scans only when authenticated), so this artifact is the only per-PR record. +- `flakes.json`, uploaded as `flakes-java-` on PRs and + `flakes-main-java-` on `main`. Develocity's flaky-test data does not + cover fork PRs (`.mvn/develocity.xml` publishes build scans only when + authenticated), so this artifact is the only per-PR record. Notes: +- **The section names its JDK** (`flake-label` on the action, `--label` on the + script, also recorded in `flakes.json`). The PR-comment artifact is uploaded + with `overwrite: true` across the JDK matrix on the grounds that the content is + identical between entries. Flake data is the one part that is not: if JDK 17 + flakes and JDK 25 does not, whichever finishes last decides what the comment + shows. The label means the reader can tell which entry a shown flake came from, + and the per-JDK artifacts remain the complete record. + - **No time figure is reported.** Surefire records no per-attempt timing, and `` reflects only the final successful attempt. Estimating cost from it would understate timeout-driven flakes, which are the common kind. - The script declares its dependencies inline via [PEP 723](https://peps.python.org/pep-0723/) and must be run with `uv run`; plain `python3` ignores the metadata block. `uv` is installed by the action. -- XML is parsed with `defusedxml`. A pre-parse byte scan for `` through. A pre-parse byte scan for `` through. """ raw = Path(path).read_bytes() try: - root = ET.fromstring(raw) + root = ET.fromstring(raw, forbid_dtd=True) except DefusedXmlException as exc: raise ValueError( f"refusing hostile XML report {path}: {type(exc).__name__}" @@ -80,7 +84,9 @@ def parse_report(path): flakes = [] for testcase in root.iter("testcase"): - attempts = [el for tag in FLAKY_TAGS for el in testcase.findall(tag)] + # Document order, so attempts[0] really is the first attempt: grouping by + # tag would report the first flakyFailure even when a flakyError came first. + attempts = [el for el in testcase if el.tag in FLAKY_TAGS] if not attempts: continue flakes.append( @@ -94,25 +100,47 @@ def parse_report(path): return flakes +def _report_files(root): + """Yield ``(module, report path)`` for every report under a reactor root. + + os.walk rather than a ``**`` glob so the recursion can be pruned: after a + full build every module's target/ holds thousands of class and + generated-source directories, none of which can hold a report. + """ + for dirpath, dirnames, filenames in os.walk(root): + name = os.path.basename(dirpath) + parent = os.path.basename(os.path.dirname(dirpath)) + if name in REPORT_DIRS and parent == "target": + dirnames[:] = [] + # ...//target/ + module = Path(dirpath).parent.parent.relative_to(root).as_posix() + for filename in sorted(filenames): + if filename.startswith("TEST-") and filename.endswith(".xml"): + yield module, Path(dirpath) / filename + elif name == "target": + dirnames[:] = [d for d in dirnames if d in REPORT_DIRS] + else: + dirnames[:] = [d for d in dirnames if not d.startswith(".")] + + def collect(root): """Walk a reactor and return every recovered flake, labelled by module. An unreadable or hostile report is skipped with a warning rather than aborting: this runs on the always-path of a build whose result is already - determined, and must never be the reason a job fails. + determined, and must never be the reason a job fails. ParseError covers a + report truncated by a JVM killed mid-write, ValueError the hostile documents + parse_report rejects, and OSError an unreadable file. """ root = Path(root) flakes = [] - for reports_dir in REPORT_DIRS: - for report in sorted(root.glob(f"**/target/{reports_dir}/TEST-*.xml")): - # ...//target//TEST-*.xml - module = report.parent.parent.parent.relative_to(root).as_posix() - try: - parsed = parse_report(report) - except (ParseError, ValueError, OSError) as exc: - print(f"skipping unreadable report {report}: {exc}", file=sys.stderr) - continue - flakes.extend(replace(flake, module=module) for flake in parsed) + for module, report in _report_files(root): + try: + parsed = parse_report(report) + except (ParseError, ValueError, OSError) as exc: + print(f"skipping unreadable report {report}: {exc}", file=sys.stderr) + continue + flakes.extend(replace(flake, module=module) for flake in parsed) return flakes @@ -120,14 +148,17 @@ def _ordered(flakes): return sorted(flakes, key=lambda f: (f.module, f.classname, f.test)) -def to_payload(flakes): +def to_payload(flakes, label=""): """Build the machine-readable summary uploaded as a workflow artifact. Aggregating this across PRs is what turns anecdotes about flaky tests into - the evidence needed to decide which ones to quarantine. + the evidence needed to decide which ones to quarantine. ``label`` records + which matrix entry produced the data, so the aggregate can tell a test that + only flakes on one JDK from one that flakes everywhere. """ ordered = _ordered(flakes) return { + "label": label, "total_flakes": len(ordered), "total_retried_attempts": sum(f.failed_attempts for f in ordered), "flakes": [ @@ -144,24 +175,40 @@ def to_payload(flakes): def _cell(text): - """Make a value safe to drop into a markdown table cell.""" - return " ".join(text.split()).replace("|", "\\|") or "(no message)" + """Make a value safe to drop into a markdown table cell. + Angle brackets are escaped, not just passed through: assertion messages are + full of them (``expected: but was: ``) and GitHub's renderer + treats ```` as raw HTML and strips it, silently eating the message. + """ + collapsed = escape(" ".join(text.split()), quote=False) + return collapsed.replace("|", "\\|") or "(no message)" -def render_markdown(flakes): - """Render the PR-comment section, or an empty string when nothing was retried.""" + +def render_markdown(flakes, label=""): + """Render the PR-comment section, or an empty string when nothing was retried. + + ``label`` names the matrix entry the data came from (for example ``JDK 17``). + The PR-comment artifact is uploaded with overwrite: true across the JDK + matrix, and unlike the rest of the comment, flake data is genuinely not + identical between entries. Naming the entry means a reader can at least tell + which JDK a reported flake came from; the per-JDK flakes-java-* artifacts + remain the complete record. + """ ordered = _ordered(flakes) if not ordered: return "" attempts = sum(f.failed_attempts for f in ordered) noun = "test" if len(ordered) == 1 else "tests" + attempt_noun = "attempt" if attempts == 1 else "attempts" + on_label = f" on {label}" if label else "" lines = [ "", - f":repeat: **{len(ordered)} {noun} passed only after a retry** " - f"({attempts} retried attempts)", + f":repeat: **{len(ordered)} {noun} passed only after a retry{on_label}** " + f"({attempts} retried {attempt_noun})", "", - f"
Recovered flaky tests ({len(ordered)})", + f"
Recovered flaky tests{on_label} ({len(ordered)})", "", "| Module | Test | Failed attempts | First failure |", "| --- | --- | --- | --- |", @@ -197,10 +244,16 @@ def main(argv=None): "--step-summary", help="path to append the section to as well, typically $GITHUB_STEP_SUMMARY", ) + parser.add_argument( + "--label", + default="", + help="matrix entry this run covers (e.g. 'JDK 17'), named in the section " + "and recorded in the JSON so aggregation can tell the entries apart", + ) args = parser.parse_args(argv) flakes = collect(args.root) - section = render_markdown(flakes) + section = render_markdown(flakes, args.label) for target in (args.comment_file, args.step_summary): if target and section: @@ -209,7 +262,8 @@ def main(argv=None): if args.json_out: Path(args.json_out).write_text( - json.dumps(to_payload(flakes), indent=2) + "\n", encoding="utf-8" + json.dumps(to_payload(flakes, args.label), indent=2) + "\n", + encoding="utf-8", ) print(f"recovered flaky tests: {len(flakes)}") diff --git a/.github/actions/incremental-build/incremental-build.sh b/.github/actions/incremental-build/incremental-build.sh index 6ebf4be239d54..8f56c57324dda 100755 --- a/.github/actions/incremental-build/incremental-build.sh +++ b/.github/actions/incremental-build/incremental-build.sh @@ -438,15 +438,21 @@ reportRecoveredFlakes() { return fi - local step_summary_args=() + local extra_args=() if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then - step_summary_args=(--step-summary "$GITHUB_STEP_SUMMARY") + extra_args+=(--step-summary "$GITHUB_STEP_SUMMARY") + fi + # Names the matrix entry in the section. The PR comment is overwritten + # last-writer-wins across the JDK matrix, and flake data, unlike the rest of + # the comment, differs between entries. + if [ -n "${FLAKE_LABEL:-}" ]; then + extra_args+=(--label "$FLAKE_LABEL") fi uv run --quiet "${script_dir}/collect-flakes.py" . \ --comment-file "$comment_file" \ --json-out flakes.json \ - "${step_summary_args[@]}" \ + "${extra_args[@]}" \ || echo "WARNING: recovered-flake reporting failed, continuing" } @@ -996,6 +1002,20 @@ main() { local comment_file="incremental-test-comment.md" writeComment "$comment_file" "$pl" "$grep_dep_module_ids" "$grep_changed_props" "$testedDependents" "$extraModules" + # Step summary header. Written before anything else appends to the summary so + # the reader gets "what was built" before "what happened while building it". + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + { + echo "" + echo "### Tested modules" + echo "" + for w in $(echo "$final_pl" | tr ',' '\n'); do + echo "- \`$w\`" + done + echo "" + } >> "$GITHUB_STEP_SUMMARY" + fi + # Recovered flakes: a green build can still have retried its way there reportRecoveredFlakes "$comment_file" @@ -1037,19 +1057,6 @@ main() { append_reactor_timing_report "$log" "$comment_file" "$reactor_label" "${GITHUB_STEP_SUMMARY:-}" fi - # Write step summary header - if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then - { - echo "" - echo "### Tested modules" - echo "" - for w in $(echo "$final_pl" | tr ',' '\n'); do - echo "- \`$w\`" - done - echo "" - } >> "$GITHUB_STEP_SUMMARY" - fi - if [[ ${ret} -ne 0 ]]; then echo "" echo "============================================================" diff --git a/.github/actions/incremental-build/test_collect_flakes.py b/.github/actions/incremental-build/test_collect_flakes.py index 05419442e8bfb..7ee4fac4ef6f4 100644 --- a/.github/actions/incremental-build/test_collect_flakes.py +++ b/.github/actions/incremental-build/test_collect_flakes.py @@ -16,7 +16,11 @@ # limitations under the License. # -"""Tests for collect-flakes.py. Run with: python3 -m unittest discover""" +"""Tests for collect-flakes.py. + +Run with: uv run --with defusedxml python3 -m unittest discover +(plain python3 fails on the defusedxml import unless it is already installed). +""" import contextlib import importlib.util @@ -25,6 +29,7 @@ import shutil import tempfile import unittest +from dataclasses import replace from pathlib import Path HERE = Path(__file__).parent @@ -66,6 +71,16 @@ def test_captures_class_and_failure_message(self): self.assertEqual("org.apache.camel.component.probe.ProbeTest", flake.classname) self.assertEqual("deliberate first-attempt failure", flake.message) + def test_reports_the_earliest_attempt_when_the_kinds_differ(self): + (flake,) = collector.parse_report(TESTDATA / "TEST-error-then-failure-flake.xml") + + self.assertEqual( + "first attempt timed out", + flake.message, + "the column is labelled 'First failure'; reading flakyFailure before " + "flakyError would report the second attempt and hide the real cause", + ) + def test_counts_every_failed_attempt_not_just_the_first(self): (flake,) = collector.parse_report(TESTDATA / "TEST-two-attempt-flake.xml") @@ -159,9 +174,42 @@ def test_names_the_module_test_and_attempt_count(self): self.assertIn("2", rendered) self.assertIn("Connection refused", rendered) + def test_escapes_a_message_github_would_otherwise_render_as_html(self): + rendered = collector.render_markdown( + [replace(self.FLAKES[0], message="expected: but was: ")] + ) + + self.assertIn( + "expected: <true> but was: <false>", + rendered, + "GitHub strips as raw HTML, which would empty the column for " + "the assertion messages that produce most flake reports", + ) + + def test_names_the_matrix_entry_so_the_overwritten_comment_stays_readable(self): + rendered = collector.render_markdown(self.FLAKES, "JDK 25") + + self.assertIn( + "on JDK 25", + rendered, + "the PR comment is overwritten last-writer-wins across the JDK " + "matrix, so an unlabelled section leaves the reader unable to tell " + "which JDK the flake came from", + ) + + def test_omits_the_label_entirely_when_no_matrix_entry_was_given(self): + rendered = collector.render_markdown(self.FLAKES) + + self.assertNotIn( + " on ", + rendered.splitlines()[1], + "a single-entry caller must not get a dangling 'on ' in the heading", + ) + def test_json_payload_records_the_total_and_the_detail(self): payload = collector.to_payload(self.FLAKES) + self.assertEqual("", payload["label"]) self.assertEqual(1, payload["total_flakes"]) self.assertEqual(2, payload["total_retried_attempts"]) self.assertEqual( @@ -177,6 +225,17 @@ def test_json_payload_records_the_total_and_the_detail(self): payload["flakes"], ) + def test_json_payload_records_the_matrix_entry_for_cross_run_aggregation(self): + payload = collector.to_payload(self.FLAKES, "JDK 17") + + self.assertEqual( + "JDK 17", + payload["label"], + "aggregating these artifacts is the point, and it cannot " + "distinguish a test that only flakes on one JDK from one that " + "flakes everywhere unless each payload says which JDK it is", + ) + class MainTest(unittest.TestCase): """This runs on the always-path of a build whose verdict is already decided. @@ -251,6 +310,14 @@ def test_rejects_a_report_containing_a_doctype(self): with self.assertRaises(ValueError): collector.parse_report(TESTDATA / "TEST-doctype-rejected.xml") + def test_rejects_a_doctype_that_declares_no_entities_of_its_own(self): + """defusedxml forbids entity *declarations* by default but allows a bare + DOCTYPE, so forbid_dtd has to be requested explicitly. Without it an + external subset pointing at an attacker-controlled DTD is accepted. + """ + with self.assertRaises(ValueError): + collector.parse_report(TESTDATA / "TEST-doctype-no-entities-rejected.xml") + def test_rejects_a_doctype_hidden_by_a_non_utf8_encoding(self): """A byte-level scan for b' + + + + + + + diff --git a/.github/actions/incremental-build/testdata/TEST-error-then-failure-flake.xml b/.github/actions/incremental-build/testdata/TEST-error-then-failure-flake.xml new file mode 100644 index 0000000000000..4839a9b064469 --- /dev/null +++ b/.github/actions/incremental-build/testdata/TEST-error-then-failure-flake.xml @@ -0,0 +1,19 @@ + + + + + + java.util.concurrent.TimeoutException + at org.apache.camel.component.probe.OrderTest.errorsThenFails(OrderTest.java:31) + + + + org.opentest4j.AssertionFailedError: <elided> + at org.apache.camel.component.probe.OrderTest.errorsThenFails(OrderTest.java:31) + + + + diff --git a/.github/workflows/main-build.yml b/.github/workflows/main-build.yml index 8ec9178f06b81..9f34d3a6ad262 100644 --- a/.github/workflows/main-build.yml +++ b/.github/workflows/main-build.yml @@ -91,9 +91,19 @@ jobs: with: github-token: ${{ secrets.GITHUB_TOKEN }} skip-mvnd-install: 'true' + flake-label: JDK ${{ matrix.java }} - name: archive incremental test logs uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: always() with: name: incremental-test-main-java-${{ matrix.java }}.log path: incremental-test.log + # Main-branch flakes are the least noisy signal there is: no PR-specific + # changes to blame, so a retry here points at the test, not the diff. + # The action already produces flakes.json — without this it is discarded. + - name: Upload recovered-flake report + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() && hashFiles('flakes.json') != '' + with: + name: flakes-main-java-${{ matrix.java }} + path: flakes.json diff --git a/.github/workflows/pr-build-main.yml b/.github/workflows/pr-build-main.yml index 0eafeaa1a0e7d..2fd063f6d9fd5 100644 --- a/.github/workflows/pr-build-main.yml +++ b/.github/workflows/pr-build-main.yml @@ -198,6 +198,7 @@ jobs: skip-mvnd-install: 'true' extra-modules: ${{ inputs.extra_modules || '' }} maven-extra-args: ${{ matrix.maven_extra_args || '' }} + flake-label: JDK ${{ matrix.java }} - name: archive incremental test logs uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: always() @@ -219,6 +220,12 @@ jobs: # comment file actually exists — a cancelled build (e.g., JDK 25 killed # while JDK 17 fails) won't have the file and must not overwrite an # artifact from a matrix entry that did produce it. + # + # The recovered-flake section is the one part that is NOT identical across + # JDKs, so it names its JDK (flake-label above) and the reader can tell + # which entry it came from. Last writer still wins, so the comment shows + # one JDK's flakes; the per-JDK flakes-java-* artifacts above are the + # complete record. - name: Save PR number and test comment for commenter workflow if: always() && !matrix.experimental shell: bash From 60596ee57357d199d61070f511794e9f8c3349a9 Mon Sep 17 00:00:00 2001 From: Adriano Machado <60320+ammachado@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:48:37 -0400 Subject: [PATCH 3/6] CAMEL-24290: ci - address review comments on recovered-flake reporting - Escape module, class, and test name through _cell() in the markdown table row, not just the failure message. A test name containing '|' (a JUnit 5 @ParameterizedTest display name, or a Camel URI/DSL parameterized test) otherwise splits the row into the wrong columns. - Gate the "Upload recovered-flake report" step on !matrix.experimental for consistency with the other post-build steps. Co-Authored-By: Claude Sonnet 5 --- .github/actions/incremental-build/collect-flakes.py | 4 ++-- .../incremental-build/test_collect_flakes.py | 13 +++++++++++++ .github/workflows/pr-build-main.yml | 2 +- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/.github/actions/incremental-build/collect-flakes.py b/.github/actions/incremental-build/collect-flakes.py index 67f07fcd477c1..86cdd3e338561 100644 --- a/.github/actions/incremental-build/collect-flakes.py +++ b/.github/actions/incremental-build/collect-flakes.py @@ -214,9 +214,9 @@ def render_markdown(flakes, label=""): "| --- | --- | --- | --- |", ] for f in ordered: - simple_class = f.classname.rsplit(".", 1)[-1] + simple_class = _cell(f.classname.rsplit(".", 1)[-1]) lines.append( - f"| `{f.module}` | `{simple_class}.{f.test}` | {f.failed_attempts} " + f"| `{_cell(f.module)}` | `{simple_class}.{_cell(f.test)}` | {f.failed_attempts} " f"| {_cell(f.message)} |" ) lines += [ diff --git a/.github/actions/incremental-build/test_collect_flakes.py b/.github/actions/incremental-build/test_collect_flakes.py index 7ee4fac4ef6f4..b07c526d1d78c 100644 --- a/.github/actions/incremental-build/test_collect_flakes.py +++ b/.github/actions/incremental-build/test_collect_flakes.py @@ -186,6 +186,19 @@ def test_escapes_a_message_github_would_otherwise_render_as_html(self): "the assertion messages that produce most flake reports", ) + def test_escapes_a_pipe_in_the_test_name_so_it_cannot_split_the_row(self): + rendered = collector.render_markdown( + [replace(self.FLAKES[0], test="[1] input=|extra")] + ) + + self.assertIn( + "<script>alert(1)</script>\\|extra", + rendered, + "a JUnit 5 @ParameterizedTest display name or a Camel URI/DSL " + "parameterized test routinely contains '|', which would otherwise " + "split the row into the wrong columns", + ) + def test_names_the_matrix_entry_so_the_overwritten_comment_stays_readable(self): rendered = collector.render_markdown(self.FLAKES, "JDK 25") diff --git a/.github/workflows/pr-build-main.yml b/.github/workflows/pr-build-main.yml index 2fd063f6d9fd5..384fc61673b68 100644 --- a/.github/workflows/pr-build-main.yml +++ b/.github/workflows/pr-build-main.yml @@ -210,7 +210,7 @@ jobs: # across PRs is what turns flakiness into evidence instead of anecdote. - name: Upload recovered-flake report uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - if: always() && hashFiles('flakes.json') != '' + if: always() && !matrix.experimental && hashFiles('flakes.json') != '' with: name: flakes-java-${{ matrix.java }} path: flakes.json From cbda9e9020acf8b0364911ef6861275b7f68de84 Mon Sep 17 00:00:00 2001 From: Adriano Machado <60320+ammachado@users.noreply.github.com> Date: Tue, 25 Aug 2026 09:53:37 -0400 Subject: [PATCH 4/6] CAMEL-24290: ci - pin uv and Python versions for reproducible builds Pins `version`/`python-version` on the setup-uv step so the recovered-flake collector runs against a fixed uv (0.12.5) and Python (3.11) rather than whatever "latest" resolves to at run time, even though the setup-uv action itself is already SHA-pinned. Co-Authored-By: Claude Sonnet 5 --- .github/actions/incremental-build/action.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/actions/incremental-build/action.yaml b/.github/actions/incremental-build/action.yaml index 945b35e824cfa..8cd0866696ecf 100644 --- a/.github/actions/incremental-build/action.yaml +++ b/.github/actions/incremental-build/action.yaml @@ -59,6 +59,8 @@ runs: uses: astral-sh/setup-uv@20cfd1bf945f4377ade1205e4dbc17946fc9a30d # v10.0.1 with: enable-cache: true + version: "0.12.5" + python-version: "3.11" - name: maven test env: GITHUB_TOKEN: ${{ inputs.github-token }} From f03024aa1b06139db01176ad61f9af5048b66bba Mon Sep 17 00:00:00 2001 From: Adriano Machado <60320+ammachado@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:12:11 -0400 Subject: [PATCH 5/6] Update .github/workflows/pr-ci-scripts-validation.yml Co-authored-by: Guillaume Nodet - AI Bot --- .github/workflows/pr-ci-scripts-validation.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pr-ci-scripts-validation.yml b/.github/workflows/pr-ci-scripts-validation.yml index 0966eb007ca94..8e8ae4c8dadcd 100644 --- a/.github/workflows/pr-ci-scripts-validation.yml +++ b/.github/workflows/pr-ci-scripts-validation.yml @@ -48,13 +48,13 @@ jobs: python3 -m unittest discover --verbose # collect-flakes.py declares defusedxml inline (PEP 723); uv resolves it. + - name: Install uv - name: Install uv uses: astral-sh/setup-uv@20cfd1bf945f4377ade1205e4dbc17946fc9a30d # v10.0.1 with: enable-cache: true - - - name: Test the incremental build scripts - shell: bash + version: "0.12.5" + python-version: "3.11" run: | cd .github/actions/incremental-build uv run --with defusedxml python3 -m unittest discover --verbose From 6a89cfc0ea0ef9bed1ddcc15f0c80d0be304784c Mon Sep 17 00:00:00 2001 From: Adriano Machado <60320+ammachado@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:21:49 -0400 Subject: [PATCH 6/6] CAMEL-24290: ci - fix validation workflow broken by applied suggestion The review suggestion applied in f03024aa replaced a narrower line range than it covered, leaving a duplicate empty "Install uv" step and folding the test step's `run:` into the `uses:` step. GitHub Actions rejects both, so the validation workflow failed to run. Restore the separate "Test the incremental build scripts" step while keeping the uv 0.12.5 / Python 3.11 pins the suggestion intended to add. Net change versus cbda9e9 is only those two pin lines. _Claude Code on behalf of Adriano Machado (@ammachado)_ _This was generated by an AI agent and may contain inaccuracies. Please verify before relying on it._ Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-ci-scripts-validation.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr-ci-scripts-validation.yml b/.github/workflows/pr-ci-scripts-validation.yml index 8e8ae4c8dadcd..c8f859b2e11c0 100644 --- a/.github/workflows/pr-ci-scripts-validation.yml +++ b/.github/workflows/pr-ci-scripts-validation.yml @@ -48,13 +48,15 @@ jobs: python3 -m unittest discover --verbose # collect-flakes.py declares defusedxml inline (PEP 723); uv resolves it. - - name: Install uv - name: Install uv uses: astral-sh/setup-uv@20cfd1bf945f4377ade1205e4dbc17946fc9a30d # v10.0.1 with: enable-cache: true version: "0.12.5" python-version: "3.11" + + - name: Test the incremental build scripts + shell: bash run: | cd .github/actions/incremental-build uv run --with defusedxml python3 -m unittest discover --verbose