From c6e6634a015cd39445359992d6cfa486507001b5 Mon Sep 17 00:00:00 2001 From: elasticspoon Date: Tue, 19 Mar 2024 22:30:15 -0400 Subject: [PATCH 1/2] feat(cli): add golden tests for errors (#11588) Creates golden files from `coder/cli/errors.go`. Adds a unit test to test against golden files. Adds a make file command to regenerate golden files. Abstracts test against golden files. fix: prevent data race refactor: make formatter public Parse error command in a less brittle way --- Makefile | 2 +- cli/clitest/golden.go | 57 +++++----- cli/errors_test.go | 100 ++++++++++++++++++ cli/root.go | 20 ++-- cli/server_test.go | 16 +-- .../coder_exp_example-error_api.golden | 3 + ...oder_exp_example-error_arg-required.golden | 2 + .../coder_exp_example-error_cmd.golden | 2 + ...coder_exp_example-error_multi-error.golden | 7 ++ ...exp_example-error_multi-multi-error.golden | 6 ++ .../coder_exp_example-error_validation.golden | 1 + 11 files changed, 168 insertions(+), 48 deletions(-) create mode 100644 cli/errors_test.go create mode 100644 cli/testdata/coder_exp_example-error_api.golden create mode 100644 cli/testdata/coder_exp_example-error_arg-required.golden create mode 100644 cli/testdata/coder_exp_example-error_cmd.golden create mode 100644 cli/testdata/coder_exp_example-error_multi-error.golden create mode 100644 cli/testdata/coder_exp_example-error_multi-multi-error.golden create mode 100644 cli/testdata/coder_exp_example-error_validation.golden diff --git a/Makefile b/Makefile index 4aabb3951fcc6..84a97323cd442 100644 --- a/Makefile +++ b/Makefile @@ -642,7 +642,7 @@ update-golden-files: \ .PHONY: update-golden-files cli/testdata/.gen-golden: $(wildcard cli/testdata/*.golden) $(wildcard cli/*.tpl) $(GO_SRC_FILES) $(wildcard cli/*_test.go) - go test ./cli -run="Test(CommandHelp|ServerYAML)" -update + go test ./cli -run="Test(CommandHelp|ServerYAML|ErrorExamples)" -update touch "$@" enterprise/cli/testdata/.gen-golden: $(wildcard enterprise/cli/testdata/*.golden) $(wildcard cli/*.tpl) $(GO_SRC_FILES) $(wildcard enterprise/cli/*_test.go) diff --git a/cli/clitest/golden.go b/cli/clitest/golden.go index eec504e70b231..635ced97d4b50 100644 --- a/cli/clitest/golden.go +++ b/cli/clitest/golden.go @@ -87,40 +87,45 @@ ExtractCommandPathsLoop: StartWithWaiter(t, inv.WithContext(ctx)).RequireSuccess() - actual := outBuf.Bytes() - if len(actual) == 0 { - t.Fatal("no output") - } - - for k, v := range replacements { - actual = bytes.ReplaceAll(actual, []byte(k), []byte(v)) - } + TestGoldenFile(t, tt.Name, outBuf.Bytes(), replacements) + }) + } +} - actual = NormalizeGoldenFile(t, actual) - goldenPath := filepath.Join("testdata", strings.Replace(tt.Name, " ", "_", -1)+".golden") - if *UpdateGoldenFiles { - t.Logf("update golden file for: %q: %s", tt.Name, goldenPath) - err := os.WriteFile(goldenPath, actual, 0o600) - require.NoError(t, err, "update golden file") - } +// TestGoldenFile will test the given bytes slice input against the +// golden file with the given file name, optionally using the given replacements. +func TestGoldenFile(t *testing.T, fileName string, actual []byte, replacements map[string]string) { + if len(actual) == 0 { + t.Fatal("no output") + } - expected, err := os.ReadFile(goldenPath) - require.NoError(t, err, "read golden file, run \"make update-golden-files\" and commit the changes") + for k, v := range replacements { + actual = bytes.ReplaceAll(actual, []byte(k), []byte(v)) + } - expected = NormalizeGoldenFile(t, expected) - require.Equal( - t, string(expected), string(actual), - "golden file mismatch: %s, run \"make update-golden-files\", verify and commit the changes", - goldenPath, - ) - }) + actual = normalizeGoldenFile(t, actual) + goldenPath := filepath.Join("testdata", strings.ReplaceAll(fileName, " ", "_")+".golden") + if *UpdateGoldenFiles { + t.Logf("update golden file for: %q: %s", fileName, goldenPath) + err := os.WriteFile(goldenPath, actual, 0o600) + require.NoError(t, err, "update golden file") } + + expected, err := os.ReadFile(goldenPath) + require.NoError(t, err, "read golden file, run \"make update-golden-files\" and commit the changes") + + expected = normalizeGoldenFile(t, expected) + require.Equal( + t, string(expected), string(actual), + "golden file mismatch: %s, run \"make update-golden-files\", verify and commit the changes", + goldenPath, + ) } -// NormalizeGoldenFile replaces any strings that are system or timing dependent +// normalizeGoldenFile replaces any strings that are system or timing dependent // with a placeholder so that the golden files can be compared with a simple // equality check. -func NormalizeGoldenFile(t *testing.T, byt []byte) []byte { +func normalizeGoldenFile(t *testing.T, byt []byte) []byte { // Replace any timestamps with a placeholder. byt = timestampRegex.ReplaceAll(byt, []byte("[timestamp]")) diff --git a/cli/errors_test.go b/cli/errors_test.go new file mode 100644 index 0000000000000..cf34fdfa219ad --- /dev/null +++ b/cli/errors_test.go @@ -0,0 +1,100 @@ +package cli_test + +import ( + "bytes" + "fmt" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/cli" + "github.com/coder/coder/v2/cli/clitest" + "github.com/coder/serpent" +) + +type commandErrorCase struct { + Name string + Cmd []string +} + +// TestErrorExamples will test the help output of the +// coder exp example-error using golden files. +func TestErrorExamples(t *testing.T) { + t.Parallel() + + coderRootCmd := getRoot(t) + + var exampleErrorRootCmd *serpent.Command + coderRootCmd.Walk(func(command *serpent.Command) { + if command.Name() == "example-error" { + // cannot abort early, but list is small + exampleErrorRootCmd = command + } + }) + require.NotNil(t, exampleErrorRootCmd, "example-error command not found") + + var cases []commandErrorCase + +ExtractCommandPathsLoop: + for _, cp := range extractCommandPaths(nil, exampleErrorRootCmd.Children) { + cmd := append([]string{"exp", "example-error"}, cp...) + name := fmt.Sprintf("coder %s", strings.Join(cmd, " ")) + for _, tt := range cases { + if tt.Name == name { + continue ExtractCommandPathsLoop + } + } + cases = append(cases, commandErrorCase{Name: name, Cmd: cmd}) + } + + for _, tt := range cases { + tt := tt + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + + var outBuf bytes.Buffer + + coderRootCmd := getRoot(t) + + inv, _ := clitest.NewWithCommand(t, coderRootCmd, tt.Cmd...) + inv.Stderr = &outBuf + inv.Stdout = &outBuf + + // This example expects to close stdin twice and joins + // the error messages to create a multi-multi error. + if tt.Name == "coder exp example-error multi-multi-error" { + inv.Stdin = os.Stdin + } + + err := inv.Run() + + errFormatter := cli.NewPrettyErrorFormatter(&outBuf, false) + errFormatter.Format(err) + + clitest.TestGoldenFile(t, tt.Name, outBuf.Bytes(), nil) + }) + } +} + +func extractCommandPaths(cmdPath []string, cmds []*serpent.Command) [][]string { + var cmdPaths [][]string + for _, c := range cmds { + cmdPath := append(cmdPath, c.Name()) + cmdPaths = append(cmdPaths, cmdPath) + cmdPaths = append(cmdPaths, extractCommandPaths(cmdPath, c.Children)...) + } + return cmdPaths +} + +// Must return a fresh instance of cmds each time. +func getRoot(t *testing.T) *serpent.Command { + t.Helper() + + var root cli.RootCmd + rootCmd, err := root.Command(root.AGPL()) + require.NoError(t, err) + + return rootCmd +} diff --git a/cli/root.go b/cli/root.go index bbc59a8d94890..83367169df154 100644 --- a/cli/root.go +++ b/cli/root.go @@ -167,9 +167,9 @@ func (r *RootCmd) RunWithSubcommands(subcommands []*serpent.Command) { //nolint:revive os.Exit(code) } - f := prettyErrorFormatter{w: os.Stderr, verbose: r.verbose} + f := PrettyErrorFormatter{w: os.Stderr, verbose: r.verbose} if err != nil { - f.format(err) + f.Format(err) } //nolint:revive os.Exit(code) @@ -909,15 +909,23 @@ func ExitError(code int, err error) error { return &exitError{code: code, err: err} } -type prettyErrorFormatter struct { +// NewPrettyErrorFormatter creates a new PrettyErrorFormatter. +func NewPrettyErrorFormatter(w io.Writer, verbose bool) *PrettyErrorFormatter { + return &PrettyErrorFormatter{ + w: w, + verbose: verbose, + } +} + +type PrettyErrorFormatter struct { w io.Writer // verbose turns on more detailed error logs, such as stack traces. verbose bool } -// format formats the error to the console. This error should be human -// readable. -func (p *prettyErrorFormatter) format(err error) { +// Format formats the error to the writer in PrettyErrorFormatter. +// This error should be human readable. +func (p *PrettyErrorFormatter) Format(err error) { output, _ := cliHumanFormatError("", err, &formatOpts{ Verbose: p.verbose, }) diff --git a/cli/server_test.go b/cli/server_test.go index 21fd076787732..7842f9e62b510 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -1774,21 +1774,7 @@ func TestServerYAMLConfig(t *testing.T) { err = enc.Encode(n) require.NoError(t, err) - wantByt := wantBuf.Bytes() - - goldenPath := filepath.Join("testdata", "server-config.yaml.golden") - - wantByt = clitest.NormalizeGoldenFile(t, wantByt) - if *clitest.UpdateGoldenFiles { - require.NoError(t, os.WriteFile(goldenPath, wantByt, 0o600)) - return - } - - got, err := os.ReadFile(goldenPath) - require.NoError(t, err) - got = clitest.NormalizeGoldenFile(t, got) - - require.Equal(t, string(wantByt), string(got)) + clitest.TestGoldenFile(t, "server-config.yaml", wantBuf.Bytes(), nil) } func TestConnectToPostgres(t *testing.T) { diff --git a/cli/testdata/coder_exp_example-error_api.golden b/cli/testdata/coder_exp_example-error_api.golden new file mode 100644 index 0000000000000..e15b60abbd3d5 --- /dev/null +++ b/cli/testdata/coder_exp_example-error_api.golden @@ -0,0 +1,3 @@ +Encountered an error running "coder exp example-error api", see "coder exp example-error api --help" for more information +error: Top level sdk error message. +Have you tried turning it off and on again? diff --git a/cli/testdata/coder_exp_example-error_arg-required.golden b/cli/testdata/coder_exp_example-error_arg-required.golden new file mode 100644 index 0000000000000..fdb5264072217 --- /dev/null +++ b/cli/testdata/coder_exp_example-error_arg-required.golden @@ -0,0 +1,2 @@ +Encountered an error running "coder exp example-error arg-required", see "coder exp example-error arg-required --help" for more information +error: wanted 1 args but got 0 [] diff --git a/cli/testdata/coder_exp_example-error_cmd.golden b/cli/testdata/coder_exp_example-error_cmd.golden new file mode 100644 index 0000000000000..aaae237095144 --- /dev/null +++ b/cli/testdata/coder_exp_example-error_cmd.golden @@ -0,0 +1,2 @@ +Encountered an error running "coder exp example-error cmd", see "coder exp example-error cmd --help" for more information +error: some error: function decided not to work, and it never will diff --git a/cli/testdata/coder_exp_example-error_multi-error.golden b/cli/testdata/coder_exp_example-error_multi-error.golden new file mode 100644 index 0000000000000..73a32afd8006c --- /dev/null +++ b/cli/testdata/coder_exp_example-error_multi-error.golden @@ -0,0 +1,7 @@ +Encountered an error running "coder exp example-error multi-error", see "coder exp example-error multi-error --help" for more information +error: 3 errors encountered: Trace=[wrapped: ]) +1. first error: function decided not to work, and it never will +2. second error: function decided not to work, and it never will +3. Trace=[wrapped api error: ] + Top level sdk error message. + magic dust unavailable, please try again later diff --git a/cli/testdata/coder_exp_example-error_multi-multi-error.golden b/cli/testdata/coder_exp_example-error_multi-multi-error.golden new file mode 100644 index 0000000000000..b69d33af6ddc5 --- /dev/null +++ b/cli/testdata/coder_exp_example-error_multi-multi-error.golden @@ -0,0 +1,6 @@ +2 errors encountered: +1. Encountered an error running "coder exp example-error multi-multi-error", see "coder exp example-error multi-multi-error --help" for more information + error: 2 errors encountered: + 1. first error: function decided not to work, and it never will + 2. second error: function decided not to work, and it never will +2. close /dev/stdin: file already closed diff --git a/cli/testdata/coder_exp_example-error_validation.golden b/cli/testdata/coder_exp_example-error_validation.golden new file mode 100644 index 0000000000000..02f24e23e1ed4 --- /dev/null +++ b/cli/testdata/coder_exp_example-error_validation.golden @@ -0,0 +1 @@ +Missing values for the required flags: magic-word From fe9efa5b6f7fe3ef683768e910bc63058b631077 Mon Sep 17 00:00:00 2001 From: elasticspoon Date: Fri, 29 Mar 2024 18:12:26 -0400 Subject: [PATCH 2/2] refactor: remove test conditional Change multi-multi-error do not produce an error using a second close of os.Stdin --- cli/errors.go | 14 +++++--------- cli/errors_test.go | 7 ------- ...oder_exp_example-error_multi-multi-error.golden | 12 ++++++------ 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/cli/errors.go b/cli/errors.go index 414474c3aecc1..fbcaf8091c95b 100644 --- a/cli/errors.go +++ b/cli/errors.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "net/http/httptest" - "os" "golang.org/x/xerrors" @@ -83,15 +82,12 @@ func (RootCmd) errorExample() *serpent.Command { Use: "multi-multi-error", Short: "This is a multi error inside a multi error", Handler: func(inv *serpent.Invocation) error { - // Closing the stdin file descriptor will cause the next close - // to fail. This is joined to the returned Command error. - if f, ok := inv.Stdin.(*os.File); ok { - _ = f.Close() - } - return errors.Join( - xerrors.Errorf("first error: %w", errorWithStackTrace()), - xerrors.Errorf("second error: %w", errorWithStackTrace()), + xerrors.Errorf("parent error: %w", errorWithStackTrace()), + errors.Join( + xerrors.Errorf("child first error: %w", errorWithStackTrace()), + xerrors.Errorf("child second error: %w", errorWithStackTrace()), + ), ) }, }, diff --git a/cli/errors_test.go b/cli/errors_test.go index cf34fdfa219ad..75272fc86d8d3 100644 --- a/cli/errors_test.go +++ b/cli/errors_test.go @@ -3,7 +3,6 @@ package cli_test import ( "bytes" "fmt" - "os" "strings" "testing" @@ -62,12 +61,6 @@ ExtractCommandPathsLoop: inv.Stderr = &outBuf inv.Stdout = &outBuf - // This example expects to close stdin twice and joins - // the error messages to create a multi-multi error. - if tt.Name == "coder exp example-error multi-multi-error" { - inv.Stdin = os.Stdin - } - err := inv.Run() errFormatter := cli.NewPrettyErrorFormatter(&outBuf, false) diff --git a/cli/testdata/coder_exp_example-error_multi-multi-error.golden b/cli/testdata/coder_exp_example-error_multi-multi-error.golden index b69d33af6ddc5..029710e7d4aec 100644 --- a/cli/testdata/coder_exp_example-error_multi-multi-error.golden +++ b/cli/testdata/coder_exp_example-error_multi-multi-error.golden @@ -1,6 +1,6 @@ -2 errors encountered: -1. Encountered an error running "coder exp example-error multi-multi-error", see "coder exp example-error multi-multi-error --help" for more information - error: 2 errors encountered: - 1. first error: function decided not to work, and it never will - 2. second error: function decided not to work, and it never will -2. close /dev/stdin: file already closed +Encountered an error running "coder exp example-error multi-multi-error", see "coder exp example-error multi-multi-error --help" for more information +error: 2 errors encountered: +1. parent error: function decided not to work, and it never will +2. 2 errors encountered: + 1. child first error: function decided not to work, and it never will + 2. child second error: function decided not to work, and it never will