feat(cli): add golden tests for errors (#11588) #12698
Conversation
|
Thanks for the PR @elasticspoon. There seems to be a data race, caught by CI, would you mind taking a look at that? |
|
Fixed |
Honestly, if it simplifies the imports, just export |
There was a problem hiding this comment.
This all looks good 👍. Was just talking about getting this in, thanks!
Let's just export the original prettyErrorFormatter and the new. If we can import that directly, it'll make code navigation easier.
You will have to rebase on main to get the latest error change update as well.
Ping me with any updates, happy to fast track this in 👍
cli/errors_test.go
Outdated
| var cases []commandErrorCase | ||
|
|
||
| ExtractCommandPathsLoop: | ||
| for _, cp := range extractCommandPaths(nil, rootCmd.Children) { | ||
| name := fmt.Sprintf("coder %s", strings.Join(cp, " ")) | ||
| // space to end to not match base exp example-error | ||
| if !strings.Contains(name, "exp example-error ") { | ||
| continue | ||
| } | ||
| for _, tt := range cases { | ||
| if tt.Name == name { | ||
| continue ExtractCommandPathsLoop | ||
| } | ||
| } | ||
| cases = append(cases, commandErrorCase{Name: name, Cmd: cp}) | ||
| } |
There was a problem hiding this comment.
Searching sub commands is very annoying at present, I might make a PR to improve this experience in serpent. Until then though, can we do something a bit more exact? It feels strange to match a string like this.
We can do something like this:
// Walk all commands and find the `example-error` subcommand.
// Unfortunately, this cannot abort early, but it's not that huge of
// a list to walk.
var exampleErrorRootCmd *serpent.Command
rootCmd.Walk(func(command *serpent.Command) {
if command.Name() == "example-error" {
exampleErrorRootCmd = command
}
})
require.NotNil(t, exampleErrorRootCmd, "example-error command not found")
var cases []commandErrorCase
ExtractCommandPathsLoop:
for _, cp := range extractCommandPaths(nil, exampleErrorRootCmd.Children) {
name := fmt.Sprintf("coder %s", strings.Join(cp, " "))
for _, tt := range cases {
if tt.Name == name {
continue ExtractCommandPathsLoop
}
}
cases = append(cases, commandErrorCase{Name: name, Cmd: cp})
}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
a7bc37e to
c6e6634
Compare
|
@Emyrk addressed the issues. And exporting the formatted did clean it up a decent amount. Only other concern I have is: if tt.Name == "coder exp example-error multi-multi-error" {
inv.Stdin = os.Stdin
}I have that line in my test because the multi-multi error does: {
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()),
)
},I would like to change the example error to something like: return errors.Join(
xerrors.Errorf("parent error: %w", errorWithStackTrace()),
errors.Join(
xerrors.Errorf("child first error: %w", errorWithStackTrace()),
xerrors.Errorf("child second error: %w", errorWithStackTrace()),
),
)but I am unsure if this changes the meaning of what is being tested. Is the example test only for the rendering of error or should it also be testing that the errors get joined together correctly? |
|
@elasticspoon Change the test to what you suggested. > ```go
> return errors.Join(
> xerrors.Errorf("parent error: %w", errorWithStackTrace()),
> errors.Join(
> xerrors.Errorf("child first error: %w", errorWithStackTrace()),
> xerrors.Errorf("child second error: %w", errorWithStackTrace()),
> ),
> )
> ```
It's all fine imo. The Make that change, drop the condition, and I'll green check this 👍 |
Change multi-multi-error do not produce an error using a second close of os.Stdin
|
@Emyrk done |
|
Merged! Thanks @elasticspoon ! |
Fixes #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.
Most of the error printing is handled bycli.prettyErrorFormatterandprettyErrorFormatter.format(err), neither is exported. I wanted to add tests inroot_internal_test.gobut that resulted in a circular dependency since it neededclitest.I solved this issue by creating anexport_cli_test.gofile in which I exported those values. Since it is an_testfile its exports will limited tocli_testpackage files.Exported the formatted for testing purposes.