From 0a9eddb4e3e2b2227c204375458bf943e6542422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Desbiens?= Date: Wed, 26 Aug 2026 08:36:21 -0400 Subject: [PATCH] Enforced a coverage floor on the merged report The coverage summary reported a percentage and could not fail. Coverage could fall from 99.97% to anything at all and every check stayed green, against an AGENTS.md that asks for 100% test coverage -- a stated requirement measured with a gauge that had no failure mode. CodeCoverageSummary already takes thresholds and fail_below_min; neither was set. Both are now, through a new coverage_thresholds input on the template, because the two suites do not sit at the same figure: ThreadX 99, SMP 98. Three things were probed against the pinned action on a runner before picking those numbers, using the real merged.xml files from the dev push run of #666. The floor compares the line rate and nothing else. That mattered because branch coverage is around 78% in both suites while line coverage is 98.8-100%, so a floor aimed at the line figure would have been an immediate red wall had it tested branches or the lower of the two. The ThreadX report at 100.00% lines and 77.67% branches clears a floor of 99. The thresholds are whole numbers. '99.9 100' -- the value this was meant to be -- is rejected with 'System.ArgumentException - Threshold parameter set incorrectly.', and the step fails whether or not fail_below_min is set. So the choice is 99 or 100 with nothing between. 100 would fail on a race. tx_thread_system_resume.c:529 is reached by timing rather than by construction and flaps between runs of the same green tree, which is why #666 left it; 4502/4503 fails a floor of 100 and clears one of 99. A coverage gate that goes red on a coin toss is how coverage gates get switched off. SMP is 5114/5178 lines, 98.76%, with 64 uncovered lines across 11 files of common_smp/src -- #666 closed the equivalent gaps in common/src only. A shared floor of 99 would have failed that job on every run while ThreadX passed. One limit is recorded in the file rather than fixed: an empty report reads as 100%. gcovr writes line-rate="1.0" beside lines-valid="0" when it finds no data, and the action prints 'Line Rate = 100% (0 / 0)' and passes any floor. The check for that is the emptiness assertion #664 put in each suite's coverage.sh, not this one. Also corrected two stale filenames in the deploy job's comment: since #665 each coverage artifact carries merged.xml, not default_build_coverage.xml. Verified on the runner. Assisted-by: Claude Opus 5 --- .github/workflows/regression_template.yml | 55 ++++++++++++++++++++--- .github/workflows/regression_test.yml | 20 +++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/.github/workflows/regression_template.yml b/.github/workflows/regression_template.yml index 7eea8f432..ad2d42aa7 100644 --- a/.github/workflows/regression_template.yml +++ b/.github/workflows/regression_template.yml @@ -38,6 +38,33 @@ on: default: 'merged' required: false type: string + # The lower and upper threshold percentages handed to + # CodeCoverageSummary, lower first. The lower one is a hard floor: the + # step below sets fail_below_min, so a run whose merged line rate falls + # under it turns the job red. + # + # Both must be whole numbers. Probed against the pinned action on a + # runner, 26 Aug 2026: '99.9 100' is rejected with + # 'System.ArgumentException - Threshold parameter set incorrectly.' and + # the step fails whether or not fail_below_min is set, because the action + # parses each half with an integer parse. So a floor can be 99 or 100 and + # nothing between, and the intended 99.9 is not expressible. + # + # The floor is compared against the LINE rate only -- not the branch + # rate, and not the lower of the two. Probed on the same run: the + # ThreadX report at 100.00% lines and 77.67% branches passes a floor of + # 99. Worth knowing, because branch coverage is around 78% in both + # suites while AGENTS.md asks for 100%, and a floor set from the + # headline line figure says nothing about it. + # + # Each suite sets its own in regression_test.yml, because they do not + # sit at the same figure. The default here is the action's own, low + # enough to be no floor at all in practice -- a caller that collects + # coverage is expected to name its number. + coverage_thresholds: + default: '50 75' + required: false + type: string skip_deploy: default: false required: false @@ -128,6 +155,18 @@ jobs: # so a cancelled run still stops promptly -- the same idiom the # deploy_code_coverage job below already uses. The ${{ }} is required: a bare # ! opens a YAML tag, and the expression will not parse without it. + # + # fail_below_min turns the summary into a gate. Until now coverage could + # fall from any figure to any other and no check went red, against an + # AGENTS.md that asks for 100% -- a stated requirement measured with a + # gauge that could not fail. + # + # One thing the floor does not defend, and it is the likeliest way for + # coverage to break: an empty report reads as 100%. gcovr writes + # line-rate="1.0" beside lines-valid="0" when it finds no data, and the + # action reports 'Line Rate = 100% (0 / 0)' and passes any floor -- + # probed on a runner, 26 Aug 2026. The check that catches that is the + # emptiness assertion in each suite's coverage.sh, not this one. - name: Generate Code Coverage Results Summary if: ${{ !cancelled() && (!inputs.skip_coverage) }} uses: irongut/CodeCoverageSummary@51cc3a756ddcd398d447c044c02cb6aa83fdae95 # v1.3.0 @@ -137,6 +176,8 @@ jobs: badge: true hide_complexity: true output: file + thresholds: ${{ inputs.coverage_thresholds }} + fail_below_min: true - name: Write Code Coverage Summary if: ${{ !cancelled() && (!inputs.skip_coverage) }} @@ -239,13 +280,15 @@ jobs: # merge-multiple puts the contents of both coverage artifacts directly # into coverage_report rather than under a directory named for each # artifact. Each one holds a single directory named for its suite, - # ThreadX or SMP, renamed from default_build_coverage by the "Prepare - # Coverage GitHub Pages" step, so the merge yields exactly the two - # suite directories the deploy expects, and the artifact name -- with - # its timestamp -- stops appearing in the published path at all. + # ThreadX or SMP, renamed from merged by the "Prepare Coverage GitHub + # Pages" step, so the merge yields exactly the two suite directories the + # deploy expects, and the artifact name -- with its timestamp -- stops + # appearing in the published path at all. # - # The two artifacts also each carry a default_build_coverage.xml, and - # the merge means one overwrites the other. That file is consumed by + # The two artifacts also each carry a merged.xml -- it was + # default_build_coverage.xml until #665 instrumented every configuration + # and unioned them -- and the merge means one overwrites the other. + # Verified on the runner: both hold merged.xml. That file is consumed by # CodeCoverageSummary back in run_tests and is not read here, so this is # untidy rather than wrong. - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 diff --git a/.github/workflows/regression_test.yml b/.github/workflows/regression_test.yml index 2aebd7b41..a1e6f0c0a 100644 --- a/.github/workflows/regression_test.yml +++ b/.github/workflows/regression_test.yml @@ -37,6 +37,17 @@ jobs: cmake_path: ./test/tx/cmake result_affix: ThreadX skip_deploy: true + # 99 is the tightest floor available below this suite's figure. The + # merged report reads 4503/4503 lines -- 100.00% -- on the dev push run + # of #666, and two local runs of the same green tree came in at 100.00% + # and 99.98%: tx_thread_system_resume.c:529 is reached by timing rather + # than by construction and flaps between runs, and #666 could not reach + # it deliberately. So a floor of 100 is a check that fails on a race -- + # probed against the pinned action, 4502/4503 fails a floor of 100 and + # clears one of 99 -- and the thresholds are integers, so there is + # nothing between the two. Ratchet this to 100 if 529 is ever covered + # by construction. + coverage_thresholds: '99 100' smp: permissions: contents: read @@ -52,6 +63,15 @@ jobs: cmake_path: ./test/smp/cmake result_affix: SMP skip_deploy: true + # Lower than ThreadX's, because this suite is: the merged report reads + # 5114/5178 lines -- 98.76% -- on the same run, with 64 uncovered lines + # across 11 files of common_smp/src. #666 closed the equivalent gaps in + # common/src only, so common_smp still carries them, plus a family of + # its own in the *_delete.c teardown paths and tx_byte_pool_search.c. + # A shared floor of 99 would therefore red-wall this job on every run + # while ThreadX passed -- probed against the pinned action. Raise this + # towards 99 as those lines are covered. + coverage_thresholds: '98 100' freertos: permissions: contents: read