From 8d2731820c3e1c3058e99ac4f0d973a4ee133c19 Mon Sep 17 00:00:00 2001 From: Anton Krivoborodov Date: Thu, 24 Sep 2026 15:44:25 +0000 Subject: [PATCH 1/4] ci: name the modules whose tests failed quality_runners.py already records an exit code per module, but the job summary it renders uses the columns module/passed/failed/skipped/total. A module whose bazel invocation aborted during analysis therefore shows up as all zeroes - indistinguishable from one that simply has no tests, and `failed` even claims zero failures. In #355 that hid two broken modules behind these rows: | score_logging | 0 | 0 | 0 | 0 | | score_config_management | 0 | 0 | 0 | 0 | Add a status column, emit ::error annotations so GitHub names the module above the step list of the run, and close the log with an explicit block naming the failures instead of a pprint dump. Mirror the summary into a single comment on the pull request, rewritten on every run. The job summary is only reachable behind the "Details" link, so the conversation view showed nothing beyond "test_and_docs - Failing after 45m". Also skip coverage extraction when the test run failed. genhtml reads the .dat file from a fixed location, which after a failed run still holds the previous module's data: the coverage report published for score_config_management in #355 in fact contained score_time's numbers. --- .github/workflows/test_and_docs.yml | 36 +++++++++++++++ scripts/quality_runners.py | 69 ++++++++++++++++++++++++----- 2 files changed, 95 insertions(+), 10 deletions(-) diff --git a/.github/workflows/test_and_docs.yml b/.github/workflows/test_and_docs.yml index 0e8a9e49c40..cb407acfe6a 100644 --- a/.github/workflows/test_and_docs.yml +++ b/.github/workflows/test_and_docs.yml @@ -89,6 +89,11 @@ jobs: # push run without --trust-cache would save a cache that contains zero test results, # leaving every later PR run with nothing to hit. run: | + # pull_request_target checks out the PR branch, so these two files could + # be planted by the PR itself. They are published to the job summary and + # to a PR comment, so make sure nothing but this run can write them. + rm -f docs/verification_report/unit_test_summary.md \ + docs/verification_report/coverage_summary.md python ./scripts/quality_runners.py \ ${{ github.ref_type != 'tag' && '--trust-cache' || '' }} - name: Execute Feature Integration Tests @@ -115,6 +120,37 @@ jobs: else echo "No coverage summary file found (docs/verification_report/coverage_summary.md)" >> "$GITHUB_STEP_SUMMARY" fi + # The job summary above only shows up behind the "Details" link of the check. + # Mirror it into a single comment on the pull request so that a failing module + # is named in the conversation itself. The comment is rewritten on every run + # instead of appended, so a PR never collects more than one of them. + - name: Comment test summary on pull request + if: ${{ always() && github.event_name == 'pull_request_target' }} + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs') + const marker = '' + const read = (p) => fs.existsSync(p) ? fs.readFileSync(p, 'utf8') : `_${p} was not produced._` + const { owner, repo } = context.repo + const issue_number = context.payload.pull_request.number + const runUrl = + `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}` + const body = [ + marker, + read('docs/verification_report/unit_test_summary.md'), + read('docs/verification_report/coverage_summary.md'), + `[Full log of this run](${runUrl})`, + ].join('\n\n') + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number, + }) + const previous = comments.find((c) => c.body.includes(marker)) + if (previous) { + await github.rest.issues.updateComment({ owner, repo, comment_id: previous.id, body }) + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body }) + } - name: Create archive of test reports if: github.ref_type == 'tag' run: | diff --git a/scripts/quality_runners.py b/scripts/quality_runners.py index 2cdddde4965..063a1559125 100644 --- a/scripts/quality_runners.py +++ b/scripts/quality_runners.py @@ -16,7 +16,6 @@ import sys from dataclasses import dataclass from pathlib import Path -from pprint import pprint from subprocess import PIPE, Popen, run from known_good.models.known_good import load_known_good @@ -175,6 +174,51 @@ def generate_markdown_report( output_path.write_text(md) +def with_status(data: dict[str, dict[str, int]]) -> dict[str, dict[str, int]]: + """Derive a readable status column from the exit code each runner reports. + + Without it a module whose Bazel invocation aborted during analysis is + indistinguishable from one that simply has no tests: both show up as all + zeroes, and ``failed`` even claims zero failures. + """ + return { + name: {**stats, "status": "pass" if stats.get("exit_code", 0) == 0 else "FAILED"} + for name, stats in data.items() + } + + +def report_failures(unit_tests: dict[str, dict[str, int]], coverage: dict[str, dict[str, int]]) -> list[str]: + """Name every module that failed, via annotations and a final summary block. + + ``::error`` annotations are rendered by GitHub above the step list of the + run, so the failing module is visible without opening the log at all. + """ + failed = sorted(name for name, stats in unit_tests.items() if stats.get("exit_code", 0) != 0) + + for name in failed: + print( + f"::error title=Unit tests failed::{name}: bazel exited with " + f"{unit_tests[name]['exit_code']} and produced no test results" + ) + for name, stats in coverage.items(): + if stats.get("exit_code", 0) != 0: + print(f"::error title=Coverage failed::{name}: coverage extraction did not succeed") + + print_centered("QR: UNIT TEST EXECUTION SUMMARY", fillchar="=") + for name, stats in sorted(unit_tests.items()): + if stats.get("exit_code", 0) == 0: + print(f" pass {name:<26} {stats['passed']:>6} passed, {stats['skipped']:>3} skipped") + for name in failed: + print(f" FAILED {name:<26} bazel exit code {unit_tests[name]['exit_code']}, no results") + + if failed: + print_centered( + f"QR: {len(failed)} of {len(unit_tests)} MODULES FAILED: {', '.join(failed)}", + fillchar="=", + ) + return failed + + def extract_ut_summary(logs: str) -> dict[str, int]: summary = {"passed": 0, "failed": 0, "skipped": 0, "total": 0} @@ -338,6 +382,14 @@ def main() -> bool: print_centered(f"QR: Testing module: {module.name}") unit_tests_summary[module.name] = run_unit_test_with_coverage(module=module, trust_cache=args.trust_cache) + # Coverage extraction reads the .dat file Bazel leaves in a fixed + # location. When the test run failed, that file is still the one the + # previous module produced, so genhtml would silently report another + # module's numbers under this module's name. + if unit_tests_summary[module.name]["exit_code"] != 0: + print_centered(f"QR: Skipping coverage for {module.name}: unit test run failed") + continue + if "cpp" in module.metadata.langs: coverage_summary[f"{module.name}_cpp"] = run_cpp_coverage_extraction( module=module, output_path=args.coverage_output_dir @@ -357,22 +409,19 @@ def main() -> bool: print_centered(f"QR: Finished testing module: {module.name}") generate_markdown_report( - unit_tests_summary, + with_status(unit_tests_summary), title="Unit Test Execution Summary", - columns=["module", "passed", "failed", "skipped", "total"], + columns=["module", "status", "passed", "failed", "skipped", "total"], output_path=path_to_docs / "unit_test_summary.md", ) - print_centered("QR: UNIT TEST EXECUTION SUMMARY", fillchar="=") - pprint(unit_tests_summary, width=120) - generate_markdown_report( - coverage_summary, + with_status(coverage_summary), title="Coverage Analysis Summary", - columns=["module", "lines", "functions", "branches"], + columns=["module", "status", "lines", "functions", "branches"], output_path=path_to_docs / "coverage_summary.md", ) - print_centered("QR: COVERAGE ANALYSIS SUMMARY", fillchar="=") - pprint(coverage_summary, width=120) + + report_failures(unit_tests_summary, coverage_summary) # Check all exit codes and return non-zero if any test or coverage extraction failed return any(r["exit_code"] != 0 for r in {**unit_tests_summary, **coverage_summary}.values()) From 3eed724aa2b80c463fe88efbfe6fa8108abf6eb8 Mon Sep 17 00:00:00 2001 From: Anton Krivoborodov Date: Thu, 24 Sep 2026 15:48:17 +0000 Subject: [PATCH 2/4] ci: list skipped coverage runs instead of dropping them Skipping the extraction removed the module from the coverage table entirely, which is silent in the same way the zero rows were. Emit a row with status 'skipped' instead, and suppress the coverage annotation for those, since the unit test annotation for the same module already names the cause. The run of #355 shows why the table cannot simply be trusted: the coverage published for the two modules whose tests never ran was character-identical to the module tested just before them. score_config_management_cpp lines 90.8% functions 84.4% branches 67.1% score_time_cpp lines 90.8% functions 84.4% branches 67.1% score_logging_cpp lines 77.6% functions 81.8% branches 51.2% score_lifecycle_cpp lines 77.6% functions 81.8% branches 51.2% --- scripts/quality_runners.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/scripts/quality_runners.py b/scripts/quality_runners.py index 063a1559125..8aaa28a59fc 100644 --- a/scripts/quality_runners.py +++ b/scripts/quality_runners.py @@ -179,10 +179,11 @@ def with_status(data: dict[str, dict[str, int]]) -> dict[str, dict[str, int]]: Without it a module whose Bazel invocation aborted during analysis is indistinguishable from one that simply has no tests: both show up as all - zeroes, and ``failed`` even claims zero failures. + zeroes, and ``failed`` even claims zero failures. An explicit status set by + the caller (``skipped``) wins over the derived one. """ return { - name: {**stats, "status": "pass" if stats.get("exit_code", 0) == 0 else "FAILED"} + name: {**stats, "status": stats.get("status") or ("pass" if stats.get("exit_code", 0) == 0 else "FAILED")} for name, stats in data.items() } @@ -201,7 +202,9 @@ def report_failures(unit_tests: dict[str, dict[str, int]], coverage: dict[str, d f"{unit_tests[name]['exit_code']} and produced no test results" ) for name, stats in coverage.items(): - if stats.get("exit_code", 0) != 0: + # A coverage run that was skipped is already covered by the unit test + # annotation for the same module; annotating it again is just noise. + if stats.get("exit_code", 0) != 0 and stats.get("status") != "skipped": print(f"::error title=Coverage failed::{name}: coverage extraction did not succeed") print_centered("QR: UNIT TEST EXECUTION SUMMARY", fillchar="=") @@ -388,6 +391,8 @@ def main() -> bool: # module's numbers under this module's name. if unit_tests_summary[module.name]["exit_code"] != 0: print_centered(f"QR: Skipping coverage for {module.name}: unit test run failed") + for lang in module.metadata.langs: + coverage_summary[f"{module.name}_{lang}"] = {"exit_code": 1, "status": "skipped"} continue if "cpp" in module.metadata.langs: From 2ba8c1d1175ac468ddd2b108898a66fc94491c89 Mon Sep 17 00:00:00 2001 From: Anton Krivoborodov Date: Thu, 24 Sep 2026 15:52:08 +0000 Subject: [PATCH 3/4] ci: colour the status column with an emoji prefix Markdown has no way to colour a table cell that survives both a GitHub comment and the Sphinx build of the same file, so carry the colour in an emoji and keep the word next to it for readers without emoji support. --- scripts/quality_runners.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/scripts/quality_runners.py b/scripts/quality_runners.py index 8aaa28a59fc..0d84f2a87bf 100644 --- a/scripts/quality_runners.py +++ b/scripts/quality_runners.py @@ -174,6 +174,9 @@ def generate_markdown_report( output_path.write_text(md) +STATUS_LABELS = {"pass": "✅ pass", "FAILED": "❌ FAILED", "skipped": "⚪ skipped"} + + def with_status(data: dict[str, dict[str, int]]) -> dict[str, dict[str, int]]: """Derive a readable status column from the exit code each runner reports. @@ -181,9 +184,16 @@ def with_status(data: dict[str, dict[str, int]]) -> dict[str, dict[str, int]]: indistinguishable from one that simply has no tests: both show up as all zeroes, and ``failed`` even claims zero failures. An explicit status set by the caller (``skipped``) wins over the derived one. + + The emoji carries the colour: Markdown offers no way to colour a table cell + that survives both GitHub and the Sphinx build of these same files. The word + stays next to it so the table is still readable where emoji are not. """ return { - name: {**stats, "status": stats.get("status") or ("pass" if stats.get("exit_code", 0) == 0 else "FAILED")} + name: { + **stats, + "status": STATUS_LABELS[stats.get("status") or ("pass" if stats.get("exit_code", 0) == 0 else "FAILED")], + } for name, stats in data.items() } From 4d11bf77f927bbaeecba4201a994eb4c5de903bb Mon Sep 17 00:00:00 2001 From: Anton Krivoborodov Date: Fri, 25 Sep 2026 08:04:03 +0000 Subject: [PATCH 4/4] ci: write the job summary from the script instead of commenting on the PR Review feedback: the pull-requests:write scope should not gain another user, and the summary is better assembled where the data already exists. The script now appends the two tables to GITHUB_STEP_SUMMARY directly, so the shell step that cat'ed the files and the github-script step that mirrored them into a pull request comment both go away. Nothing reads the markdown files back any more - they are written purely for the documentation build - which also removes the need to delete them up front. The failing modules stay visible without opening the log: the ::error annotations render above the step list of the run. --- .github/workflows/test_and_docs.yml | 50 ----------------------------- scripts/quality_runners.py | 24 ++++++++++++-- 2 files changed, 21 insertions(+), 53 deletions(-) diff --git a/.github/workflows/test_and_docs.yml b/.github/workflows/test_and_docs.yml index cb407acfe6a..5fd870535c2 100644 --- a/.github/workflows/test_and_docs.yml +++ b/.github/workflows/test_and_docs.yml @@ -89,11 +89,6 @@ jobs: # push run without --trust-cache would save a cache that contains zero test results, # leaving every later PR run with nothing to hit. run: | - # pull_request_target checks out the PR branch, so these two files could - # be planted by the PR itself. They are published to the job summary and - # to a PR comment, so make sure nothing but this run can write them. - rm -f docs/verification_report/unit_test_summary.md \ - docs/verification_report/coverage_summary.md python ./scripts/quality_runners.py \ ${{ github.ref_type != 'tag' && '--trust-cache' || '' }} - name: Execute Feature Integration Tests @@ -106,51 +101,6 @@ jobs: - name: Build build tools SBOM run: | bazel build --lockfile_mode=error //:build_tools_sbom - - name: Publish build summary - if: always() - run: | - if [ -f docs/verification_report/unit_test_summary.md ]; then - cat docs/verification_report/unit_test_summary.md >> "$GITHUB_STEP_SUMMARY" - else - echo "No build summary file found (docs/verification_report/unit_test_summary.md)" >> "$GITHUB_STEP_SUMMARY" - fi - echo "" >> "$GITHUB_STEP_SUMMARY" # Add a newline for better formatting - if [ -f docs/verification_report/coverage_summary.md ]; then - cat docs/verification_report/coverage_summary.md >> "$GITHUB_STEP_SUMMARY" - else - echo "No coverage summary file found (docs/verification_report/coverage_summary.md)" >> "$GITHUB_STEP_SUMMARY" - fi - # The job summary above only shows up behind the "Details" link of the check. - # Mirror it into a single comment on the pull request so that a failing module - # is named in the conversation itself. The comment is rewritten on every run - # instead of appended, so a PR never collects more than one of them. - - name: Comment test summary on pull request - if: ${{ always() && github.event_name == 'pull_request_target' }} - uses: actions/github-script@v7 - with: - script: | - const fs = require('fs') - const marker = '' - const read = (p) => fs.existsSync(p) ? fs.readFileSync(p, 'utf8') : `_${p} was not produced._` - const { owner, repo } = context.repo - const issue_number = context.payload.pull_request.number - const runUrl = - `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.runId}` - const body = [ - marker, - read('docs/verification_report/unit_test_summary.md'), - read('docs/verification_report/coverage_summary.md'), - `[Full log of this run](${runUrl})`, - ].join('\n\n') - const comments = await github.paginate(github.rest.issues.listComments, { - owner, repo, issue_number, - }) - const previous = comments.find((c) => c.body.includes(marker)) - if (previous) { - await github.rest.issues.updateComment({ owner, repo, comment_id: previous.id, body }) - } else { - await github.rest.issues.createComment({ owner, repo, issue_number, body }) - } - name: Create archive of test reports if: github.ref_type == 'tag' run: | diff --git a/scripts/quality_runners.py b/scripts/quality_runners.py index 0d84f2a87bf..01b2d6b2a4a 100644 --- a/scripts/quality_runners.py +++ b/scripts/quality_runners.py @@ -11,6 +11,7 @@ # SPDX-License-Identifier: Apache-2.0 # ******************************************************************************* import argparse +import os import re import select import sys @@ -159,7 +160,7 @@ def generate_markdown_report( title: str, columns: list[str], output_path: Path = Path("unit_test_summary.md"), -) -> None: +) -> str: # Build header and separator title = f"# {title}\n" header = "| " + " | ".join(columns) + " |" @@ -172,6 +173,22 @@ def generate_markdown_report( md = "\n".join([title, header, separator] + rows + [""]) output_path.write_text(md) + return md + + +def append_to_step_summary(*blocks: str) -> None: + """Mirror the reports into the job summary GitHub shows above the log. + + Writing straight from the data keeps what gets published tied to this run. + The markdown files are still needed by the documentation build, but nothing + reads them back, so a stale or hand-edited copy cannot be mistaken for a + result. Outside of Actions the variable is unset and this does nothing. + """ + step_summary = os.environ.get("GITHUB_STEP_SUMMARY") + if not step_summary: + return + with open(step_summary, "a", encoding="utf-8") as handle: + handle.write("\n".join(blocks)) STATUS_LABELS = {"pass": "✅ pass", "FAILED": "❌ FAILED", "skipped": "⚪ skipped"} @@ -423,18 +440,19 @@ def main() -> bool: print_centered(f"QR: Finished testing module: {module.name}") - generate_markdown_report( + unit_tests_md = generate_markdown_report( with_status(unit_tests_summary), title="Unit Test Execution Summary", columns=["module", "status", "passed", "failed", "skipped", "total"], output_path=path_to_docs / "unit_test_summary.md", ) - generate_markdown_report( + coverage_md = generate_markdown_report( with_status(coverage_summary), title="Coverage Analysis Summary", columns=["module", "status", "lines", "functions", "branches"], output_path=path_to_docs / "coverage_summary.md", ) + append_to_step_summary(unit_tests_md, coverage_md) report_failures(unit_tests_summary, coverage_summary)