stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
23.46k stars 1.6k forks source link

Using generics #1147

Closed borosr closed 2 weeks ago

borosr commented 2 years ago

Are you planning to update project to use generics and also support go 1.18?

glesica commented 2 years ago

I'm not sure how much value we would get from generics. I suppose some of the assertions would benefit. But I doubt any of the maintainers is going to put much effort into it any time soon. Pull requests or further discussion are both always welcome, of course!

brackendawson commented 2 years ago

Testify already supports generics. Is there a specific test case you've been unable to write?

jan-xyz commented 2 years ago

I think the assertions would benefit from it. It reduces turn-around time if the compiler already tells you that expected and actual are different types and incompatible instead of waiting for the test result in some cases.

borosr commented 2 years ago

I agree with @jan-xyz, sometimes it would be helpful if the compiler would prevent me to do assertions between different types.

jan-xyz commented 2 years ago

@brackendawson I noticed that you down voted my reply, I'd be interested to hear your reasoning for it? Do you think it's not useful and if so why?

In the meantime, I played around with go generics today and implemented some assertions from testify how I imagine them to be.

pellared commented 2 years ago

I think this feature would require releasing a v2 of testify.

Side note: For sake of "more type-safe" assertions and evaluating a slightly different design, I started experimenting here https://github.com/pellared/fluentassert/pull/26. I would appreciate any feedback.

brackendawson commented 2 years ago

@brackendawson I noticed that you down voted my reply, I'd be interested to hear your reasoning for it? Do you think it's not useful and if so why?

The main reason is that it would require all users of testify to be on Go 1.18 and to have >= 1.18 in their go.mod's go directive, as pellared says it would have to be a v2 release. But there are other things I'd like to see in a v2 release (like a suite package that can handle parallel subtests) that I wouldn't like to restrict to only those on 1.18, at least for now. Maybe this situation is different in a couple of years.

On the actual use of generics for some assertions where testify currently uses interface{}, is it actually better? Errors where the assertion is not the right one to use will be compile time rather than runtime (good, I think) but the message is out of testify's control and it might not be clear what the user needs to do (maybe not good).

And also on your func Equal[T comparable](t *testing.T, expected, actual T, msgAndArgs ...any), testify can currently compare some types which aren't comparable`, slices for example. Testify does this using reflect.DeepEqual or as is proposed for v2; go-cmp (#535). This change would be a regression.

jordaniversen commented 2 years ago

Sorry for my ignorance as I'm new to Go and testify, but would support for generics allow the caveat described at https://pkg.go.dev/github.com/stretchr/testify/mock#hdr-Example_Usage to be voided? That is, would it prevent us from having to do a nil check in our mock method when the return value is a pointer in order to prevent the panic?

This may cause a panic if the object you are getting is nil (the type assertion will fail), in those cases you should check for nil first.

brackendawson commented 2 years ago

Sorry for my ignorance as I'm new to Go and testify, but would support for generics allow the caveat described at https://pkg.go.dev/github.com/stretchr/testify/mock#hdr-Example_Usage to be voided? That is, would it prevent us from having to do a nil check in our mock method when the return value is a pointer in order to prevent the panic?

@jordaniversen I think you're asking if mock.Arguments can be a generic type to avoid needing to do type assertion of values returned from Get(). That's not going to be possible because the arguments are likely to be different types. Both of these would be invalid:

Arguments[any]{"seven", 7}
[]Argument[any]{{"seven"}, {7}}

So I don't think compile time type safety can be achieved here.

967 solves your problem though.

jordaniversen commented 2 years ago

Ah thanks for that @brackendawson. Have you had any update on this?

zhulik commented 2 years ago

As a compromise, is it possible to write generic 'more type-safe' wrappers around assert for example and see how it's in real use? From the fisrt sight such an API should be fully backward compatible

brackendawson commented 2 years ago

As a compromise, is it possible to write generic 'more type-safe' wrappers around assert for example and see how it's in real use? From the fisrt sight such an API should be fully backward compatible

