fix: use go run gotestsum instead of calling gotestsum directly#5206
fix: use go run gotestsum instead of calling gotestsum directly#5206nestoracunablanco wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthrough
Changesgotestsum invocation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (16 passed)
✨ 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @nestoracunablanco. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
144-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
gotestsuminstall to thee2etarget (it callshack/test-go.shwhich requires it).
hack/test-go.shrunsgotestsumdirectly (Makefilehack/test-go.shline 11) but only thetesttarget installsgotest.tools/gotestsum@latest.make e2ecurrently callshack/test-go.shwithout installinggotestsum, so it can fail whengotestsumisn’t already present.🔧 Proposed fix
e2e: $(TMPDIR)/.boskos-credentials + go install gotest.tools/gotestsum@latest BOSKOS_CREDENTIALS_FILE="$(TMPDIR)/.boskos-credentials" PACKAGES="$(PACKAGES)" TESTFLAGS="$(TESTFLAGS) -tags $(TAGS) -timeout 70m -parallel 100" hack/test-go.sh🤖 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 `@Makefile` at line 144, The e2e Makefile target calls hack/test-go.sh which expects gotestsum to be installed but e2e doesn’t install it; update the e2e target (the rule invoking hack/test-go.sh) to install gotestsum the same way the test target does (e.g., run the go install gotest.tools/gotestsum@latest step) before calling hack/test-go.sh so hack/test-go.sh can use gotestsum reliably; reference the Makefile e2e target and hack/test-go.sh when making the change.
🤖 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.
Outside diff comments:
In `@Makefile`:
- Line 144: The e2e Makefile target calls hack/test-go.sh which expects
gotestsum to be installed but e2e doesn’t install it; update the e2e target (the
rule invoking hack/test-go.sh) to install gotestsum the same way the test target
does (e.g., run the go install gotest.tools/gotestsum@latest step) before
calling hack/test-go.sh so hack/test-go.sh can use gotestsum reliably; reference
the Makefile e2e target and hack/test-go.sh when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9bb6d2ab-83fe-4c7e-9b82-d8ea566e8521
📒 Files selected for processing (1)
Makefile
nestoracunablanco
force-pushed
the
fix/makeUnitTests
branch
from
2839c56 to
a3d73d1
Compare
|
@hector-vido @smg247 , what do you think about this change? |
|
/pj-rehearse ack |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bear-redhat, nestoracunablanco The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test ci/prow/breaking-changes |
|
/test breaking-changes |
|
@nestoracunablanco: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@nestoracunablanco: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@nestoracunablanco: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Revision a3d73d1 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
This ensures that the tool will be available in the environments Signed-off-by: nestoracunablanco <nestor.acuna@ibm.com>
nestoracunablanco
force-pushed
the
fix/makeUnitTests
branch
from
a3d73d1 to
49b3356
Compare
|
New changes are detected. LGTM label has been removed. |
nestoracunablanco
changed the title
2 participants
This ensures that the tool will be available in the environments
Summary
This PR modifies the test execution script (
hack/test-go.sh) to dynamically fetch and run thegotestsumtest runner viago run gotest.tools/gotestsum@latestinstead of relying on a pre-installed binary.What Changed
The Go test runner invocation was updated from calling a pre-installed
gotestsumbinary to dynamically downloading and executing it using Go'sgo runmechanism. This change preserves all existing functionality including JUnit report generation, package selection, race condition detection, and custom test flags.Practical Impact for CI Operators
This change eliminates a build-time dependency requirement for test execution environments. Previously, CI jobs needed to ensure
gotestsumwas pre-installed in their container images or execution environments. Now, the tool is automatically fetched at runtime when the test script runs, similar to how developers might usego runlocally for tools. This makes the testing infrastructure more self-contained and reduces environmental setup complexity.The modification specifically impacts how the
ci-toolsproject runs its own unit and integration tests, making the test workflow more reliable and less dependent on pre-provisioned tooling in CI containers.