Return error when failing to parse deletion time #1447
Conversation
There was a problem hiding this comment.
Thanks for the suggestion, I think there's a small issue with one of the conditions - see below.
I had been thinking about how I could do this yesterday using the new errors functionality (ie. without doing string comparisons) but couldn't find a good solution. Came back today with a fresh mind and found one... see what you think (#1451). Happy for you to land either that or this (with the point below fixed).
| @@ -206,8 +207,10 @@ func TestConvert(t *testing.T) { | |||
| t.Run(test.Description, func(t *testing.T) { | |||
| // Perform conversion | |||
| compatibleH3rls, err := Convert(test.Helm3Release) | |||
| if got, want := err, test.ExpectedError; got != want { | |||
| t.Errorf("got: %v, want: %v", got, want) | |||
| if test.ExpectedError != nil || err != nil { | |||
There was a problem hiding this comment.
This line is problematic: I assume you added it because you saw a panic on either got.Error() or want.Error() (or both) when either of them is nil. So in the case where a test would fail (say a test where test.ExpectedError == nil but an error was returned), then you'd still see a panic. To avoid that, you'd need to replace the || with &&, but then you're effectively skipping this assertion when we don't want to.
We could fix that here easily enough by first testing to ensure we have an error when we didn't expect one (or vice-verse):
if gotError, wantError := err != nil, test.ExpectedError != nil; gotError != wantError {
...
}
if test.ExpectedError != nil && err != nil {
...(something that uses .Error() on both)
}
But, I think this alternative, using the new error wrapping, is simpler and perhaps a good precedent: #1451
|
Thanks for the comment and investigating this! I indeed prefer your solution in #1451, it's cleaner and more readable 👍 |
Description of the change
Context: #1445 (comment)