Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: assert.Consistently #1087

Open
shaneutt opened this issue Jun 16, 2021 · 14 comments · May be fixed by #1606
Open

Feature: assert.Consistently #1087

shaneutt opened this issue Jun 16, 2021 · 14 comments · May be fixed by #1606
Labels
assert.Eventually About assert.Eventually/EventuallyWithT enhancement pkg-assert Change related to package testify/assert revisit

Comments

@shaneutt
Copy link

shaneutt commented Jun 16, 2021

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

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
Copy link
Collaborator

brackendawson commented Jul 16, 2021

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
Copy link
Author

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
Copy link
Collaborator

brackendawson commented Jul 16, 2021

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
Copy link
Collaborator

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
Copy link
Author

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)
  • totalTestDuration: how long overall the test will run
  • requiredSuccesses: how many success results are required during totalTestDuration

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link

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
Copy link
Collaborator

dolmen commented Jul 7, 2023

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.

@dolmen dolmen added pkg-assert Change related to package testify/assert enhancement revisit labels Jul 7, 2023
@dolmen dolmen added the assert.Eventually About assert.Eventually/EventuallyWithT label Jul 31, 2023
@jfmyers9
Copy link

jfmyers9 commented Aug 9, 2023

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
Copy link

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

jfmyers9 added a commit to jfmyers9/testify that referenced this issue Jun 4, 2024
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.

Closes stretchr#1087
@jfmyers9 jfmyers9 linked a pull request Jun 4, 2024 that will close this issue
jfmyers9 added a commit to jfmyers9/testify that referenced this issue Jun 4, 2024
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.

Closes stretchr#1087
@dolmen
Copy link
Collaborator

dolmen commented Jun 13, 2024

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
Copy link
Collaborator

dolmen commented Jun 13, 2024

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert.Eventually About assert.Eventually/EventuallyWithT enhancement pkg-assert Change related to package testify/assert revisit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants