Option to abort Eventually/Consistently also for functions

[vespian]

Documentation mentions that:

Note: Eventually and Consistently only exercise the MatchMayChangeInTheFuture method if they are passed a bare value. If they are passed functions to be polled it is not possible to guarantee that the return value of the function will not change between polling intervals. In this case, MatchMayChangeInTheFuture is not called and the polling continues until either a match is found or the timeout elapses.

which is reflected in the code:

gomega/internal/asyncassertion/async_assertion.go

Lines 98 to 100 in 1a3d249

	if assertion.actualInputIsAFunction() {
	return true
	}

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 the actual is a function or a value so that people can decide on their own?

Thanks in advance.

[blgm]

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.

[vespian]

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!

[blgm]

Thank you @vespian. I'll improve the docs.

[matthchr]

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.

[onsi]

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 < ***@***.***> 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 <#386 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEIJFXJPP4LGKDFHWYA4V3SLXZ7LANCNFSM4M64HLAQ> .

[matthchr]

@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.

[dlipovetsky]

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

[ramakrishnan-sundaramoorthy]

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

[pohly]

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.

[onsi]

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.

[pohly]

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"))

[onsi]

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.

[onsi]

yep - I had to do the errors.As dance. The code is written and tested and I'll cut a patch release later today.

[grosser]

issue can be closed @onsi ?

[grosser]

opened a new issue for "successful StopTring()" #786