ci: fix 'Run Go Tests' required check never reporting on PRs#5389
Open
taliesin-ai wants to merge 1 commit into
Open
ci: fix 'Run Go Tests' required check never reporting on PRs#5389taliesin-ai wants to merge 1 commit into
taliesin-ai wants to merge 1 commit into
Conversation
The test_go matrix job reports per-platform checks (e.g. 'Run Go Tests (ubuntu-22.04, 1.23)'),
so no single 'Run Go Tests' check is ever posted. The branch protection rule requires exactly
that name, leaving all PRs permanently stuck on 'Expected'.
Add a go_test_results fan-in job that:
- runs after all matrix legs with if: always()
- reports as exactly 'Run Go Tests' to match the branch protection context
- passes when test_go succeeds or is skipped, or when skip_tests succeeds (sponsor updates)
- has minimal permissions (permissions: {}) to satisfy CodeQL
Closes wailsapp#5316
Contributor
WalkthroughThe PR updates the ChangesCI Workflow Test Results Aggregation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-master.yml:
- Around line 128-145: The aggregation job go_test_results currently treats a
skipped test_go as success; update the workflow so go_test_results only runs or
exits 0 when tests actually ran: either (A) tighten the go_test_results job
condition to match test_go (same event filters/conditions that cause test_go to
run), or (B) add an explicit runtime check inside go_test_results that treats
the combination test_result=="skipped" && skip_result=="skipped" as failure
(exit non‑zero) instead of success; reference the job names test_go, skip_tests
and the variables test_result/skip_result when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b1630cc-b5d7-4975-819b-0f168cf9bdb1
📒 Files selected for processing (1)
.github/workflows/pr-master.yml
Comment on lines
+128
to
+145
| - name: Check matrix result | ||
| run: | | ||
| test_result="${{ needs.test_go.result }}" | ||
| skip_result="${{ needs.skip_tests.result }}" | ||
|
|
||
| if [[ "$test_result" == "success" || "$test_result" == "skipped" ]]; then | ||
| echo "Go tests result: $test_result — passed" | ||
| exit 0 | ||
| fi | ||
|
|
||
| if [[ "$skip_result" == "success" ]]; then | ||
| echo "Tests skipped (sponsor update) — passed" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Go tests result: $test_result" | ||
| echo "Skip tests result: $skip_result" | ||
| exit 1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Analyze when test_go vs go_test_results execute based on event types
echo "=== test_go conditions ==="
echo "Runs when:"
echo " - repo == 'wailsapp/wails'"
echo " - head.ref != 'update-sponsors'"
echo " - AND one of:"
echo " * event_name == 'workflow_dispatch'"
echo " * base.ref == 'master' AND event_name == 'pull_request'"
echo " * base.ref == 'master' AND event_name == 'pull_request_review' AND review.state == 'approved'"
echo ""
echo "=== go_test_results conditions ==="
echo "Runs when:"
echo " - always() [even if dependencies fail/skip]"
echo " - repo == 'wailsapp/wails'"
echo " - AND one of:"
echo " * event_name == 'workflow_dispatch'"
echo " * base.ref == 'master'"
echo ""
echo "=== Problematic scenario ==="
echo "Event: pull_request_review with review.state='changes_requested' on PR to master"
echo "Branch: feature-branch (not update-sponsors)"
echo ""
echo "Result:"
echo " test_go: SKIPPED (review.state != 'approved')"
echo " skip_tests: SKIPPED (head.ref != 'update-sponsors')"
echo " go_test_results: RUNS (base.ref == 'master' and always())"
echo " Check status: PASSES (line 133 catches test_result == 'skipped')"
echo ""
echo "⚠️ This creates a passing check without running any tests!"Repository: wailsapp/wails
Length of output: 1046
🏁 Script executed:
cat -n .github/workflows/pr-master.yml | sed -n '1,160p'Repository: wailsapp/wails
Length of output: 5406
Critical: Aggregation logic passes without verifying tests actually ran.
The go_test_results job can pass when both test_go and skip_tests are skipped, without any actual tests executing in the workflow run.
Problematic scenario: A pull_request_review event with a non-approved review on a PR targeting master (and branch is not update-sponsors):
test_gois skipped (condition requiresreview.state == 'approved'for pull_request_review events)skip_testsis skipped (condition requireshead.ref == 'update-sponsors')go_test_resultsruns (becausealways()andbase.ref == 'master')- Line 133 evaluates
test_result == "skipped"as true and exits 0 ✅
The check passes without running any tests, potentially masking test failures and violating branch protection intent.
Root cause: The go_test_results condition (lines 118-124) is less restrictive than test_go (lines 44-56). The aggregation job runs for any pull_request_review event on master, but test_go only runs if the review is approved.
Fix options:
Tighten go_test_results condition (recommended)
Match the event-type restrictions from test_go:
if: >
always() &&
github.repository == 'wailsapp/wails' &&
(
github.event_name == 'workflow_dispatch' ||
- github.event.pull_request.base.ref == 'master'
+ (
+ github.event.pull_request.base.ref == 'master' &&
+ (
+ github.event_name == 'pull_request' ||
+ (github.event_name == 'pull_request_review' && github.event.review.state == 'approved')
+ )
+ )
)Add explicit validation when both jobs skip
- name: Check matrix result
run: |
test_result="${{ needs.test_go.result }}"
skip_result="${{ needs.skip_tests.result }}"
+
+ if [[ "$test_result" == "skipped" && "$skip_result" == "skipped" ]]; then
+ echo "ERROR: Both test_go and skip_tests were skipped - no tests ran"
+ exit 1
+ fi
if [[ "$test_result" == "success" || "$test_result" == "skipped" ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-master.yml around lines 128 - 145, The aggregation job
go_test_results currently treats a skipped test_go as success; update the
workflow so go_test_results only runs or exits 0 when tests actually ran: either
(A) tighten the go_test_results job condition to match test_go (same event
filters/conditions that cause test_go to run), or (B) add an explicit runtime
check inside go_test_results that treats the combination test_result=="skipped"
&& skip_result=="skipped" as failure (exit non‑zero) instead of success;
reference the job names test_go, skip_tests and the variables
test_result/skip_result when making this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
permissions: contents: readat workflow level to satisfy CodeQLgo_test_resultsfan-in job that posts a singleRun Go Testsstatus check, matching the branch protection required check nameif: always()andneeds: [test_go, skip_tests]so it always reports, covering both normal PRs and sponsor-update PRsBackground
The
test_gomatrix job reports per-platform checks likeRun Go Tests (ubuntu-22.04, 1.23). No aggregateRun Go Testscheck is ever posted, so the branch protection requirement (which expects exactly that name) is permanently unsatisfied on every PR.Changes
.github/workflows/pr-master.ymlpermissions: contents: read; addgo_test_resultsfan-in jobCloses #5316
Summary by CodeRabbit