Add CI Test Pipeline With Test Reports#315
Add CI Test Pipeline With Test Reports#315iqbalcodes6602 wants to merge 4 commits intocontainer-registry:mainfrom
Conversation
Signed-off-by: Anas Iqbal <mohd.abd.6602@gmail.com>
Signed-off-by: Anas Iqbal <mohd.abd.6602@gmail.com>
📝 WalkthroughWalkthroughThis pull request adds a test pipeline for the HarborSatellite module by introducing three new Dagger-based methods (TestReport, TestCoverage, TestCoverageReport) in the Dagger module and a corresponding GitHub Actions workflow job that orchestrates unit test execution, reporting, and coverage analysis. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Codacy's Analysis Summary3 new issues (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".github/workflows/test.yaml">
<violation number="1" location=".github/workflows/test.yaml:59">
P2: The test summary step will be skipped when unit tests fail because steps default to `if: success()`. Add `if: always()` (or `success() || failure()`) so the report runs even on failures, otherwise failed runs won’t generate annotations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| verb: call | ||
| args: test-report --source=. export --path=TestReport.json | ||
|
|
||
| - name: Summarize Tests |
There was a problem hiding this comment.
P2: The test summary step will be skipped when unit tests fail because steps default to if: success(). Add if: always() (or success() || failure()) so the report runs even on failures, otherwise failed runs won’t generate annotations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/test.yaml, line 59:
<comment>The test summary step will be skipped when unit tests fail because steps default to `if: success()`. Add `if: always()` (or `success() || failure()`) so the report runs even on failures, otherwise failed runs won’t generate annotations.</comment>
<file context>
@@ -40,6 +40,39 @@ jobs:
+ verb: call
+ args: test-report --source=. export --path=TestReport.json
+
+ - name: Summarize Tests
+ uses: robherley/go-test-action@v0.6.0
+ with:
</file context>
| - name: Summarize Tests | |
| - name: Summarize Tests | |
| if: ${{ success() || failure() }} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yaml (1)
3-8:⚠️ Potential issue | 🟡 MinorWorkflow triggers only
pull_request— coverage conditions are redundant andpushtrigger is missing per requirements.The workflow triggers only on
pull_request(lines 3-8), so theif: github.event_name == 'pull_request'guards on lines 65 and 73 are always true and serve no purpose currently. The PR objectives (issue#297) specify running tests on bothpull_requestandpushevents. Ifpushis intentionally deferred, the conditions are fine as future-proofing; otherwise, add thepushtrigger:Proposed fix to add push trigger
on: pull_request: paths-ignore: - "*.md" - "assets/**" + push: + paths-ignore: + - "*.md" + - "assets/**"Also applies to: 64-74
🤖 Fix all issues with AI agents
In @.dagger/main.go:
- Around line 212-213: Replace the hardcoded image string "golang:1.24.1-alpine"
used when constructing the dag.Container() (seen in the container variable
instantiation across the three new methods) with the existing DEFAULT_GO
constant so the container image uses DEFAULT_GO (which resolves to
"golang:1.24.11"); update all three occurrences to reference DEFAULT_GO instead
of the literal to ensure tests and builds use the same Go version.
- Around line 204-282: TestReport/TestCoverage/TestCoverageReport currently run
tests only in the root module; update each function (TestReport, TestCoverage,
TestCoverageReport) to run test/coverage commands twice: once from /src
(satellite) and once from /src/ground-control (ground-control), producing
per-module outputs (e.g., satellite-report.json, gc-report.json and
satellite-coverage.out, gc-coverage.out), then merge them into the final
artifact (for JSON test reports cat the two files into TestReport.json; for
coverage merge coverage profiles using a tool or shell trick that removes
duplicate headers into coverage.out and generate the markdown report from the
merged profile); ensure the container stages call WithWorkdir("/src") then
WithExec for the root run and another WithWorkdir("/src/ground-control") then
WithExec for the ground-control run, and add any needed tooling (e.g.,
gocovmerge or awk) in the container before merging.
- Around line 240-250: The TestCoverage container mounts /go/pkg/mod and
/go/build-cache but doesn't set the corresponding Go env vars, so add
environment variables to the container configuration (the same place where you
call WithMountedCache/WithMountedDirectory on the container variable) to set
GOMODCACHE=/go/pkg/mod and GOCACHE=/go/build-cache (matching the TestReport
behavior) so Go actually uses those cache locations.
- Around line 264-273: The TestCoverageReport container currently re-downloads
modules each run and unnecessarily installs bc; update the container setup (the
variable named container built with dag.Container()) to mount persistent Go
module and build caches (e.g., mount a cache volume for GOPATH/pkg/mod or
GOMODCACHE and for GOCACHE) into the container so go test reuses modules and
compiled caches, and remove the WithExec call that runs "apk add --no-cache bc"
since bc is unused; keep the /etc/machine-id echo and the go test -coverprofile
step as-is.
In @.github/workflows/test.yaml:
- Around line 49-55: Add an explicit step id for the Dagger Version step so its
output can be referenced: give the sagikazarmark/dagger-version-action step an
id of dagger_version (matching the expression
steps.dagger_version.outputs.version used later). Locate the step with name
"Dagger Version" and add id: dagger_version, then verify downstream steps like
the "Run Unit Tests" step still reference ${{
steps.dagger_version.outputs.version }} and that actionlint passes.
🧹 Nitpick comments (2)
.github/workflows/test.yaml (1)
50-60: Pin third-party actions to full-length commit SHAs.Codacy flagged that
sagikazarmark/dagger-version-action@v0.0.1,dagger/dagger-for-github@v7, androbherley/go-test-action@v0.6.0are referenced by mutable tags rather than pinned commit SHAs. Tag-based references are vulnerable to supply-chain attacks if a tag is force-pushed. Consider pinning to the full SHA (with a version comment for readability), e.g.:uses: robherley/go-test-action@<full-sha> # v0.6.0.dagger/main.go (1)
204-282: Consider extracting shared container setup into a helper.All three methods (
TestReport,TestCoverage,TestCoverageReport) repeat the same container initialization pattern (base image, cache mounts, env vars, machine-id, source mount, workdir). A private helper liketestContainer(source *dagger.Directory) *dagger.Containerwould reduce duplication and ensure consistency (e.g., the cache/env var discrepancies flagged above wouldn't have happened with a shared setup).
| // Unit Test Report | ||
| func (m *HarborSatellite) TestReport( | ||
| ctx context.Context, | ||
| source *dagger.Directory, | ||
| ) (*dagger.File, error) { | ||
|
|
||
| reportName := "TestReport.json" | ||
|
|
||
| container := dag.Container(). | ||
| From("golang:1.24.1-alpine"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithEnvVariable("GOCACHE", "/go/build-cache"). | ||
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | ||
| WithExec([]string{"go", "install", "gotest.tools/gotestsum@v1.12.0"}). | ||
| WithMountedDirectory("/src", source). | ||
| WithWorkdir("/src"). | ||
| WithExec([]string{ | ||
| "gotestsum", | ||
| "--jsonfile", | ||
| reportName, | ||
| "./...", | ||
| }) | ||
|
|
||
| return container.File(reportName), nil | ||
| } | ||
|
|
||
| // Coverage Raw Output | ||
| func (m *HarborSatellite) TestCoverage( | ||
| ctx context.Context, | ||
| source *dagger.Directory, | ||
| ) (*dagger.File, error) { | ||
|
|
||
| coverage := "coverage.out" | ||
|
|
||
| container := dag.Container(). | ||
| From("golang:1.24.1-alpine"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithMountedDirectory("/src", source). | ||
| WithWorkdir("/src"). | ||
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | ||
| WithExec([]string{ | ||
| "go", "test", "./...", | ||
| "-coverprofile=" + coverage, | ||
| }) | ||
|
|
||
| return container.File(coverage), nil | ||
| } | ||
|
|
||
| // Coverage Markdown Report | ||
| func (m *HarborSatellite) TestCoverageReport( | ||
| ctx context.Context, | ||
| source *dagger.Directory, | ||
| ) (*dagger.File, error) { | ||
|
|
||
| report := "coverage-report.md" | ||
| coverage := "coverage.out" | ||
|
|
||
| container := dag.Container(). | ||
| From("golang:1.24.1-alpine"). | ||
| WithMountedDirectory("/src", source). | ||
| WithWorkdir("/src"). | ||
| WithExec([]string{"apk", "add", "--no-cache", "bc"}). | ||
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | ||
| WithExec([]string{ | ||
| "go", "test", "./...", | ||
| "-coverprofile=" + coverage, | ||
| }) | ||
|
|
||
| return container.WithExec([]string{"sh", "-c", ` | ||
| echo "<h2> Test Coverage</h2>" > ` + report + ` | ||
| total=$(go tool cover -func=` + coverage + ` | grep total: | grep -Eo '[0-9]+\.[0-9]+') | ||
| echo "<b>Total Coverage:</b> $total%" >> ` + report + ` | ||
| echo "<details><summary>Details</summary><pre>" >> ` + report + ` | ||
| go tool cover -func=` + coverage + ` >> ` + report + ` | ||
| echo "</pre></details>" >> ` + report + ` | ||
| `}).File(report), nil |
There was a problem hiding this comment.
Tests only cover the root (satellite) module — ground-control tests are not executed.
The project uses a two-module structure (go.mod at root for satellite, ground-control/go.mod for ground control). Running go test ./... or gotestsum ./... from /src only exercises the root module. The PR objective (issue #297) requires testing both modules.
You'd need to either run a second pass from the ground-control directory or use a script that iterates over both modules. For example, in TestReport:
// Run satellite tests
WithWorkdir("/src").
WithExec([]string{"gotestsum", "--jsonfile", "satellite-report.json", "./..."}).
// Run ground-control tests
WithWorkdir("/src/ground-control").
WithExec([]string{"gotestsum", "--jsonfile", "gc-report.json", "./..."}).
// Merge reports
WithExec([]string{"sh", "-c", "cat satellite-report.json gc-report.json > TestReport.json"})Similar changes would be needed in TestCoverage and TestCoverageReport. Based on learnings: "Use two-module structure: root module (go.mod) for Satellite component and ground-control/go.mod for Ground Control component."
🤖 Prompt for AI Agents
In @.dagger/main.go around lines 204 - 282,
TestReport/TestCoverage/TestCoverageReport currently run tests only in the root
module; update each function (TestReport, TestCoverage, TestCoverageReport) to
run test/coverage commands twice: once from /src (satellite) and once from
/src/ground-control (ground-control), producing per-module outputs (e.g.,
satellite-report.json, gc-report.json and satellite-coverage.out,
gc-coverage.out), then merge them into the final artifact (for JSON test reports
cat the two files into TestReport.json; for coverage merge coverage profiles
using a tool or shell trick that removes duplicate headers into coverage.out and
generate the markdown report from the merged profile); ensure the container
stages call WithWorkdir("/src") then WithExec for the root run and another
WithWorkdir("/src/ground-control") then WithExec for the ground-control run, and
add any needed tooling (e.g., gocovmerge or awk) in the container before
merging.
| container := dag.Container(). | ||
| From("golang:1.24.1-alpine"). |
There was a problem hiding this comment.
Hardcoded Go version 1.24.1 diverges from the DEFAULT_GO constant (golang:1.24.11).
All three new methods use "golang:1.24.1-alpine" while the rest of the file uses DEFAULT_GO (which resolves to "golang:1.24.11"). This means tests run on a different (older) Go version than builds, which can mask or introduce version-specific bugs. Use the constant for consistency:
Proposed fix (apply in all three methods)
- From("golang:1.24.1-alpine").
+ From(DEFAULT_GO + "-alpine").📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| container := dag.Container(). | |
| From("golang:1.24.1-alpine"). | |
| container := dag.Container(). | |
| From(DEFAULT_GO + "-alpine"). |
🤖 Prompt for AI Agents
In @.dagger/main.go around lines 212 - 213, Replace the hardcoded image string
"golang:1.24.1-alpine" used when constructing the dag.Container() (seen in the
container variable instantiation across the three new methods) with the existing
DEFAULT_GO constant so the container image uses DEFAULT_GO (which resolves to
"golang:1.24.11"); update all three occurrences to reference DEFAULT_GO instead
of the literal to ensure tests and builds use the same Go version.
| container := dag.Container(). | ||
| From("golang:1.24.1-alpine"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithMountedDirectory("/src", source). | ||
| WithWorkdir("/src"). | ||
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | ||
| WithExec([]string{ | ||
| "go", "test", "./...", | ||
| "-coverprofile=" + coverage, | ||
| }) |
There was a problem hiding this comment.
TestCoverage mounts cache volumes but doesn't set GOMODCACHE/GOCACHE env vars.
Unlike TestReport (which correctly sets both), this method mounts /go/pkg/mod and /go/build-cache but never tells Go to use them via environment variables. The build cache at /go/build-cache will be ignored because GOCACHE defaults to ~/.cache/go-build. Add the missing env vars for consistency and effective caching:
Proposed fix
container := dag.Container().
- From("golang:1.24.1-alpine").
+ From(DEFAULT_GO + "-alpine").
WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")).
+ WithEnvVariable("GOMODCACHE", "/go/pkg/mod").
WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")).
+ WithEnvVariable("GOCACHE", "/go/build-cache").
WithMountedDirectory("/src", source).🤖 Prompt for AI Agents
In @.dagger/main.go around lines 240 - 250, The TestCoverage container mounts
/go/pkg/mod and /go/build-cache but doesn't set the corresponding Go env vars,
so add environment variables to the container configuration (the same place
where you call WithMountedCache/WithMountedDirectory on the container variable)
to set GOMODCACHE=/go/pkg/mod and GOCACHE=/go/build-cache (matching the
TestReport behavior) so Go actually uses those cache locations.
| container := dag.Container(). | ||
| From("golang:1.24.1-alpine"). | ||
| WithMountedDirectory("/src", source). | ||
| WithWorkdir("/src"). | ||
| WithExec([]string{"apk", "add", "--no-cache", "bc"}). | ||
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | ||
| WithExec([]string{ | ||
| "go", "test", "./...", | ||
| "-coverprofile=" + coverage, | ||
| }) |
There was a problem hiding this comment.
TestCoverageReport has no Go module/build caches and installs an unused bc package.
- This method doesn't mount any cache volumes, so every invocation downloads all Go modules from scratch — a notable performance hit.
apk add --no-cache bcinstallsbc, but the shell script on lines 275-282 never uses it (it only callsgo tool coverandgrep). Remove the unnecessary install.
Proposed fix
container := dag.Container().
- From("golang:1.24.1-alpine").
+ From(DEFAULT_GO + "-alpine").
+ WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")).
+ WithEnvVariable("GOMODCACHE", "/go/pkg/mod").
+ WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")).
+ WithEnvVariable("GOCACHE", "/go/build-cache").
WithMountedDirectory("/src", source).
WithWorkdir("/src").
- WithExec([]string{"apk", "add", "--no-cache", "bc"}).
WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| container := dag.Container(). | |
| From("golang:1.24.1-alpine"). | |
| WithMountedDirectory("/src", source). | |
| WithWorkdir("/src"). | |
| WithExec([]string{"apk", "add", "--no-cache", "bc"}). | |
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | |
| WithExec([]string{ | |
| "go", "test", "./...", | |
| "-coverprofile=" + coverage, | |
| }) | |
| container := dag.Container(). | |
| From(DEFAULT_GO + "-alpine"). | |
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | |
| WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). | |
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | |
| WithEnvVariable("GOCACHE", "/go/build-cache"). | |
| WithMountedDirectory("/src", source). | |
| WithWorkdir("/src"). | |
| WithExec([]string{"sh", "-c", "echo test-machine-id > /etc/machine-id"}). | |
| WithExec([]string{ | |
| "go", "test", "./...", | |
| "-coverprofile=" + coverage, | |
| }) |
🤖 Prompt for AI Agents
In @.dagger/main.go around lines 264 - 273, The TestCoverageReport container
currently re-downloads modules each run and unnecessarily installs bc; update
the container setup (the variable named container built with dag.Container()) to
mount persistent Go module and build caches (e.g., mount a cache volume for
GOPATH/pkg/mod or GOMODCACHE and for GOCACHE) into the container so go test
reuses modules and compiled caches, and remove the WithExec call that runs "apk
add --no-cache bc" since bc is unused; keep the /etc/machine-id echo and the go
test -coverprofile step as-is.
| - name: Dagger Version | ||
| uses: sagikazarmark/dagger-version-action@v0.0.1 | ||
|
|
||
| - name: Run Unit Tests | ||
| uses: dagger/dagger-for-github@v7 | ||
| with: | ||
| version: ${{ steps.dagger_version.outputs.version }} |
There was a problem hiding this comment.
Missing id: dagger_version on the Dagger Version step — workflow will fail.
The step at line 50 uses sagikazarmark/dagger-version-action@v0.0.1 but does not declare id: dagger_version. The expression ${{ steps.dagger_version.outputs.version }} on lines 55 and 68 will resolve to an empty string, causing the Dagger calls to use an unspecified version or fail entirely. This is also flagged by actionlint.
Note: A retrieved learning from a prior PR (PR #153) claims this action works without an explicit id. That contradicts standard GitHub Actions behavior — step outputs require an id to be referenced. Please verify, but the fix is straightforward.
Proposed fix
- name: Dagger Version
+ id: dagger_version
uses: sagikazarmark/dagger-version-action@v0.0.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Dagger Version | |
| uses: sagikazarmark/dagger-version-action@v0.0.1 | |
| - name: Run Unit Tests | |
| uses: dagger/dagger-for-github@v7 | |
| with: | |
| version: ${{ steps.dagger_version.outputs.version }} | |
| - name: Dagger Version | |
| id: dagger_version | |
| uses: sagikazarmark/dagger-version-action@v0.0.1 | |
| - name: Run Unit Tests | |
| uses: dagger/dagger-for-github@v7 | |
| with: | |
| version: ${{ steps.dagger_version.outputs.version }} |
🧰 Tools
🪛 actionlint (1.7.10)
[error] 55-55: property "dagger_version" is not defined in object type {}
(expression)
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 50-50: .github/workflows/test.yaml#L50
An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release.
[warning] 53-53: .github/workflows/test.yaml#L53
An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release.
🤖 Prompt for AI Agents
In @.github/workflows/test.yaml around lines 49 - 55, Add an explicit step id
for the Dagger Version step so its output can be referenced: give the
sagikazarmark/dagger-version-action step an id of dagger_version (matching the
expression steps.dagger_version.outputs.version used later). Locate the step
with name "Dagger Version" and add id: dagger_version, then verify downstream
steps like the "Run Unit Tests" step still reference ${{
steps.dagger_version.outputs.version }} and that actionlint passes.
1 participant
Summary
This PR adds a CI test pipeline that generates and displays unit test and coverage reports, following the harbor-cli implementation.
Description
Screenshot
Benefits
Summary by cubic
Adds a CI test pipeline that runs unit tests for satellite and ground-control and publishes a JSON report and coverage summary to PR checks. Improves test visibility and makes CI failures easier to debug.
Written for commit 86a21e1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores