test: fix test execution and improve assertions#2631
Merged
theakshaypant merged 2 commits intotektoncd:mainfrom Apr 2, 2026
Merged
test: fix test execution and improve assertions#2631theakshaypant merged 2 commits intotektoncd:mainfrom
theakshaypant merged 2 commits intotektoncd:mainfrom
Conversation
af397b9 to
cd191c9
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the Makefile to use gotestsum for unit tests and refactors the logs package to allow mocking of syscall.Exec by introducing a package-level variable. It also improves error handling in showlogswithtkn by returning errors instead of calling os.Exit. Review feedback highlights that the package-level variable approach is prone to race conditions and that syscall.Exec lacks Windows support, which could lead to compilation failures on that platform.
Updated the unit test command to use `gotestsum` when available to provide more readable test output, while maintaining compatibility with the standard Go test runner. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
The logs tests had a critical issue preventing all test cases from running. The production code used os.Exit(127) when syscall.Exec failed, which terminated the entire test process after the first subtest completed. This made gotestsum report failures and prevented comprehensive test coverage. Here is the error you can see in gotestsum previously: ```console PASS pkg/cmd/tknpac/logs.TestLogs/good/show_logs (0.01s) === RUN TestLogs FAIL pkg/cmd/tknpac/logs.TestLogs (-1.00s) === Failed === FAIL: pkg/cmd/tknpac/logs TestLogs (unknown) ok github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/logs 0.009s DONE 2 tests, 1 failure in 0.011s ``` Changes made: - Replace os.Exit with proper error return in showlogswithtkn() - Add execFunc variable to make syscall.Exec mockable in tests - Mock execFunc in tests to prevent process replacement - Upgrade to gotest.tools/v3/assert for stronger assertions - Fix duplicate test name causing gotestsum confusion - Remove unused "shift" field and invalid test case - Simplify test setup by removing unnecessary exec.LookPath gotestsum, and all linting checks pass. Assisted-by: Claude Sonnet 4.5 (via Claude Code)
cd191c9 to
31766c3
Compare
theakshaypant
approved these changes
Apr 2, 2026
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
2 participants
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
Fixed test execution issues in the logs package that prevented comprehensive test coverage:
os.Exit()with proper error handling inshowlogswithtkn()syscall.Execmockable viaexecFuncto prevent process replacement during testsgotest.tools/v3/assertfor stronger validationgotestsumintegration to Makefile for improved test output reportingerror as detected was:
Changes Made
Makefile
gotestsumif available on the systemgo testif not installedpkg/cmd/tknpac/logs/logs.go
os.Exit()onsyscall.Execfailuresyscall.Execinto mockableexecFuncvariable for testingpkg/cmd/tknpac/logs/logs_test.go
gotest.tools/v3/assertexecFuncto prevent actual process replacementexec.LookPathcallsTesting
All tests pass with proper assertions validated.
🧪 Testing Strategy
🤖 AI Assistance
✅ Submitter Checklist
test:) matches the type of changemake testandmake lintlocally - all checks pass