- Notifications
You must be signed in to change notification settings - Fork928
feat(cli): add golden tests for errors (#11588)#12698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This all looks good 👍. Was just talking about getting this in, thanks!
Let's just export the originalprettyErrorFormatter and thenew. If we can import that directly, it'll make code navigation easier.
You will have to rebase onmain 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Searching sub commands is very annoying at present, I might make a PR to improve this experience inserpent. 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.varexampleErrorRootCmd*serpent.CommandrootCmd.Walk(func(command*serpent.Command) {ifcommand.Name()=="example-error" {exampleErrorRootCmd=command}})require.NotNil(t,exampleErrorRootCmd,"example-error command not found")varcases []commandErrorCaseExtractCommandPathsLoop:for_,cp:=rangeextractCommandPaths(nil,exampleErrorRootCmd.Children) {name:=fmt.Sprintf("coder %s",strings.Join(cp," "))for_,tt:=rangecases {iftt.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 racerefactor: make formatter publicParse error command in a less brittle way
a7bc37e toc6e6634Compare@Emyrk addressed the issues. And exporting the formatted did clean it up a decent amount. Only other concern I have is: iftt.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.iff,ok:=inv.Stdin.(*os.File);ok {_=f.Close() }returnerrors.Join(xerrors.Errorf("first error: %w",errorWithStackTrace()),xerrors.Errorf("second error: %w",errorWithStackTrace()), )},I would like to change the example error to something like: returnerrors.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 errorusing a second close of os.Stdin
@Emyrk done |
Merged! Thanks@elasticspoon ! |
Uh oh!
There was an error while loading.Please reload this page.
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.