That would be possible and would give you a safer Assert for most purposes, it would have to have a different name (SaferAssert()?) and so would any others you wrap. It would not be backwards compatible at all because modules with a go directive <1.18 would not compile, even with a 1.18 compiler.

zhulik commented 2 years ago

@brackendawson by 'backward compatibility' I meant that safer assert would be a drop-in replacement for assert when built with go 1.18. One only needs to change the import string.

anthony-chang commented 1 year ago

I had a stab at refactoring some of the assertion APIs using generics but there are a couple of roadblocks that make me feel like this is impossible without some major changes to testify and/or Golang itself.

  1. Golang does not support generics in methods with receivers (more detail on this issue here). This makes it impossible to actually genericise the public-facing APIs in the *_forward.go files without substantially changing the testify architecture (ie. stop using receiver methods).
  2. The type inferencing in Golang is not strong enough for arrays/slices/map types for some use cases. For example, consider the assert.Contains method. If we wanted to use generics to force the arguments to be the same slice/map/string type, we might do something like this:
    func Contains[K comparable, V any, T []V | map[K]V | string](t TestingT, s T, contains V, msgAndArgs ...interface{})

    But now when you call Contains, the compiler is not able to infer the argument types.

    arr1 := []int{1, 2, 3}
    arr2 := []int{1, 2, 3}
    Contains(arr1, arr2) // error message: "cannot infer K"

    So you would need to always call it like Contains[int, int](arr1, arr2), which is too intrusive in my opinion.

cc: @mchlp @TheAndrew2115 @KieranJQuan

alokmenghrajani commented 1 year ago

An example where having a generic Equal[T any] would make the test nicer to write:

    a := uint(1)
    assert.Equal(t, 1, a)

This is equivalent to being allowed to compare e.g. uint and int but only when one value is a literal -- something which can't be achieved using reflection.

shoenig commented 1 year ago

FWIW https://github.com/shoenig/test was lifted out of the HashiCorp Nomad project as a generics based re-imagination of testify. It leverages Google's go-cmp library and integrates with specifying custom cmp.Option values (particularly useful for protobuf stuff), as well as specifying custom output formats all via variadic ...Setting arguments. The library de-emphasises magic. As built-in types do not interoperate with generics naturally, you'll see variations of assertions specific to strings, maps, and slices. We've been using and iterating on the library for over a year now, and it's been growing in usage across more projects.

peterldowns commented 1 month ago

Is this still being considered as a potential improvement for the next version of testify?

(For anyone looking for a typesafe alternative using generics, with go-cmp for equality, zero other dependencies, and a much more reasonable set of assertion methods, check out my project https://github.com/peterldowns/testy.)

brackendawson commented 1 month ago

@peterldowns a v2 of testify is currently not being considered. I do recognise that there may be enhancements possible by making some of the assertions generic, but we won't be doing that in in v1.

peterldowns commented 1 month ago

@brackendawson got it, makes sense. Do you know if/when v2 will be considered? The line at the top of the README about it has been there for 4 years, I remember submitting the form myself ages ago, it seems a little misleading to keep it up there with the current wording if there are no plans to work on v2.

brackendawson commented 1 month ago

@peterldowns yes, agreed. I also submitted a note to that form many years ago. I don't have access to the results, I asked one of our emeritus but didn't get a reply.

1518 will remove the note. @hendrywiranto what do you think of those words?

peterldowns commented 1 month ago

@brackendawson thanks Bracken, I appreciate the link to #1518. I appreciate your maintainership and I hope you and the rest of the team keep up the great work.

hendrywiranto commented 2 weeks ago

The words you suggested looks good to me @brackendawson, I have applied the changes in https://github.com/stretchr/testify/pull/1518

brackendawson commented 2 weeks ago

Closing this one as not planned for now.

ddkwork commented 5 days ago

Is this still being considered as a potential improvement for the next version of testify?

(For anyone looking for a typesafe alternative using generics, with go-cmp for equality, zero other dependencies, and a much more reasonable set of assertion methods, check out my project https://github.com/peterldowns/testy.)

nice work