ci: always report LAVA results; even on failure - #594
ci: always report LAVA results; even on failure#594Christopher Obbard (obbardc) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Ensures LAVA results remain visible when test submission jobs fail.
Changes:
- Runs result-publishing jobs after failures, excluding skipped or cancelled runs.
- Adds the triggering build workflow link to PR comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/test-on-pr.yml |
Publishes failed test results and links the originating build run. |
.github/workflows/lava-test.yml |
Publishes available LAVA artifacts despite submission failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Loïc Minier (lool)
left a comment
Loïc Minier (lool)
left a comment
There was a problem hiding this comment.
Loïc asked Claude to review this PR and here's what it found.
Overall: the fix looks correct. Four things worth raising before merge, one of which is a concrete suggestion to fold in.
The fix itself is right
The root cause diagnosis is accurate. A job with needs: and no if: carries an implicit success(), so a single failing submit-job matrix leg skipped publish-test-results entirely — discarding the results of every board that did run, from both the check run/PR comment and the job summary. fail-fast: false on the matrix meant the other legs ran to completion and then had their results thrown away.
!cancelled() is the right primitive (unlike always(), it still honours cancellation), and the second clause is meaningful rather than defensive padding in both files:
lava-test.yml:submit-jobgenuinely reachesskippedviaif: needs.prepare-job-list.outputs.has_jobs == 'true'— either no LAVA template matched the board filters, orprepare-job-listfailed (empty output →'' == 'true'→ false).test-on-pr.yml:testis reportedskippedwhenretrieve-build-urlis skipped (build conclusion ≠ success) or fails. Both mean "nothing to report", which matches the comment.
The inline comments explain why rather than restating the expression.
1. The total-failure case now posts a near-empty comment
Worth deciding whether you care. When no board produces results, nothing in the chain objects:
actions/download-artifactinpattern:mode succeeds silently on zero matches (it filters the artifact list and logs only at debug level, then reports success) — unlikename:or ID mode, which throw.EnricoMi/publish-unit-test-result-actionv2.24.0 defaults toaction_fail: falseandaction_fail_on_inconclusive: false.
So on total failure the job now runs green and posts a PR comment containing just the heading plus the new "Triggered by build workflow run" line, with no job bullets, alongside a contentless "Test Results" check run. Previously it skipped silently. Arguably still an improvement — the "Triggered by" link at least gets you to the failed run — but if you want it to read as a failure rather than as an empty success, either emit an explicit "no test jobs produced results" line when the find loop finds nothing, or set action_fail_on_inconclusive: true.
2. Concrete suggestion: report_test_results_externally: false on the test: call in test-on-pr.yml
This is the one worth actually changing. test-on-pr.yml calls lava-test.yml without report_test_results_externally, so it defaults to true — meaning lava-test.yml's own publish-test-results job also does check_run: true and comment_mode: always, in addition to test-on-pr.yml's publish job.
The inner one has no commit: / event_file: / event_name: overrides, so under workflow_run it attaches its check run to github.sha, which for that trigger is the default branch tip, not the PR head. test-on-pr.yml's own publish job is the one that gets this right (commit: ${{ github.event.workflow_run.head_sha }}, event_file, event_name).
That mis-targeting is pre-existing and already happens on the happy path, so it's not a regression — but this PR makes it fire on the failure path too, which is where a stray red check on main would be most noticeable. Passing report_test_results_externally: false on that call would leave the inner job's job-summary table intact while letting test-on-pr.yml own all external reporting.
3. Blast radius is wider than the description suggests
lava-test.yml has more callers than the PR body implies. With external reporting left at its true default: build.yml, build-on-push.yml, test-on-pr.yml, and — via linux.yml — linux-qcom-next.yml and linux-arduino.yml. (Off for build-debian.yml, linux-next.yml, linux-mainline.yml.)
So partial LAVA failures in the daily qcom-next and arduino kernel builds, the daily build.yml run, and pushes to main will now all start producing red "Test Results" check runs, and comment_mode: always will comment on the associated (already-merged) PRs where previously the job just skipped. That's presumably the intended behaviour — it's the whole point — but it's a visible change in CI noise on main beyond the PR path, and worth a sentence in the commit message or PR body so nobody is surprised by the first red check.
4. The "can't be tested before merge" claim is only half true
The description says this is difficult to test because workflow_run-triggered workflows always run the default-branch copy of the file. That's correct for test-on-pr.yml — no way around it.
It doesn't hold for the lava-test.yml half, though. Every scheduled caller (build.yml, build-debian.yml, linux-*.yml) explicitly permits workflow_dispatch from any branch of the main repo, with comments to that effect: "manual runs are allowed from any branch of the main repository so that changes to the workflows can be tested before they are merged." A manual dispatch of build.yml from this branch would exercise the new guard for real, including the partial-failure path if a board is currently flaky. Given that's the change with the widest reach, it seems worth doing rather than merging on inspection.
Minor
- The third commit (
69be2d44, the build-run link in the PR comment) is a genuinely useful addition but isn't mentioned in the PR title or body — worth a line so it doesn't look like drive-by scope creep. A link to the test run might be at least as useful as the build run, since that's where the failure will be. - All three commits are well scoped with why-focused messages and
Signed-off-by:trailers.
Adjacent, not this PR: the mirror-image footgun in linux.yml
While tracing the callers, two jobs in linux.yml stood out — an explicit if: on a job with needs: that doesn't check status:
debos-linux-deb:
if: ${{ !inputs.skip_image_build }}
needs: build-linux-deb
test-linux-deb:
if: ${{ !inputs.skip_lava_tests }}
needs: debos-linux-debBecause an explicit if: replaces the implicit success(), these should run even when their dependency fails — so test-linux-deb would submit LAVA jobs with an empty url after a failed debos build. That's the exact inverse of the bug this PR fixes. Worth confirming against a real failed run rather than taking this at face value; if it holds, && success() on both (or needs.<job>.result == 'success') would be a good follow-up issue. Out of scope here.
Loïc Minier (lool)
left a comment
Loïc Minier (lool)
left a comment
There was a problem hiding this comment.
Thanks, I asked both copilot and claude for a review of this PR; copilot found nothing, but claude had interesting things to say
Looking at the claude review, I think the report_test_results_externally (point 2) would be good to change before landing, otherwise we'll continue attaching test run info to the wrong commits. The rest can be improved later, it was also already there.
The `publish-test-results` job only depends on `submit-job`, so GitHub skips the job whenever any board's submit job fails (e.g. LAVA submission error, job timeout, job failure, etc). The results of the boards which did run are then lost; neither the check run nor the job summary listing the LAVA jobs is produced for any caller of this workflow. Guard the job with `!cancelled()` instead so it runs on failure and keep skipping it when `submit-job` did not run at all, e.g. when no test jobs match the board filters. Now that the summary is written even when every board failed to submit, say so explicitly when there are no job details to list: an empty list otherwise reads as if there had been nothing to test. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The `publish-test-results` job only depends on `test`, so GitHub skips it whenever the lava-test workflow it calls fails, e.g. because one board's test job failed. The comment listing the LAVA job links is then never added to the pull request; the results of the boards which did run are not published either. Guard the job with `!cancelled()` instead so it runs on failure and keep skipping it when the test workflow itself did not run (e.g. the build URL could not be retrieved), as there is nothing to report in that case. As the comment is now posted even when every board failed to submit, say so when there are no job links to list, rather than commenting a bare heading which reads as if the tests had passed. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The comment lists the individual LAVA jobs, but nothing points back at the "Build on PR" run whose image they tested. Add a link to it above the job list, taken from the `html_url` of the `workflow_run` payload which triggered this workflow. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
The `test` job calls lava-test.yml without setting `report_test_results_externally`, which defaults to true, so that the workflow's own `publish-test-results` job creates a check run and comments on the pull request as well. It has no way to know which commit is being tested: under `workflow_run`, `github.sha` is the tip of the default branch, not the pull request head, so its check run is attached to the wrong commit. The `publish-test-results` job in this workflow gets this right as it passes `commit`, `event_file` and `event_name` explicitly. Set `report_test_results_externally: false` on the call so this workflow owns all external reporting; the job summary written by lava-test.yml is unaffected. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
A job with `needs:` and no `if:` gets an implicit `success()`, but an explicit `if:` replaces it rather than adding to it. Four jobs which carry a condition of their own therefore run whatever their dependency did: `test` in build.yml, `test-debian` in build-debian.yml and `debos-linux-deb` and `test-linux-deb` in linux.yml. Once the build they need fails, the image URL they pass on is empty and LAVA jobs are submitted against nothing, which is the exact inverse of the missing results this branch otherwise fixes. Check the result of the dependency in the condition. Comparing it against 'success' rather than calling `success()` also covers the case where the dependency was skipped, e.g. when build-linux-deb is guarded out because the workflow runs from a fork. Signed-off-by: Christopher Obbard <chris.obbard@oss.qualcomm.com>
69be2d4 to
4c5331c
Compare
Currently the LAVA results are not published (to workflow summary or PR comments) if a LAVA job fails. Fix it so the results are always published.
See these sample workflow runs to see broken behaviour:
build.yml:test-on-pr.yml:Effect on other callers
lava-test.ymlhas more callers than just the PR path and external reporting (report_test_results_externally) is left at itstruedefault forbuild.yml,build-on-push.ymland vialinux.yml,linux-qcom-next.ymlandlinux-arduino.yml. It is off forbuild-debian.yml,linux-next.ymlandlinux-mainline.yml.Because
publish-test-resultspreviously skipped whenever any board's submit job failed, partial LAVA failures in those runs reported nothing at all. They will now publish: expect "Test Results" check runs on the dailybuild.ymland qcom-next/arduino kernel runs and on pushes tomainand, sincecomment_modeisalways, comments on the associated, already-merged PRs. That is the intended effect of the fix, but it is a visible increase in CI noise beyond the PR path, so the first failure should not come as a surprise.The PR path itself does not gain a duplicate:
test-on-pr.ymlnow passesreport_test_results_externally: false, because the inner job has no way to know the PR head SHA underworkflow_runand was attaching its check run to the default-branch tip.test-on-pr.yml's own publish job does the external reporting with the correct commit.Testing
The
test-on-pr.ymlhalf cannot be exercised before merge:workflow_runalways runs the default-branch copy of the workflow file. Thelava-test.ymlhalf can: every scheduled caller permitsworkflow_dispatchfrom any branch of the main repo, so dispatchingbuild.ymlfrom this branch exercises the new guard, including the partial-failure path if a board is currently flaky.