stretchr / testify

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

Feature: assert.Consistently #1087

Open shaneutt opened 3 years ago

shaneutt commented 3 years ago

I've been using testify for a long time now, I appreciate you building and maintaining this for us all :bow:

I'm looking for functionality that is inspired by other testing frameworks that would be similar to assert.Eventually and require.Eventually, but instead would be:

stagger := time.Millisecond * 200
tries := 15
assert.Consistently(t, func() bool {
   fmt.Println("my logic here")
   return result()
}, stagger, tries)

The point of this would be to ensure that a result is consistently occurring over a period of time. In the above example, that the fake result() function is returning true consistently every 200ms for 3 seconds total.

Does testify already have something like this and I've just missed it?

If not, are you cool with me contributing something like this?

Thanks!

brackendawson commented 3 years ago

Would Consistently() be more readable over say this code:

for i := 1 ; i <= 15 ; i++ {
    t.Logf("attempt %d", i)
    time.Sleep(200*time.Millisecond)
    got := myCode()
    assert.Equal(t, expectation, got)
}

Another concern I have is what if someone calls t.Error() inside the function? The testify framework will have no way to know they did that, so the Consistently() assertion will pass but the test will fail and you wont be told on which attempt it failed.

Edit: Actually testify could call t.Failed() after each iteration to see if it passed. Perhaps using t and regular assertions in the function is a better way to indicate failures than having a return value because then all assertions are available to the user. Calls to t.FailNow() would still escape though.

shaneutt commented 3 years ago

Readability is subjective: I'm more interested in the prescriptiveness it would bring: setting a clear standard in tests that use testify for how to achieve this and leaving less room for interpretation for contributors. As far as your concerns about t.Error does that problem also exist for Eventually or no?

brackendawson commented 3 years ago

Eventually probably does have the same quirk with use of t inside it's function. And it does set a precedent for your suggested function signature.

I think I wouldn't object to adding this.

brackendawson commented 3 years ago

Maybe it should take two durations, a tick and a testFor, a bit like Eventually does?

If the function takes longer than the tick do you run them concurrently or fail the assertion?

shaneutt commented 3 years ago

Maybe it should take two durations, a tick and a testFor, a bit like Eventually does?

Right, in the original post I had mocked it up that way but apparently messed up the var names so it's a little confusing, here's a redux:

assert.Consistently(t, func() bool {
   fmt.Println("my logic here")
   return result()
}, testFor, tick)

If the function takes longer than the tick do you run them concurrently or fail the assertion?

Good question, I think the caller should have to instruct the test mechanism in regards to how long the actual test takes.

Perhaps this would be a better signature:

assert.Consistently(t, func() bool {
   fmt.Println("my logic here")
   return result()
}, totalTestDuration, requiredSuccesses, tick)

This fits well with my use case, because the current tests I would use these in test things like HTTP responses to a local API and this adds a slight performance testing edge to the mix which I think adds to the utility, and fits with the theme of "consistency" I think.

brackendawson commented 3 years ago

Maybe, specifying a total duration and a number of successful executions feel a little over-constrained though? You can define a Consistently call which can never pass. It might just be that it needs a better name, but it sounds like requiredSuccesses means you can have some failures so long as you get that many passes, which isn't consistent.

It makes me lean towards something similar to how you wrote it in your first post here actually, even though that wasn't what you intended:

func Consistently(t TestingT, f func() bool, every time.Duration, times int) bool

Run f times times at least every times apart. So if f is slower than every the test takes longer but still runs times times. I know it's not a performance test any more, is it an approach that fits your use case?

shaneutt commented 3 years ago

Maybe, specifying a total duration and a number of successful executions feel a little over-constrained though? You can define a Consistently call which can never pass. It might just be that it needs a better name, but it sounds like requiredSuccesses means you can have some failures so long as you get that many passes, which isn't consistent.

It makes me lean towards something similar to how you wrote it in your first post here actually, even though that wasn't what you intended:

func Consistently(t TestingT, f func() bool, every time.Duration, times int) bool

Run f times times at least every times apart. So if f is slower than every the test takes longer but still runs times times. I know it's not a performance test any more, is it an approach that fits your use case?

Yes, this definition works too. The performance style stuff can be a consideration for later.

MatthiasReumann commented 2 years ago

Hey! I would love to implement that feature. As far as I am aware the way to do this is:

  1. Fork the Repository
  2. Implement the feature
  3. Create PR to merge my work into the original repository
  4. "include a complete test function that demonstrates the issue"

Did I understand the process correctly? Thanks a lot!

dolmen commented 1 year ago

It looks like it could be called Repeat as well:

func Repeat(t TestingT, f func() bool, every time.Duration, times int) bool

However you can use the -count parameter of go test to repeat a test. Also, you could just wrap your test in a loop with time.Sleep. I don't think this is worth having to learn the testify API to be able to read the test code.

In addition, I see nothing directly related to assert that would require it to be in the same package.

So I'm not convinced that this is worth extending the API surface of testify for this feature.

jfmyers9 commented 1 year ago

In my mind this isn't intended as a utility to repeat a test, but instead it serves as the inverse of Eventually when testing asynchronous code.

Consistently allows someone to write an assertion that something will hold true over the acceptable period of time. Obviously this should be used within reason as it will increase the duration of the test by the specified interval, but from an assertion perspective:

require.Consistently(t, func bool() {
  // do something
}, duration, interval, message);

Reads nicer than:

for i := 1 ; i <= 15 ; i++ {
    t.Logf("attempt %d", i)
    time.Sleep(200*time.Millisecond)
    got := myCode()
    assert.Equal(t, expectation, got)
}

To provide a concrete example: https://book.kubebuilder.io/cronjob-tutorial/writing-tests#testing-your-controllers-behavior

You can see how Kubebuilder uses a similar assertion provided by Gomega when testing Kubernetes Operators:


            By("By checking the CronJob has zero active Jobs")
            Consistently(func() (int, error) {
                err := k8sClient.Get(ctx, cronjobLookupKey, createdCronjob)
                if err != nil {
                    return -1, err
                }
                return len(createdCronjob.Status.Active), nil
            }, duration, interval).Should(Equal(0))
z0marlin commented 5 months ago

Any updates on this issue? I have a use case similar to @jfmyers9, for testing K8s operators.

dolmen commented 5 months ago

We have not discussed what should happen in case of failure of the condition:

  1. Stop immediately on the first failure
  2. Run until the full duration elapsed and report if at least one failure occurred. Report each failure with t.Errorf.
dolmen commented 5 months ago

It appears we already have assert.Never and Neverf which looks like this proposal, but with the condition reversed.

So it looks like we just need to improve the documentation to link to it from the doc of Eventually*.

z0marlin commented 5 months ago

Currently there is:

  1. assert.Eventually - to assert that some condition is true within some duration.
  2. assert.Never - to assert that a condition is consistently false for a given duration.
  3. There is also assert.EventuallyWithT - similar to assert.Eventually, but passes a CollectT to the condition func which the func can use to perform its own assertions.

Now, in my case, I have a use case for a construct similar to assert.EventuallyWithT where I want to test that certain assertions consistently return no error for a specific duration. So something like a assert.ConsistentlyWithT. assert.Never can be used in place of assert.Consistently by inverting the condition func, but it cannot be used in the case when I want the condition func to perform its own assertions.