testify
testify copied to clipboard
Replace type assertions with reflect.ValueOf()
Summary
Replace type assertions with reflect.ValueOf(...).Int() to fix comparison for aliased types
Changes
obj1.(int)=>reflect.ValueOf(obj1).Int()- Reduce number of cases since we can handle different types in one block
- Add tests for comparing order of
time.Duration(alias ofint64)
Motivation
From reflect perspective both int64 and time.Duration (or any other aliased type) have type reflect.Int64 but type assert will not work because it's different types. This PR supposed to fix this.
assert.Greater(t, time.Second, time.Millisecond)
...
panic: interface conversion: interface {} is time.Duration, not int64
Related issues
Fixes #780
@boyan-soubachov this should go into 2.0 right?
I think I may have wrongly labelled this change. Having a second look, it doesn't actually look like a breaking change. My only concern is given that it's a hot path in the code, the reflection would most likely result in a significant performance hit.
Would it not be a good idea to add some testing.TB style benchmarks to this PR so that we can compare?
Would it not be a good idea to add some
testing.TBstyle benchmarks to this PR so that we can compare?
Agreed.
I think it's unlikely two more reflection calls will cause a significant performance hit since this code already uses reflection to determine type of objects.
Anyway, I'm not sure if this PR should be merged now since 51595dcf940cfb09eb91b6b78babba78bb2bfb25 fixed comparison of aliased types so feel free to close it. Otherwise I can resolve conflicts and add some benchmarks for this code
I think it's unlikely two more reflection calls will cause a significant performance hit since this code already uses reflection to determine type of objects.
Anyway, I'm not sure if this PR should be merged now since 51595dc fixed comparison of aliased types so feel free to close it. Otherwise I can resolve conflicts and add some benchmarks for this code
Probably not but we'd still like to have data to be sure of the impact :)
Up to you, but the way the switch has been cleaned up seems like a very useful thing to merge, even if we leave out any fixes and new features.