diff --git a/.github/CI-ARCHITECTURE.md b/.github/CI-ARCHITECTURE.md index 90139a08698bf..3962855a28a00 100644 --- a/.github/CI-ARCHITECTURE.md +++ b/.github/CI-ARCHITECTURE.md @@ -102,8 +102,60 @@ 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 +#### 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 `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`, with `forbid_dtd=True` passed explicitly — + the default only forbids entity *declarations*, which would let a bare + `` through. 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 os +import sys +from dataclasses import dataclass, replace +from html import escape +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 carries a DOCTYPE or tries an + entity-expansion or external-entity attack. Surefire never emits a DOCTYPE, + so any report that declares one did not come from the build. forbid_dtd has + to be passed explicitly: defusedxml only forbids entity *declarations* by + default, which would let a bare `` through. + """ + raw = Path(path).read_bytes() + try: + root = ET.fromstring(raw, forbid_dtd=True) + except DefusedXmlException as exc: + raise ValueError( + f"refusing hostile XML report {path}: {type(exc).__name__}" + ) from exc + + flakes = [] + for testcase in root.iter("testcase"): + # 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( + Flake( + classname=testcase.get("classname", ""), + test=testcase.get("name", ""), + failed_attempts=len(attempts), + message=attempts[0].get("message", ""), + ) + ) + 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. 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 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 + + +def _ordered(flakes): + return sorted(flakes, key=lambda f: (f.module, f.classname, f.test)) + + +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. ``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": [ + { + "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. + + 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, 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{on_label}** " + f"({attempts} retried {attempt_noun})", + "", + f"
Recovered flaky tests{on_label} ({len(ordered)})", + "", + "| Module | Test | Failed attempts | First failure |", + "| --- | --- | --- | --- |", + ] + for f in ordered: + simple_class = _cell(f.classname.rsplit(".", 1)[-1]) + lines.append( + f"| `{_cell(f.module)}` | `{simple_class}.{_cell(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", + ) + 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, args.label) + + 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, args.label), 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 d33270cb4bc5e..29a1d6f1affec 100755 --- a/.github/actions/incremental-build/incremental-build.sh +++ b/.github/actions/incremental-build/incremental-build.sh @@ -413,6 +413,44 @@ 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 extra_args=() + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + 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 \ + "${extra_args[@]}" \ + || echo "WARNING: recovered-flake reporting failed, continuing" +} + # ── Scalpel shadow comparison ────────────────────────────────────────── # Write Scalpel shadow comparison section to the PR comment. @@ -944,6 +982,23 @@ 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" + # 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. @@ -1009,19 +1064,6 @@ main() { fi 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 new file mode 100644 index 0000000000000..b07c526d1d78c --- /dev/null +++ b/.github/actions/incremental-build/test_collect_flakes.py @@ -0,0 +1,344 @@ +#!/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: 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 +import io +import json +import shutil +import tempfile +import unittest +from dataclasses import replace +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_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") + + 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(" as raw HTML, which would empty the column for " + "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") + + 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( + [ + { + "module": "components/camel-probe", + "classname": "org.apache.camel.component.probe.SlowTest", + "test": "connectsEventually", + "failed_attempts": 2, + "message": "Connection refused", + } + ], + 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. + It appends to the existing comment rather than replacing it, and never + reports failure. + """ + + def _reactor_with_one_flake(self, root): + reports = root / "components/camel-probe/target/surefire-reports" + reports.mkdir(parents=True) + shutil.copy( + TESTDATA / "TEST-two-attempt-flake.xml", + reports / "TEST-two-attempt-flake.xml", + ) + + def test_appends_to_the_existing_comment_and_writes_the_json_artifact(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + self._reactor_with_one_flake(root) + comment = root / "incremental-test-comment.md" + comment.write_text(":test_tube: **CI tested the following modules:**\n") + payload_file = root / "flakes.json" + + with contextlib.redirect_stdout(io.StringIO()): + exit_code = collector.main( + [str(root), "--comment-file", str(comment), "--json-out", str(payload_file)] + ) + + text = comment.read_text() + payload = json.loads(payload_file.read_text()) + + self.assertEqual(0, exit_code) + self.assertTrue( + text.startswith(":test_tube:"), "must not clobber the existing comment" + ) + self.assertIn("connectsEventually", text) + self.assertEqual(1, payload["total_flakes"]) + self.assertEqual(2, payload["total_retried_attempts"]) + + def test_writes_a_zero_payload_and_no_comment_section_on_a_clean_run(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + comment = root / "incremental-test-comment.md" + comment.write_text("original\n") + payload_file = root / "flakes.json" + + with contextlib.redirect_stdout(io.StringIO()): + exit_code = collector.main( + [str(root), "--comment-file", str(comment), "--json-out", str(payload_file)] + ) + + text = comment.read_text() + payload = json.loads(payload_file.read_text()) + + self.assertEqual(0, exit_code) + self.assertEqual("original\n", text) + self.assertEqual( + 0, + payload["total_flakes"], + "a clean run must still record zero, so aggregating artifacts across " + "PRs does not mistake a missing file for a missing flake", + ) + + +class DoctypeRejectionTest(unittest.TestCase): + """Surefire never emits a DOCTYPE, so any report carrying one is not a + surefire report. Refusing it up front closes entity-expansion DoS without + pulling in a third-party XML parser. + """ + + 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-doctype-rejected.xml b/.github/actions/incremental-build/testdata/TEST-doctype-rejected.xml new file mode 100644 index 0000000000000..e213817db623d --- /dev/null +++ b/.github/actions/incremental-build/testdata/TEST-doctype-rejected.xml @@ -0,0 +1,13 @@ + + + + +]> + + + + + 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/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 0000000000000..5dffb5b254f35 Binary files /dev/null and b/.github/actions/incremental-build/testdata/TEST-utf16-doctype-rejected.xml differ diff --git a/.github/workflows/main-build.yml b/.github/workflows/main-build.yml index 5270ffaf4b84f..4196fe078ce1c 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 7b9a9e4689f2c..9fa392bd47486 100644 --- a/.github/workflows/pr-build-main.yml +++ b/.github/workflows/pr-build-main.yml @@ -23,6 +23,7 @@ on: - main # CI-only changes don't need a full build. Use workflow_dispatch to # test CI changes: gh workflow run "Build and test" -f pr_number=XXXX -f pr_ref=branch-name + # -f pr_repo=owner/camel (pr_repo only needed when the branch lives on a fork) paths-ignore: - .claude-plugin/** - .idea/** @@ -44,6 +45,11 @@ on: description: 'Git ref of the pull request branch' required: true type: string + pr_repo: + description: 'Repository the branch lives on (owner/camel), only needed for a fork branch' + required: false + type: string + default: '' extra_modules: description: 'Additional modules to test (comma-separated paths, e.g. from /component-test)' required: false @@ -83,6 +89,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + repository: ${{ inputs.pr_repo || github.repository }} ref: ${{ inputs.pr_ref || '' }} - name: Fetch base branch for Scalpel change detection if: ${{ !inputs.skip_full_build }} @@ -170,18 +177,34 @@ 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() with: name: incremental-test-java-${{ matrix.java }}.log path: incremental-test.log + # Recovered flakes (tests that failed then passed on retry) leave the build + # green, so this artifact is the only durable record of them. Aggregating it + # 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() && !matrix.experimental && hashFiles('flakes.json') != '' + with: + name: flakes-java-${{ matrix.java }} + path: flakes.json # All non-experimental JDK matrix entries upload with overwrite: true. # The comment content is identical across JDKs (same modules tested), # so last writer wins. However, we only upload (and overwrite) when the # 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 diff --git a/.github/workflows/pr-ci-scripts-validation.yml b/.github/workflows/pr-ci-scripts-validation.yml index 9697d7c676f21..0966eb007ca94 100644 --- a/.github/workflows/pr-ci-scripts-validation.yml +++ b/.github/workflows/pr-ci-scripts-validation.yml @@ -22,6 +22,7 @@ on: - main paths: - '.github/actions/check-container-upgrade/**' + - '.github/actions/incremental-build/**' concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} @@ -45,3 +46,15 @@ jobs: run: | cd .github/actions/check-container-upgrade python3 -m unittest discover --verbose + + # collect-flakes.py declares defusedxml inline (PEP 723); uv resolves it. + - name: Install uv + uses: astral-sh/setup-uv@20cfd1bf945f4377ade1205e4dbc17946fc9a30d # v10.0.1 + with: + enable-cache: true + + - name: Test the incremental build scripts + shell: bash + run: | + cd .github/actions/incremental-build + uv run --with defusedxml python3 -m unittest discover --verbose