Test and remove panic possibilities in newDashboardCompatibleRelease #1445
Conversation
| var deleted *timestamp.Timestamp | ||
| if !h3r.Info.Deleted.IsZero() { | ||
| var err error | ||
| deleted, err = ptypes.TimestampProto(h3r.Info.Deleted.Time) | ||
| if err != nil { | ||
| return h2.Release{}, fmt.Errorf("Failed to parse deletion time %v", err) | ||
| return h2.Release{}, ErrFailedToParseDeletionTime |
There was a problem hiding this comment.
I think it's important to have the error text somewhere. That way, if a user reports that it's hitting this error we'd get more info (for example if .IsZero() is not working as expected or if the Time is no longer valid).
If the only reason to set a fixed message is the test, you can still do string.Contains(got, want) instead of got != want which is a bit more flexible and unlikely to report a false-positive.
There was a problem hiding this comment.
See #1451 Thanks!
As far as I can see, the fields that use pointers (which are the ones that may throw a |
|
Merging this because it will conflict with my PR in #1441 I can open a follow-up PR with my comment later on. |
Description of the change
Related to a question I had at #1434 (comment) , it was unclear why we were handling panics in tests rather than eliminating them.
I added a number of test cases which caused a panic as mentioned in the above comment, then fixed with a guard clause. While there, I also added some tests for the parsing of the Deleted timestamp.
Benefits
A number of conditions which could cause a panic have been eliminated.
Possible drawbacks
The error for the failed parsing of the Deletion timestamp is less explicit (so I could test it without doing string comparisons - guilty I'll update it if you think it's important)
Applicable issues
Additional information