fix: gotestsum instllation before running tests#5206
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nestoracunablanco The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesTest Tooling Setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 17✅ Passed checks (17 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 |
|
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. |
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
Install gotestsum before running tests to ensure the test tooling is available. Signed-off-by: nestoracunablanco <nestor.acuna@ibm.com>
2839c56 to
a3d73d1
Compare
Install gotestsum before running tests to ensure the test tooling is available.
Summary
This PR ensures the test tooling
gotestsumis installed before running tests by adding an explicit installation step in the Makefile. Both thetest(unit) ande2etargets now rungo install gotest.tools/gotestsum@latestbefore invoking the test runner script (hack/test-go.sh).Impact
gotestsumif it's missing, preventing "command not found" failures and smoothing local test execution.gotestsumis present before executing end-to-end test suites, ensuring consistent test output formatting and jUnit report generation for artifact collection.Changes
go install gotest.tools/gotestsum@latestas a pre-step in thetestande2etargets. The calls tohack/test-go.shremain unchanged.