feat: add unit tests and Codecov integration - #202
Conversation
Add unit tests for pkg/config and pkg/webterminal packages, a GitHub Actions workflow that runs tests with coverage on push to main and PRs, and uploads results to Codecov via OIDC with the unit-tests flag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
| t.Log("GetNamespace() succeeded — running in a Kubernetes pod") | ||
| return | ||
| } | ||
| t.Log("GetNamespace() returned expected error in non-cluster environment:", err) |
There was a problem hiding this comment.
both err == nil and err != nil are treated as success. Could we either remove it or make it deterministic by testing the expected behavior outside a cluster (or by injecting/mocking the namespace source)?
There was a problem hiding this comment.
Good catch, you're right this test is a no-op as written. I'll make it deterministic by asserting the expected error, since we know /var/run/secrets/kubernetes.io/serviceaccount/namespace won't exist outside a cluster.
| } | ||
|
|
||
| func TestGetSpecExecTemplateEnvUnset(t *testing.T) { | ||
| os.Unsetenv("RELATED_IMAGE_web_terminal_exec") |
There was a problem hiding this comment.
os.Unsetenv() mutates the process-wide environment and the change remains in effect for other tests. This can make tests order-dependent, and becomes especially problematic if tests are later run in parallel.
I noticed t.Setenv() is already used elsewhere in the PR, which automatically restores the environment after the test. Could we use that here as well, e.g.:
t.Setenv("RELATED_IMAGE_web_terminal_exec", "")
If the test specifically needs to verify the variable is absent rather than empty, we could instead use t.Cleanup() to restore the original environment after the test.
There was a problem hiding this comment.
Good point. I'll switch to t.Setenv("RELATED_IMAGE_web_terminal_exec", "") which auto-restores after the test. Since os.Getenv returns "" for both unset and empty-string, this is functionally equivalent. Same fix applied in tooling_test.go.
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.26.5' |
There was a problem hiding this comment.
Minor, and I only noticed because I was looking for why the cache step existed: setup-go runs before checkout. actions/setup-go@v5 has cache: true by default and resolves it from go.sum, which isn't on disk yet at that point ... so its built-in caching no-ops, and the manual actions/cache step below is doing the job it would otherwise have done.
If you swap the order you can drop the manual step entirely:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: go.mod
cache: truego-version-file also removes the hardcoded '1.26.5', which currently duplicates the go directive in go.mod and will quietly drift on the next bump.
There was a problem hiding this comment.
Great suggestion, swapped the order so setup-go can use its built-in caching, and switched to go-version-file: go.mod to avoid version drift. Removed the manual cache step.
| go test -v ./pkg/... | ||
|
|
||
| ### test-coverage: Run unit tests with coverage report | ||
| test-coverage: |
There was a problem hiding this comment.
| test-coverage: | |
| .PHONY: test test-coverage | |
| test-coverage: |
There was a problem hiding this comment.
Added .PHONY declaration. Thanks.
| patch: | ||
| default: | ||
| target: auto | ||
| threshold: 1% |
There was a problem hiding this comment.
Just checking what behavior we're expecting from the Codecov configuration.
Is target: auto with a 1% threshold intentional here? Does this configuration actually enforce a minimum coverage level, or is the goal only to track/report coverage?
There was a problem hiding this comment.
Yes, this is intentional and follows the standard configuration from the Code Coverage Onboarding Guide. target: auto sets the target to the base commit's coverage (not a fixed minimum), and threshold: 1% means the Codecov status check passes as long as coverage doesn't drop more than 1% from the base. So it's reporting + soft enforcement. It flags regressions but doesn't impose a fixed floor.
- Make TestGetNamespace deterministic by asserting expected error
- Replace os.Unsetenv() with t.Setenv("", "") for proper test cleanup
- Swap checkout/setup-go order and use go-version-file to avoid drift
- Drop manual cache step (setup-go built-in cache handles it)
- Add .PHONY declaration for test targets
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 participants
Summary
pkg/configandpkg/webterminalpackages (17 tests, 51.4% coverage)unit-tests.yml) that runs tests with coverage on push to main and PRs, uploading to Codecov via OIDC withunit-testsflagcodecov.ymlwith ignore patterns, flag configuration, and PR comment layouttestandtest-coverageMakefile targetscoverage.outto.gitignoreTest plan
make test-coverage)pkg/configcoverage: 91.7%pkg/webterminalcoverage: 47.6% (uncovered functions require a running Kubernetes cluster)unit-tests.ymlworkflow runs successfully on this PR🤖 Generated with Claude Code