stretchr / testify

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

feat: add assert.Consistently #1606

Open jfmyers9 opened 4 months ago

jfmyers9 commented 4 months ago

Summary

This changeset adds the assert.Consistently and it's associated functions to assert that a condition is true over the entire period of waitFor. This is useful when testing the behavior of asynchronous functions.

Changes

Motivation

See: https://github.com/stretchr/testify/issues/1087#issuecomment-2139222285

Related issues

Closes https://github.com/stretchr/testify/issues/1087

dolmen commented 3 months ago

I don't want more uses of CollectT (which should never have been exposed as a concrete type in the first place). So let's remove all the WithT functions for now.

dolmen commented 3 months ago

We already have Never and Neverf which match the use case: it just need a reversed condition.

z0marlin commented 3 months ago

I don't want more uses of CollectT (which should never have been exposed as a concrete type in the first place). So let's remove all the WithT functions for now.

@dolmen, curious, what are the issues with CollectT? Also, I cannot say much about the CollectT type but I have found WithT quite useful in cases when I already have a certain set of assertions and want to assert that they succeed eventually (testing k8s controllers).

dolmen commented 3 months ago

CollectT should have been an interface to keep its implementation opaque.

ccoVeille commented 3 months ago

We already have Never and Neverf which match the use case: it just need a reversed condition.

Make sense, what would be the reversed condition asserter ? NotNever ? The name would be strange. But using another name that is not the opposite of Never would be strange no?

jfmyers9 commented 3 months ago

In my opinion the opposite of Never would be Always.

Consistently works better as a name when paired with Eventually.

I don't have a strong preference on naming here.

jfmyers9 commented 3 months ago

If we are happy with Always, then this just becomes:

func Always(t TestingT, condition func() bool, waitFor time.Duration, tick time.Duration, msgAndArgs ...interface{}) bool {
    return Never(t, func() bool { return !condition() }, waitFor, tick, msgAndArgs)
}

I missed the fact that Never exists when implementing this. Thoughts on moving to that approach?

z0marlin commented 3 months ago

CollectT should have been an interface to keep its implementation opaque.

Got it. So you would like to get rid of CollectT but not the WithT feature?

z0marlin commented 3 months ago

I missed the fact that Never exists when implementing this. Thoughts on moving to that approach?

My only concern here is that using Never will not allow having something like ConsistentlyWithT or AlwaysWithT (not really concerned about the naming). Not sure about how big of a use-case it is though.

dolmen commented 3 months ago

Never isn't a great name as it isn't what users expect. Always is in the same bucket, or even worse. I think that Consistently is a better name as it is consistent (pun intended) with Eventually, even if inconsistent with Never.

I was reluctant to add Consistently (I want to limit the increase the API surface), but as Never already exists it would be consistent (pun intended) to have it also. I understand also that ConsistentlyWithT would be convenient as it would allow to use require. NeverWithT would not make sense.

About the implementation, I think it's a bit early because we are not yet done with Eventually's bugs. I would like every eyes to focus on #1611.