Skip to content

Commit f2b8933

Browse files
chmoueltheakshaypant
authored andcommitted
test: fix test execution and improve assertions
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)
1 parent ef35fcc commit f2b8933

File tree

4 files changed

+46
-30
lines changed

4 files changed

+46
-30
lines changed

pkg/cmd/tknpac/logs/exec_unix.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
//go:build !windows
2+
3+
package logs
4+
5+
import "syscall"
6+
7+
// defaultExecFunc replaces the current process with the given command (Unix only).
8+
var defaultExecFunc = syscall.Exec //nolint:gosec
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//go:build windows
2+
3+
package logs
4+
5+
import (
6+
"fmt"
7+
"os"
8+
osexec "os/exec"
9+
)
10+
11+
// defaultExecFunc runs the command as a subprocess on Windows,
12+
// since syscall.Exec (process replacement) is not available.
13+
var defaultExecFunc = func(argv0 string, argv, envv []string) error {
14+
cmd := osexec.Command(argv0, argv[1:]...)
15+
cmd.Env = envv
16+
cmd.Stdin = os.Stdin
17+
cmd.Stdout = os.Stdout
18+
cmd.Stderr = os.Stderr
19+
if err := cmd.Run(); err != nil {
20+
return fmt.Errorf("failed to run command: %w", err)
21+
}
22+
return nil
23+
}

pkg/cmd/tknpac/logs/logs.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"os/exec"
88
"path/filepath"
99
"strings"
10-
"syscall"
1110

1211
"github.com/AlecAivazis/survey/v2"
1312
"github.com/jonboulle/clockwork"
@@ -54,6 +53,7 @@ type logOption struct {
5453
limit int
5554
webBrowser bool
5655
useLastPR bool
56+
execFunc func(string, []string, []string) error
5757
}
5858

5959
func Command(run *params.Run, ioStreams *cli.IOStreams) *cobra.Command {
@@ -124,6 +124,7 @@ func Command(run *params.Run, ioStreams *cli.IOStreams) *cobra.Command {
124124
webBrowser: webBrowser,
125125
tknPath: tknPath,
126126
useLastPR: useLastPR,
127+
execFunc: defaultExecFunc,
127128
}
128129
return log(ctx, lopts)
129130
},
@@ -240,7 +241,7 @@ func log(ctx context.Context, lo *logOption) error {
240241
if lo.webBrowser {
241242
return showLogsWithWebConsole(ctx, lo, replyName)
242243
}
243-
return showlogswithtkn(lo.tknPath, replyName, lo.cs.Info.Kube.Namespace)
244+
return showlogswithtkn(lo.execFunc, lo.tknPath, replyName, lo.cs.Info.Kube.Namespace)
244245
}
245246

246247
func showLogsWithWebConsole(ctx context.Context, lo *logOption, pr string) error {
@@ -257,11 +258,9 @@ func showLogsWithWebConsole(ctx context.Context, lo *logOption, pr string) error
257258
return browser.OpenWebBrowser(ctx, lo.cs.Clients.ConsoleUI().DetailURL(prObj))
258259
}
259260

260-
func showlogswithtkn(tknPath, pr, ns string) error {
261-
//nolint: gosec
262-
if err := syscall.Exec(tknPath, []string{tknPath, "pr", "logs", "-f", "-n", ns, pr}, os.Environ()); err != nil {
263-
_, _ = fmt.Fprintf(os.Stderr, "Command finished with error: %v", err)
264-
os.Exit(127)
261+
func showlogswithtkn(execFn func(string, []string, []string) error, tknPath, pr, ns string) error {
262+
if err := execFn(tknPath, []string{tknPath, "pr", "logs", "-f", "-n", ns, pr}, os.Environ()); err != nil {
263+
return fmt.Errorf("failed to exec tkn: %w", err)
265264
}
266265
return nil
267266
}

pkg/cmd/tknpac/logs/logs_test.go

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package logs
22

33
import (
4-
"os/exec"
54
"testing"
65

76
"github.com/jonboulle/clockwork"
@@ -33,7 +32,6 @@ func TestLogs(t *testing.T) {
3332
wantErr bool
3433
repoName string
3534
currentNamespace string
36-
shift int
3735
pruns []*tektonv1.PipelineRun
3836
useLastPR bool
3937
}{
@@ -42,15 +40,14 @@ func TestLogs(t *testing.T) {
4240
wantErr: false,
4341
repoName: "test",
4442
currentNamespace: ns,
45-
shift: 0,
4643
pruns: []*tektonv1.PipelineRun{
4744
tektontest.MakePRCompletion(cw, "test-pipeline", ns, completed, nil, map[string]string{
4845
keys.Repository: "test",
4946
}, 30),
5047
},
5148
},
5249
{
53-
name: "good/show logs",
50+
name: "good/show logs with useLastPR",
5451
wantErr: false,
5552
repoName: "test",
5653
currentNamespace: ns,
@@ -64,18 +61,6 @@ func TestLogs(t *testing.T) {
6461
}, 30),
6562
},
6663
},
67-
{
68-
name: "bad/shift",
69-
wantErr: true,
70-
repoName: "test",
71-
currentNamespace: ns,
72-
shift: 2,
73-
pruns: []*tektonv1.PipelineRun{
74-
tektontest.MakePRCompletion(cw, "test-pipeline", ns, completed, nil, map[string]string{
75-
keys.Repository: "test",
76-
}, 30),
77-
},
78-
},
7964
{
8065
name: "bad/no prs",
8166
wantErr: true,
@@ -119,8 +104,6 @@ func TestLogs(t *testing.T) {
119104
}
120105
cs.Clients.SetConsoleUI(consoleui.FallBackConsole{})
121106

122-
tknPath, err := exec.LookPath("true")
123-
assert.NilError(t, err)
124107
io, _ := tcli.NewIOStream()
125108
lopts := &logOption{
126109
cs: cs,
@@ -130,16 +113,19 @@ func TestLogs(t *testing.T) {
130113
},
131114
repoName: tt.repoName,
132115
limit: 1,
133-
tknPath: tknPath,
116+
tknPath: "/fake/tkn",
134117
ioStreams: io,
135118
useLastPR: tt.useLastPR,
119+
execFunc: func(_ string, _, _ []string) error {
120+
return nil
121+
},
136122
}
137123

138-
err = log(ctx, lopts)
124+
err := log(ctx, lopts)
139125
if tt.wantErr {
140-
if err == nil {
141-
t.Errorf("log() wantError is true but no error has been set")
142-
}
126+
assert.Assert(t, err != nil, "expected an error but got none")
127+
} else {
128+
assert.NilError(t, err)
143129
}
144130
})
145131
}

0 commit comments

Comments
 (0)