ci: name the modules whose tests failed - #368
Conversation
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.
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%
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.
|
The created documentation from the pull request is available at: docu-html |
| # 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 |
There was a problem hiding this comment.
those files should not be committed, if we will add rm in ci pipeline we could miss that they were edited manually.
imo it doesnt prevent the problem, manual review is probably best solution here
There was a problem hiding this comment.
Dropped in 4d11bf7 — and with it the reason it was there.
The summary is now assembled from the script's own data and appended to $GITHUB_STEP_SUMMARY directly, so nothing reads these two files back any more. A stale or hand-edited copy can no longer be mistaken for a result of the run, which is what the rm was guarding against.
On the files themselves you're right, and it's worth noting what is actually committed today: both are three-line placeholders (## Template for a table with Unit Test execution summary), untouched since #226, kept only so the toctree in platform_verification_report.rst resolves. Removing them means teaching the docs build to generate them or to tolerate their absence — a separate change I'd rather not fold into this PR. Happy to open an issue for it if you think it's worth tracking.
| # 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 |
There was a problem hiding this comment.
Im not a fan of adding comments, as they require token with write perm etc. and goal is to reduce that to minimum in the future.
I would prefer it to be added to SUMMARY via env vars.
There was a problem hiding this comment.
Removed in 4d11bf7. quality_runners.py now appends both tables to $GITHUB_STEP_SUMMARY itself, which also let the Publish build summary shell step go away — the data is already in memory at that point, so reading the files back was a detour.
One check on the wording: I read "added to SUMMARY via env vars" as the script writing to the file GITHUB_STEP_SUMMARY points at. If you meant keeping a shell step and passing the content through a variable instead, say so and I'll switch.
One factual note, since it matters for the wider goal rather than for this PR: pull-requests: write is already declared at lines 17, 40 and 173 and already in use — deploy-versioned-pages from cicd-workflows posts the docu-html comment on every PR using find-comment + create-or-update-comment. So the scope can't be dropped from this workflow until that one changes too. Your point about not adding a second consumer of it still stands, hence the change.
What stays visible without the comment: the ::error annotations name the failing modules above the step list of the run and in the Checks tab, so it's one click instead of zero — the conversation itself only shows test_and_docs — Failing.
…e 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.
…overage-dashboards Resolve merge conflict in scripts/quality_runners.py by preserving both the status column reporting and failure annotations from main (eclipse-score#368) alongside the code coverage portal generation and interactive dashboard links.
2 participants
quality_runners.pyalready records an exit code per module, but the job summary it renders only has the columnsmodule, passed, failed, skipped, total. A module whose Bazel invocation aborted during analysis therefore appears as all zeroes - indistinguishable from one that simply has no tests, whilefailedactively claims zero failures.In #355 that is what hid two broken modules:
The data was there all along - the same run prints this to the log:
Changes
Status column. Derived from the exit code that is already collected:
Annotations. GitHub renders these above the step list of the run, so the failing module is named without opening the log:
Explicit closing block instead of the
pprintdump, as the last output of the script:PR comment. The job summary is only reachable behind the
Detailslink, so the conversation view of a PR showed nothing beyondtest_and_docs - Failing after 45m. The summary is now mirrored into a single comment that is rewritten on every run, so a PR never accumulates more than one.Coverage after a failed test run is skipped.
genhtmlreads the.datfile from a fixed location, which after a failed run still holds the data of the previously tested module. In #355 the coverage report published forscore_config_managementin fact containedscore_time's numbers:Note on pull_request_target
The workflow checks out the PR branch, so the two summary files could be planted by a PR and would then be published to the job summary and the comment. The test step now removes them before running, so only the run itself can write them.