testify icon indicating copy to clipboard operation
testify copied to clipboard

Replace type assertions with reflect.ValueOf()

Open Pr0Ger opened this issue 5 years ago • 5 comments

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 of int64)

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

Pr0Ger avatar Jun 12 '20 14:06 Pr0Ger

@boyan-soubachov this should go into 2.0 right?

mvdkleijn avatar Jul 30 '20 21:07 mvdkleijn

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?

boyan-soubachov avatar Aug 03 '20 10:08 boyan-soubachov

Would it not be a good idea to add some testing.TB style benchmarks to this PR so that we can compare?

Agreed.

mvdkleijn avatar Aug 04 '20 11:08 mvdkleijn

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

Pr0Ger avatar Aug 06 '20 14:08 Pr0Ger

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.

boyan-soubachov avatar Aug 10 '20 11:08 boyan-soubachov