Introduce Flaky-Test Detection - #798
Conversation
Introduce reusable composite actions to support CI, particularly the nightly flaky-test detection pipeline: - setup_qnx_environment: installs QEMU, enables KVM permissions and provisions the QNX license, with a post step that cleans the license up. Replaces the inline QNX/QEMU setup previously duplicated in build_and_test_qnx. - upload_bazel_testlogs_on_failure: collects and uploads bazel-testlogs as an artifact when a test step fails, wired into the gcc15, ASan/UBSan/ LSan, TSan and QNX workflows. - prepare_bazel_environment: add an optional repository-cache input so callers can opt into the Bazel repository cache. Also ignore the local .worktrees/ directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aaa089b to
617c082
Compare
Add a scheduled/dispatchable workflow that repeatedly runs the test suite across all supported configurations to surface flaky tests. - nightly_flaky_detection.yml orchestrates per-configuration runner jobs (gcc15 unit/integration, ASan/UBSan/LSan, TSan, QNX unit/integration) via the reusable _nightly_flaky_detection_runner.yml, using --runs_per_test with --runs_per_test_detects_flakes and collecting the Bazel build event protocol output per config. - collect_flaky_tests.py parses the per-config BEP into a structured report; merge_flaky_reports.py aggregates all configs into a single per-target summary (JSON + Markdown step summary) and exposes counts as workflow outputs. - Tests exclude themselves from detection via the no-flaky-test-detection tag; integration tests are tagged integration-test so the runner can select unit vs integration suites with --test_tag_filters. - README_flaky_detection.md documents the pipeline; the scripts are packaged as Bazel py_binary/py_test targets with unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the ephemeral report-only model with self-updating GitHub issues, so each flaky test target has a single durable, deduplicated issue that accumulates history across nightly runs. - sync_flaky_issues.py reconciles the merged flaky-target summary against existing issues: it creates one issue per target on first detection and, on recurrence, increments a cumulative counter, refreshes a managed stats region in the body and appends a run comment. Per-run comments are the source of truth (cumulative totals are recomputed from them), the run_id makes reruns idempotent, and closed issues are reopened rather than duplicated. Robust to human edits: a corrupted managed body block is rebuilt from the comment ledger. - The aggregate job gains issues: write permission and a sync step that runs after the report is built. - Unit tests cover creation, recurrence increment, idempotency, reopen and corrupted-body recovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
limdor
left a comment
limdor
left a comment
There was a problem hiding this comment.
I started doing a review like if this would be tooling code that is quite critical, that is why at the beginning there are quite some comments.
However, this is something that it is not critical and we know that it brings us value. Part of the value comes from the ration effort spend / value provided. Considering that most of it has been done by AI + human driven (with the human checking the overall direction), the effort spend is not so high. We already detected flaky tests that we were not aware, this means that the value provided is already quite important.
In the moment that we start goingh through a strict review process for something that it is just nice to have, the effort / value decreases quite a lot (the author will have to spend much more time and also the reviwer will have to spend much more time).
Because of that, I decided to stop the review mid way and just focus on the overall direction.
I think the direction is good and that is something we want.
The risk of not doing a full review or leaving the code as it and not addressing some of the comments provided, is that if we have a bug in this tooling it might be hard to fix/find.
The main issues I find with the code is that it might be hard to understand for a human when going deeper (not from a high level point of view). If at some point we figure out that we have too many bugs in this code, we can throw it away and do it proper/better.
Until then, I think the direction is good enough and the value important enough that we should proceed with this even if the quality in some parts might be questionable.
| } | ||
|
|
||
| function main() { | ||
| const licenseDir = process.env["INPUT_LICENSE-DIR"] || "/opt/score_qnx/license"; |
There was a problem hiding this comment.
Nitpick: We are defining the default here but also in the yml file.
| const { execSync } = require("node:child_process"); | ||
|
|
||
| function main() { | ||
| const licenseDir = process.env.STATE_LICENSE_DIR || "/opt/score_qnx/license"; |
There was a problem hiding this comment.
Nitpick: And as in 1cb242a#r3680485623, we also define the default one more time
| failed_count: ${{ steps.summary.outputs.failed_count }} | ||
| test_exit_code: ${{ steps.run-tests.outputs.test_exit_code }} | ||
| steps: | ||
| - uses: actions/checkout@v6.0.2 |
There was a problem hiding this comment.
Just note, nothing to do: This version is consistent with what we have in the others, but at some point we should consider updating it. There ia already a new patch version (6.0.3), a new minor version (6.0.1), and a new major version (7.0.1)
|
|
||
| - name: Upload per-config flaky report | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
Just note, nothing to do: This version is consistent with what we have in the others, but at some point we should consider updating it. There ia already a v5, v6, and v7
| id: summary | ||
| if: always() | ||
| run: | | ||
| REPORT_DIR="${RUNNER_TEMP}/nightly-flaky/${{ inputs.config-name }}" |
There was a problem hiding this comment.
This nightly-flaky appears in several places. It would be good if we could add a variable somewhere for the id and then reuse it.
| total_runs_value = summary.get("totalRunCount") | ||
| if total_runs_value is None: | ||
| total_runs_value = summary.get("runCount", 0) |
There was a problem hiding this comment.
Same here, it looks like a get with default
| if total_runs <= 0: | ||
| total_runs = len(summary.get("failed", [])) + len(summary.get("passed", [])) | ||
|
|
||
| current = target_stats.get(label, {"status": None, "failed_runs": 0, "total_runs": 0}) |
There was a problem hiding this comment.
This current comes from the BEP? I do not get what this contains and why we have to merge it. Maybe we can discuss it offline. If you have a link that explain that content of the BEP regarding test execution it might help.
| target_stats[label] = current | ||
|
|
||
|
|
||
| def _parse_raw_log(path: Path, target_stats: dict[str, dict]) -> None: |
There was a problem hiding this comment.
Same comments like _parse_bep when it comes to input/output parameter and silently returning if the path is not a file.
| parser.add_argument("--raw-log", required=True) | ||
| parser.add_argument("--output-dir", required=True) | ||
| parser.add_argument("--test-exit-code", required=True, type=int) | ||
| parser.add_argument("--runs-per-test", required=False, type=int, default=1000) |
There was a problem hiding this comment.
This is only for collecting and generating a report. Do we really want to provide a default here? This will not match most probably with the default that it was executed.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
There was a problem hiding this comment.
Why not exit with sys.exit? First time I see we raise an exception directly. Is there an advantage?
2 participants
No description provided.