feat: add new helpers to p/uassert #2547
Conversation
kazai777
requested review from
deelawn and
thehowl
and removed request for
a team
the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2547 +/- ##
==========================================
- Coverage 55.02% 55.01% -0.01%
==========================================
Files 595 595
Lines 79665 79645 -20
==========================================
- Hits 43837 43820 -17
+ Misses 32512 32510 -2
+ Partials 3316 3315 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I have some concerns about whether we need all of the functionality introduced here. A lot of it is hard or impossible to get right and consistent with the existing testify packages without using reflection. I'm of the opinion that if we offer something like this with a subset of the same testify API, then the behavior that we do define should be identical. If there is behavior that is not identical we should omit the type and return a "unsupported type" error. But I've also been skeptical of using testify as a package -- the conversion of types to interfaces and then using reflection to get the original types always seemed odd to me when I know what the types are before I call the testify function, so why not just do the equality checks in my testing code.
But are are some things packages like this make easier, like handling panics and doing deep equality checks, though this package can't really do deep equality checking without reflection. Let's wait for someone else to offer a review as well.
| t.Helper() | ||
| switch v1 := e1.(type) { | ||
| case int: | ||
| v2 := e2.(int) |
There was a problem hiding this comment.
Any of these type assertions can fail if the underlying types of e1 and e2 are not the same. This behavior is not consistent with the behavior of the testify package this is modeled after. Instead, it should fail will an error message that types are incompatible, or something of that equivalence.
| return len(v) == 0 | ||
| case func(): | ||
| return v == nil | ||
| default: |
There was a problem hiding this comment.
The default should specify why it failed -- this is an unsupported type.
| return v == nil | ||
| case *func(): | ||
| return v == nil | ||
| case []int: |
There was a problem hiding this comment.
Slice values need to be compared to nil, not have their length checked. A nil slice will always have a length of zero, but non-nil slices of length zero can exist.
| return len(v) == 0 | ||
| case []bool: | ||
| return len(v) == 0 | ||
| case map[string]int: |
There was a problem hiding this comment.
Same applies to maps as for slices; it needs to be compared to nil as both nil and non-nil maps can have a length of zero.
| } | ||
|
|
||
| switch v := object.(type) { | ||
| case *int, *int8, *int16, *int32, *int64: |
There was a problem hiding this comment.
Type switch cases with multiple types won't work -- v will continue to be an interface type so it will evaluate to not nil because the underlying nil value is typed. Each of the types must be on its own separate line. Here is an example of this behavior: https://play.gno.land/p/wTCoLeDZiMb
| return v == nil | ||
| case *map[string]int, *map[string]string, *map[string]bool: | ||
| return v == nil | ||
| case *func(): |
There was a problem hiding this comment.
I'm not sure how useful this one is; an empty function is only of the the infinite types of function signatures. Maybe better to keep functions as unsupported types.
| return len(v) == 0 | ||
| case map[string]bool: | ||
| return len(v) == 0 | ||
| case func(): |
There was a problem hiding this comment.
Same comment as above in regards to whether this should support functions types.
| } | ||
|
|
||
| // ElementsMatch asserts that two lists contain the same elements, regardless of order. | ||
| func ElementsMatch(t TestingT, listA, listB []interface{}, msgs ...string) bool { |
There was a problem hiding this comment.
This signature differs from how it works in testify and how most users might expect it to work. I believe that the natural expectation is to pass two slice to this function and have it check if the elements match. The current implementation requires the slices to be transformed into slices of interfaces before this can be used; if that's the case, they might as well just check equality while iterating over the slices instead of building the interface slices.
So if we want a function like Elements match, it should accept interface{} types and then:
- Check to make sure that the slice types match
- Check to make sure they have the same length
- Check to make sure each of their elements are equal
Even then, the implementation is incomplete because it assumes primitive type slices; there would need to be deeper equality checks done to do this correctly.
| } | ||
|
|
||
| // Subset asserts that all elements in the subset are present in the set. | ||
| func Subset(t TestingT, set, subset []interface{}, msgs ...string) bool { |
There was a problem hiding this comment.
Same concerns as ElementsMatch
| // Same asserts that two pointers reference the same object. | ||
| func Same(t TestingT, expected, actual interface{}, msgs ...string) bool { | ||
| t.Helper() | ||
| if expected != actual { |
There was a problem hiding this comment.
This won't work as expected until this bug is fixed: #2624
Do you think it would make more sense to develop the |
Add functions based on go
testifylibrary to complete thep/uassertpackageContributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description