Open vespian opened 4 years ago
I think I understand what you're trying to achieve. I agree that there's no point waiting for a timeout when the result cannot change.
I think the rationale of the current implementation is that an Eventually() or Consistently() will poll until the timeout, unless it can be absolutely certain that the result will not change. If there is any doubt then it will continue polling. If we change that, it could allow for subtle bugs. For instance the Exit
matcher knows that the result cannot change once the processes has finished. But a function in the Eventually()
does not guarantee to return the same process each time, so the assumption could be wrong. Similarly the Receive()
matcher knows that the result cannot change once a channel is closed. But a function may return a different channel each time, so the assumption is not safe.
Could something like this work?
deploymentFinished := func() bool { ... }
deploymentSuccessful := func() bool { ... }
Eventually(deploymentFinished).Should(BeTrue())
Expect(deploymentSuccessful).To(BeTrue())
That way the logic about whether the result can change is handled by the user-defined function.
I understand where you are coming from and the workaround you suggested would work in the use case I have. Thank you very much for the detailed explanation!
Just as a note, I am of the opinion that users should be given freedom, even when there is a chance of fatal mistakes. E.g. lots of Linux tools offer --yes
option even though it also allows users to make fat-finger kind of mistakes and wipe their hard drives out. But again - I just wanted to share a different point of view :)
Would it be possible to put the justification you have given and the workaround code into the Gomega documentation? Somewhere near the entry for MatchMayChangeInTheFuture? I find it super helpful and for users like me who just started with Gomega, it could save a lot of time.
Thanks in advance!
Thank you @vespian. I'll improve the docs.
For whatever it's worth, I just ran into this issue as well, and the example above (EKS cluster) is a great analog for the case I was testing.
I'll use the workaround for now but it would be nice if Eventually
(or an equivalent function?) had the ability to break out early if the function being called instructed it to do so.
Am not at my computer so I’ll need to check to be sure... but i think if you panic in the function it will abort.
On Tue, Oct 20, 2020 at 2:52 PM Matthew Christopher < notifications@github.com> wrote:
For whatever it's worth, I just ran into this issue as well, and the example above (EKS cluster) is a great analog for the case I was testing.
I'll use the workaround for now but it would be nice if Eventually (or an equivalent function?) had the ability to break out early if the function being called instructed it to do so.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/onsi/gomega/issues/386#issuecomment-713132345, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEIJFXJPP4LGKDFHWYA4V3SLXZ7LANCNFSM4M64HLAQ .
@onsi that seems a big hammer to use just to fail a test? I'm definitely not an expert on best practices though, so open to being corrected on that understanding.
I've wondered about this in the past. One question I have is: If there is a terminal error inside an Eventually
, should the test fail? If the answer is yes, then here's an example of "failing fast" when reaching such an error. Note that cleanup can be handled in a deferred function in the test spec, or in an AfterEach
.
package example_test
import (
"fmt"
"os"
"time"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
)
var _ = Describe("When running", func() {
It("should fail fast from Eventually", func() {
defer func() {
fmt.Fprintf(GinkgoWriter, "cleaning up in a deferred function\n")
}()
Eventually(func() bool {
if _, ok := os.LookupEnv("FAIL"); ok {
Fail("detected a terminal error, retrying is a waste of time\n")
}
fmt.Fprintf(GinkgoWriter, "trying again...\n")
return false
}, 5*time.Second, 1*time.Second).Should(BeTrue())
})
AfterEach(func() {
fmt.Fprintf(GinkgoWriter, "cleaning up in AfterEach\n")
})
})
Run with simulated terminal error:
> FAIL=true ginkgo ./eventually
Running Suite: Example
======================
Random Seed: 1611100252
Will run 1 of 1 specs
cleaning up in a deferred function
cleaning up in AfterEach
• Failure [0.001 seconds]
When running
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:12
should fail fast from Eventually [It]
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:14
detected a terminal error, retrying is a waste of time
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:20
------------------------------
Summarizing 1 Failure:
[Fail] When running [It] should fail fast from Eventually
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:20
Ran 1 of 1 Specs in 0.001 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 0 Skipped
--- FAIL: TestExample (0.00s)
FAIL
Ginkgo ran 1 suite in 560.373574ms
Test Suite Failed
Run without simulated terminal error:
> ginkgo ./eventually
Running Suite: Example
======================
Random Seed: 1611101262
Will run 1 of 1 specs
trying again...
trying again...
trying again...
trying again...
trying again...
cleaning up in a deferred function
cleaning up in AfterEach
• Failure [5.002 seconds]
When running
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:12
should fail fast from Eventually [It]
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:14
Timed out after 5.001s.
Expected
<bool>: false
to be true
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:24
------------------------------
Summarizing 1 Failure:
[Fail] When running [It] should fail fast from Eventually
/home/dlipovetsky/projects/ginkgo-experiments/eventually/example_test.go:24
Ran 1 of 1 Specs in 5.003 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 0 Skipped
--- FAIL: TestExample (5.00s)
FAIL
Ginkgo ran 1 suite in 5.594551988s
Test Suite Failed
instead of setting an env variable and reading it, can we do the check based on the returned error. like "nil" representing "success - continue" can we not have an err for "failure - continue/abort" and "failure - retry"
i am just starting here, please correct me if i am wrong
I also feel that aborting early based on a special error is the most elegant solution for this. I recently implemented such a "final" error for Kubernetes polling functions (not based on gomega):
// FinalError constructs an error that indicates to a poll function that
// polling can be stopped immediately because some permanent error has been
// encountered that is not going to go away.
func FinalError(err error) error {
return &FinalErr{Err: err}
}
type FinalErr struct {
Err error
}
func (err *FinalErr) Error() string {
if err.Err != nil {
return fmt.Sprintf("final error: %s", err.Err.Error())
}
return "final error, exact problem unknown"
}
func (err *FinalErr) Unwrap() error {
return err.Err
}
// IsFinal checks whether the error was marked as final by wrapping some error
// with FinalError.
func IsFinal(err error) bool {
var finalErr *FinalErr
return errors.As(err, &finalErr)
}
When Eventually
is passed a function which returns a value and an error, then Eventually
could use IsFinal
to abort early and log that error as failure.
Hey all sorry for the extended delay here. The latest commit on master now introduces StopTrying("message")
which is documented here. It should be out in a versioned release soon.
You can return StopTrying("message")
as an error from your polled function or you can call StopTrying("message").Now()
to immediately end execution without having to thread a new error return value through.
What isn't clear from the description is whether gomega handles wrapping of the StopTrying
error. I haven't looked at the implementation.
In other words, will this work?
return fmt.Errorf("some operation: %w", StopWriting("stop"))
Ah good catch - it will not work currently but I should be able to use errors.As
to get around that. I'll take a look.
yep - I had to do the errors.As
dance. The code is written and tested and I'll cut a patch release later today.
Documentation mentions that:
which is reflected in the code: https://github.com/onsi/gomega/blob/1a3d249459a44387a05ca2d2c2b3d5f3db596dcb/internal/asyncassertion/async_assertion.go#L98-L100
There are use-cases where aborting a function can still be usefull - imagine spinning EKS cluster on Amazon, with
Eventually
checking output of the function. Once the cluster transitions into "CREATE_FAILED" state - it will not heal itself, and Gomega forces users to wait the timeout as there is no way to abort the test.Can it be changed so that if the
MatchMayChangeInTheFuture
is defined for the matcher, it is executed no matter if theactual
is a function or a value so that people can decide on their own?Thanks in advance.