Skip to content

Conversation

@billf
Copy link

@billf billf commented Dec 11, 2025

No description provided.

Copilot AI review requested due to automatic review settings December 11, 2025 18:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modifies the GitHub Actions test workflow to generate code coverage for multiple revisions (base and head) of a pull request. The changes introduce a matrix strategy that tests different Git revisions to enable base/head coverage comparison.

  • Introduces a multi-dimensional matrix with Go versions and Git revisions
  • Updates artifact naming to include revision information
  • Adjusts checkout depth and reference parameters

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- uses: fgrosse/go-coverage-report@v1.2.0
with:
coverage-artifact-name: "code-coverage-stable"
coverage-artifact-name: "code-coverage-${{ github.head_ref }}"
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coverage-artifact-name references github.head_ref which will be empty for push events (this workflow runs on push, pull_request, and workflow_dispatch). This will cause the code_coverage job to fail when trying to download the artifact on non-PR events. Since the code_coverage job already has a condition to only run on pull requests (line 55), this might work, but it's inconsistent with how artifacts are named in line 50 which uses matrix.rev.

Copilot uses AI. Check for mistakes.
run: |
sed -e 's#github.com/netactuate/gona#github.com/${{ github.repository }}#g' cover.out > tmpfile && mv tmpfile cover.out
- name: Archive code coverage results
if: matrix.go == 'stable'
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition ensures only stable Go versions upload coverage artifacts, but with the new matrix configuration, this means 2 artifacts will be uploaded for the 2 stable matrix entries (one for head.sha and one for head_ref), while the oldstable entry won't upload any artifact. However, the artifact naming at line 50 uses matrix.rev which will create artifacts with different names for each rev value. The code_coverage job at line 65 only references one artifact name. This will result in only one of the two stable coverage artifacts being used for the coverage report, while the other is ignored.

Suggested change
if: matrix.go == 'stable'
if: matrix.go == 'stable' && matrix.rev == github.head_ref

Copilot uses AI. Check for mistakes.
Comment on lines 15 to 23
go: ["stable"]
rev:
[
"${{ github.event.pull_request.head.sha }}",
"${{ github.head_ref }}",
]
include:
- go: "oldstable"
rev: "${{ github.head_ref }}"
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The matrix rev values will be empty when this workflow runs on push events (non-PR contexts). The github.event.pull_request.head.sha and github.head_ref variables are only populated for pull_request events, but this workflow also runs on push and workflow_dispatch events. This will cause the matrix to have empty string values for rev, leading to checkout failures and invalid artifact names. Consider adding a condition to only use this matrix configuration for pull requests, or provide fallback values for non-PR contexts.

Suggested change
go: ["stable"]
rev:
[
"${{ github.event.pull_request.head.sha }}",
"${{ github.head_ref }}",
]
include:
- go: "oldstable"
rev: "${{ github.head_ref }}"
go: ["stable", "oldstable"]
rev:
- ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}

Copilot uses AI. Check for mistakes.
uses: actions/upload-artifact@v5
with:
name: code-coverage-${{ matrix.go }}
name: "code-coverage-${{ matrix.rev }}"
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The artifact name uses matrix.rev which can contain either a SHA (from github.event.pull_request.head.sha) or a branch name (from github.head_ref). GitHub artifact names have restrictions and cannot contain certain characters that might appear in SHAs or branch names (such as slashes in branch names like feature/foo). Additionally, when matrix.rev contains a SHA, the artifact name will be very long and unclear. Consider using a more stable naming scheme or sanitizing the values.

Copilot uses AI. Check for mistakes.
@billf billf force-pushed the billf/chore-gha-generate-base-rev-coverage-for-stacks branch from d8bb0b3 to 17a3f80 Compare December 11, 2025 18:44
Copilot AI review requested due to automatic review settings December 11, 2025 18:51
@billf billf force-pushed the billf/chore-gha-generate-base-rev-coverage-for-stacks branch from 17a3f80 to adf3b0d Compare December 11, 2025 18:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 47 to 51
if: matrix.go == 'stable'
uses: actions/upload-artifact@v5
with:
name: code-coverage-${{ matrix.go }}
name: "code-coverage"
path: cover.out
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if: matrix.go == 'stable' condition will cause only the stable Go jobs to upload artifacts, but the code_coverage job expects to receive coverage data. With the current matrix, this means:

  • The oldstable job (Job 3) will not upload its coverage
  • Only Jobs 1 and 2 (both stable) will upload

However, if the intent is to generate coverage for both base and head revisions, both uploads will overwrite each other in the same artifact. The condition should be adjusted to properly distinguish which jobs should upload artifacts, or the artifact naming should differentiate between revisions.

Copilot uses AI. Check for mistakes.
]
include:
- go: "oldstable"
rev: "${{github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha}}"
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after the ${{ in the GitHub expression. The correct syntax should be ${{ github.event_name with a space after the opening braces for consistency with the expressions on lines 18 and 19.

Suggested change
rev: "${{github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha}}"
rev: "${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}"

Copilot uses AI. Check for mistakes.
- uses: fgrosse/go-coverage-report@v1.2.0
with:
coverage-artifact-name: "code-coverage-stable"
coverage-artifact-name: "code-coverage"
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code_coverage job downloads artifacts using the name "code-coverage", but with the current matrix setup, this artifact will contain coverage data from only one of the test runs (whichever uploaded last). The go-coverage-report action likely expects to compare coverage between base and head revisions, but the workflow doesn't properly separate or identify which coverage belongs to which revision.

Consider either:

  1. Using unique artifact names per revision (e.g., "code-coverage-head" and "code-coverage-base") and configuring the coverage report action to use both, or
  2. Restructuring the workflow to properly handle multiple coverage files if the action supports it.

Copilot uses AI. Check for mistakes.
@billf billf force-pushed the billf/chore-gha-generate-base-rev-coverage-for-stacks branch from adf3b0d to 4c55dcd Compare December 11, 2025 18:55
@billf billf mentioned this pull request Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants