gotestyourself / gotest.tools

A collection of packages to augment the go testing package and support common patterns.
https://gotest.tools/v3
Other
513 stars 50 forks source link

assert: use type constraints (generics) #255

Open dnephin opened 1 year ago

dnephin commented 1 year ago

Closes #249

Once merged, this change would require the use of go1.18+ to import gotest.tools/v3.

This PR adds type constraints to the functions in assert and assert/cmp. By using type constraints the compiler (and also any IDE or editor that uses gopls) can warn the user if they are using different types as the arguments to comparison functions.

Also use a type constraint for BoolOrComparison. This type was always intended to be a union type, and now it's possible to express that.

glenjamin commented 1 year ago

I wonder if this should be a v4, or do you reckon there's no breaking changes for 1.18+?

dnephin commented 1 year ago

That's the big question for sure. My hope is that it can be backwards compatible for 1.18+.

The function signatures have changed, but they aren't methods on a type. I believe the only way this could break existing code is if the functions are assigned to a variable with an explicit type of func(t assert.TestingT, x any, msg...any). That seems unlikely, but it's possible.

Are there other cases where these changes might break existing code?

glenjamin commented 1 year ago

I suppose if the tests are currently passing, then the types should already match.

Although - what about go-cmp transforms?

dnephin commented 1 year ago

The options ...cmp.Options parameter to DeepEqual isn't changing. I think those should be ok, unless I'm missing something.

glenjamin commented 1 year ago

I think the transform options allow different types to compare equal

dnephin commented 1 year ago

Ahh, if I understand correctly you mean when the args passed to DeepEqual are different types, but a Transformer changes the type. That is possible. It seems strange that someone would do that, since they can transform the arguments before passing them to DeepEqual. I wonder how common it is to use transformers in this way. I don't think I've ever used one myself. I only use ignore and compare.

glenjamin commented 1 year ago

I used that feature once to ignore differences between numeric types, but that was for numbers nestled within a polymorphic structure - but never for two args at the top level

dnephin commented 1 year ago

I'm still not sure if we need a v4 for this. I wrote up #257 with some other changes we might want to make for a v4 release. I think if we decide to increment the major version we should try to include other breaking changes at the same time.

cc @vdemeester as well, to get your thoughts

vdemeester commented 1 year ago

I'm still not sure if we need a v4 for this. I wrote up #257 with some other changes we might want to make for a v4 release. I think if we decide to increment the major version we should try to include other breaking changes at the same time.

cc @vdemeester as well, to get your thoughts

I would tend to agree that it could be a good time to do a v4 and include other breaking changes there 👼🏼. So, I would go the v4 ways at least 👼🏼