add additional test assertions: assert.ErrEqual, must.SucceedT, must.ReturnT #257
Conversation
This unifies all the various ad-hoc test helper methods that we created over the years into one interface.
As the code comment in ReturnT indicates, I have agonized over these function signatures for a long time. This might still not be the optimal solution, but in the end, having this is better than having ad-hoc helper functions splattered all over the place.
majewsky
requested review from
Nuckal777,
SuperSandro2000,
VoigtS and
wagnerd3
as code owners
assert/assert.go
Outdated
| if actual == nil { | ||
| if expected == nil { | ||
| // defense in depth: this should have been covered by the previous case branch | ||
| return true | ||
| } | ||
| t.Errorf("expected error stack to contain %q, but got no error", expected.Error()) | ||
| return false | ||
| } | ||
| if expected == nil { | ||
| // defense in depth: this should have been covered by the previous case branch | ||
| t.Errorf("expected success, but got error: %s", actual.Error()) | ||
| return false | ||
| } |
There was a problem hiding this comment.
| if actual == nil { | |
| if expected == nil { | |
| // defense in depth: this should have been covered by the previous case branch | |
| return true | |
| } | |
| t.Errorf("expected error stack to contain %q, but got no error", expected.Error()) | |
| return false | |
| } | |
| if expected == nil { | |
| // defense in depth: this should have been covered by the previous case branch | |
| t.Errorf("expected success, but got error: %s", actual.Error()) | |
| return false | |
| } | |
| if expected == nil { | |
| // defense in depth: this should have been covered by the previous case branch | |
| t.Errrof("this should not happen) | |
| return true | |
| } | |
| if actual == nil { | |
| t.Errorf("expected error stack to contain %q, but got no error", expected.Error()) | |
| return false | |
| } |
There was a problem hiding this comment.
I don't want to error out on this. Whether these "defense in depth" branches are taken or not depends on how you read this specific part in the spec:
Instead of a type, a case may use the predeclared identifier
nil; that case is selected when the expression in the TypeSwitchGuard is a nil interface value.
I could swear that, in earlier Go versions, this only matched the plain nil value and not e.g. error(nil), i.e. an any-typed value containing an error interface instance with the contained error nil. It appears to be different now, but this part of the spec has not changed since Go 1.10, so I'm assuming this to be undefined behavior and thus took care to make those branches behave the same, so that it does not matter what choices the compiler implementers take in the future.
Merging this branch changes the coverage (1 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
These unify all the various ad-hoc test helper methods that we created over the years into two interfaces:
As the code comments indicate, I have agonized over these function signatures for a long time. This changeset may not be the optimal solution, but in the end, I feel that having these is better than having ad-hoc helper functions scattered all over all our codebases.