fix: support running unit tests without make test - #117
Open
shreyabiradar07 wants to merge 6 commits into
Open
Conversation
Signed-off-by: Shreya Biradar <shbirada@ibm.com> Assisted-by: Bob
Signed-off-by: Shreya Biradar <shbirada@ibm.com> Assisted-by: Bob
Reviewer's GuideUpdates envtest binary resolution to support running Go unit tests directly without Sequence diagram for envtest asset path resolution in suite_test.gosequenceDiagram
participant TestSuite
participant Env as os
participant Envtest as envtest
TestSuite->>Env: Getenv KUBEBUILDER_ASSETS
alt KUBEBUILDER_ASSETS is set
Env-->>TestSuite: assetsPath
else KUBEBUILDER_ASSETS is empty
TestSuite->>Env: Getwd
Env-->>TestSuite: projectRoot
TestSuite-->>TestSuite: build bin/k8s path
alt bin/k8s exists
TestSuite-->>TestSuite: use local bin/k8s assets
else bin/k8s missing
TestSuite->>Envtest: SetupEnvtestDefaultBinaryAssetsDirectory
Envtest-->>TestSuite: defaultAssetsPath
end
end
Flow diagram for updated CI unit-test and E2E jobsflowchart LR
gh[GitHub PR event]
ut[Job unit-test
go test ./internal/...]
bt[Job build-and-test
Kind cluster + E2E]
gh --> ut
ut --> bt
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
suite_test.go,BinaryAssetsDirectoryis always set even if no suitable directory is found; consider only assigning this field whenbinaryAssetsDiris non-empty so that controller-runtime can still fall back to its built-in default behavior. - The
Run unit testsstep inpr-check.yamluses nested double quotes inside theKUBEBUILDER_ASSETS="$(...)"assignment (specifically around--bin-dir "$(pwd)/bin"), which will break the shell; refactor the command to avoid conflicting quotes, e.g., by using single quotes for the outer string or separating the assignment into multiple lines.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `suite_test.go`, `BinaryAssetsDirectory` is always set even if no suitable directory is found; consider only assigning this field when `binaryAssetsDir` is non-empty so that controller-runtime can still fall back to its built-in default behavior.
- The `Run unit tests` step in `pr-check.yaml` uses nested double quotes inside the `KUBEBUILDER_ASSETS="$(...)"` assignment (specifically around `--bin-dir "$(pwd)/bin"`), which will break the shell; refactor the command to avoid conflicting quotes, e.g., by using single quotes for the outer string or separating the assignment into multiple lines.
## Individual Comments
### Comment 1
<location path=".github/workflows/pr-check.yaml" line_range="49" />
<code_context>
+
+ - name: Run unit tests
+ run: |
+ KUBEBUILDER_ASSETS="$(./bin/setup-envtest-release-0.19 use ${{ env.ENVTEST_K8S_VERSION }} --bin-dir "$(pwd)/bin" -p path)" \
+ go test ./internal/... -v -coverprofile=cover.out 2>&1 | tee /tmp/unit-test-output.log
+ exit ${PIPESTATUS[0]}
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix nested quoting in KUBEBUILDER_ASSETS assignment to avoid shell syntax errors.
As written, the nested double quotes around `"$(pwd)/bin"` terminate the outer double-quoted string, so bash will treat the `KUBEBUILDER_ASSETS=...` assignment as a syntax error.
You can fix this by either escaping the inner quotes:
```yaml
run: |
KUBEBUILDER_ASSETS="$(./bin/setup-envtest-release-0.19 use ${{ env.ENVTEST_K8S_VERSION }} --bin-dir \"$(pwd)/bin\" -p path)" \
go test ./internal/... -v -coverprofile=cover.out 2>&1 | tee /tmp/unit-test-output.log
exit ${PIPESTATUS[0]}
```
or by avoiding nested quotes via an intermediate variable:
```yaml
run: |
BIN_DIR="$(pwd)/bin"
KUBEBUILDER_ASSETS="$(./bin/setup-envtest-release-0.19 use ${{ env.ENVTEST_K8S_VERSION }} --bin-dir "$BIN_DIR" -p path)" \
go test ./internal/... -v -coverprofile=cover.out 2>&1 | tee /tmp/unit-test-output.log
exit ${PIPESTATUS[0]}
```
</issue_to_address>
### Comment 2
<location path=".github/workflows/pr-check.yaml" line_range="42-44" />
<code_context>
+ - name: Install setup-envtest
+ run: make envtest
+
+ - name: Download envtest binaries
+ run: |
+ ./bin/setup-envtest-release-0.19 use ${{ env.ENVTEST_K8S_VERSION }} \
+ --bin-dir "$(pwd)/bin"
+
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid running `setup-envtest` twice to reduce redundant work in CI.
`setup-envtest` is currently run once in "Download envtest binaries" and again when setting `KUBEBUILDER_ASSETS`, causing duplicate downloads/path resolution each run and increasing CI time and potential flakiness.
Consider either:
- Using `-p path` in the first call, persisting the resolved path (file/env var) and reusing it in the test step, or
- Removing the first step and relying on the `KUBEBUILDER_ASSETS` assignment (with `use ... -p path`) to both download and resolve the path.
This avoids redundant external calls and keeps the workflow simpler.
Suggested implementation:
```
- name: Install dependencies
run: go mod download && go mod verify
env:
```
To fully implement your comment, you should:
1. Update the step where `KUBEBUILDER_ASSETS` is set (likely the test step) to call `setup-envtest-release-0.19 use ${{ env.ENVTEST_K8S_VERSION }} -p path` and export `KUBEBUILDER_ASSETS` using that resolved path (or a persisted file).
2. Ensure that this single `use ... -p path` call both downloads the envtest binaries and sets the path used by the tests, so no separate `make envtest` step is required elsewhere.
</issue_to_address>
### Comment 3
<location path="internal/controller/suite_test.go" line_range="76-84" />
<code_context>
By("bootstrapping test environment")
+
+ // Lookup order: KUBEBUILDER_ASSETS env var → project-local bin/k8s/ → setup-envtest system cache.
+ binaryAssetsDir := os.Getenv("KUBEBUILDER_ASSETS")
+ if binaryAssetsDir == "" {
+ pkgDir, err := os.Getwd()
+ Expect(err).NotTo(HaveOccurred(), "could not determine working directory")
+ localDir := filepath.Join(pkgDir, "..", "..", "bin", "k8s",
+ fmt.Sprintf("%s-%s-%s", envtestK8sVersion, runtime.GOOS, runtime.GOARCH))
+ if _, err := os.Stat(localDir); err == nil {
+ binaryAssetsDir = localDir
+ }
+ }
+ if binaryAssetsDir == "" {
+ systemDir, err := envtest.SetupEnvtestDefaultBinaryAssetsDirectory()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider failing fast (or asserting) when no BinaryAssetsDirectory is resolved to avoid flaky or environment-dependent test behavior.
If none of KUBEBUILDER_ASSETS, the project-local bin/k8s path, or the envtest system cache resolve to a directory, `binaryAssetsDir` stays empty and envtest falls back to controller-runtime defaults. That can silently use an unexpected Kubernetes version or fail later with a vague error. Consider adding a final `Expect(binaryAssetsDir).NotTo(BeEmpty(), ...)` with a clear message (e.g. to run `make envtest` or set `KUBEBUILDER_ASSETS`). Also, if `envtest.SetupEnvtestDefaultBinaryAssetsDirectory()` returns an error, surfacing it instead of ignoring it would make missing binaries easier to diagnose.
```suggestion
if binaryAssetsDir == "" {
systemDir, err := envtest.SetupEnvtestDefaultBinaryAssetsDirectory()
Expect(err).NotTo(HaveOccurred(),
"failed to resolve envtest binary assets directory from system cache; "+
"ensure envtest binaries for Kubernetes %s are installed (e.g. via `make envtest`) or set KUBEBUILDER_ASSETS",
envtestK8sVersion)
binaryAssetsDir = filepath.Join(systemDir,
fmt.Sprintf("%s-%s-%s", envtestK8sVersion, runtime.GOOS, runtime.GOARCH))
}
Expect(binaryAssetsDir).NotTo(BeEmpty(),
"failed to resolve envtest binary assets directory; "+
"set KUBEBUILDER_ASSETS or run `make envtest` to download envtest binaries for Kubernetes %s",
envtestK8sVersion)
testEnv = &envtest.Environment{
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com> Assisted-by: Bob
Signed-off-by: Shreya Biradar <shbirada@ibm.com> Assisted-by: Bob
…r.go Signed-off-by: Shreya Biradar <shbirada@ibm.com> Assisted-by: Bob
Signed-off-by: Shreya Biradar <shbirada@ibm.com> Assisted-by: Bob
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
Unit tests (
go test ./internal/controller/...) failed when run directly, even after the envtest binaries had been downloaded. Onlymake testworked.Changes
internal/controller/suite_test.goKUBEBUILDER_ASSETSfirst (set automatically bymake test)bin/k8s/path viaos.Getwd()envtest.SetupEnvtestDefaultBinaryAssetsDirectory()(system cache)"1.31.0"intoconst envtestK8sVersion— single source of truth,cross-referenced to
ENVTEST_K8S_VERSIONin the MakefileMakefileENVTEST_K8S_VERSIONpointing atsuite_test.goto preventsilent version drift when upgrading Kubernetes
.github/workflows/pr-check.yamlunit-testjob (~1 min, no cluster, no Docker)build-and-test(E2E, ~15 min) now depends onunit-testvianeeds:so cluster time is not spent on PRs with broken unit tests
test/Operator_tests.mdgo testdirectlymake envtestinstalls vs what downloads the assetsKUBEBUILDER_ASSETSand1.31.0meanHow to verify
Summary by Sourcery
Ensure envtest-based Go unit tests run consistently both via
make testand directgo test, and gate E2E CI runs on fast unit tests.Bug Fixes:
go testdirectly by improving envtest binary discovery.Enhancements:
KUBEBUILDER_ASSETS, then local project cache, then the system cache.CI:
Documentation:
go test, and explainingKUBEBUILDER_ASSETSand the pinned Kubernetes version.