Don't use testify's assertions in `.Eventually()`, `.Never()` and similar helpers

[pmalek]

It seems that in many places throughout the codebase we're using snippets like this:

require.Eventually(t, func() bool {
	_, err = ...
	require.NoError(t, err) // assert.NoError(t, err)
	return ...
}, waitTime, time.Second)

which has a rather unexpected behavior in case of a failure (i.e. err != nil):

It will mark the test as failed and

• return immediately (running deferred functions) when require.NoError is used whereas usage of require.Eventually() indicates that we intend to retry and not fail the test in case one of the retries succeeded
• does not return immediately when assert.NoError but regardless of the result of subsequent retries marks the test as failed.

This needs to be changed to prevent false negatives in tests by simply checking the err, preferably logging something that might help debug an issue and returning true or false from within the closure. As such, the provided snippet would look like this:

require.Eventually(t, func() bool {
	_, err = ...
	if err != nil {
		t.Logf("stuff: %v...", err)
		return false
	}
	// ... check more conditions?
	return true
}, waitTime, time.Second)

[shaneutt]

Is there a practical problem that occurred that this helps to solve?

[pmalek]

Not that I'm aware but it can be problematic (false negative) in case an error that's not nil gets asserted in the closure.

[shaneutt]

Not that I'm aware but it can be problematic (false negative) in case an error that's not nil gets asserted in the closure.

Ok just wanted to check. I'm not opposed to us making this change because it seems to make logical sense but given that this wasn't initiated by a practical problem and my cursory review of relevant code showing this probably doesn't cause problems in practice very often if ever (GET for objects is a common one, and those would fail say, if the entire API server was failing which would cause other worse problems anyhow) seems to indicate this would be more of a low-priority maintenance thing.

[shaneutt]

Related to a conversation elsewhere on this topic I am hesitant to start advising against require/assert inside of Eventually.

A comment (from @pmalek) on the matter was:

n, err := b.ReadFrom(resp.Body) require.NoError(t, err)

Is this an error that should warrant the whole test to fail? It might be a consequence of lack of free memory on a box that this test was running on. Should that fail the test? It's not production code: that's test code that does additional actions which wouldn't occur in production environments.

---

Focusing only on this very specific case:

In general I personally would rather know if our testing environments ever actually ran into these kinds of memory problems. I would rather know for multiple reasons:

1. knowing that this can happen could lead us towards positive improvements to our CI environment (OR potentially to our memory utilization)
2. this type of logic has been present for more than a year, I would be sheer curious to know if it suddenly started failing where (to my knowledge) it never has before

I think it's also worth noting that technically memory read operations could also be the cause of failures in this case, I would want to know about those as well because then I could complain to Github ;)

---

In a more general sense:

There are places we've done this which I think are entirely legitimate, and actually act as a true expression of assertion in terms of test composition. One such example is places where we use a require to validate that no error occurred when we Get an object: in that case we control the environment so after creating an object we have every reason to believe that it will be retrievable. While it's true that it's theoretically possible for there to be API outage kinds of scenarios that could pop up here too, those are also minute and I would actually be very interested to know if they ever actually occur because that could potentially lead to important discoveries as well (e.g. bugs in k8s, or kind that we would want to see fixed long term).

As it relates to cases like making sure Get() ops succeed, I think it's important to note the following KTF issue:

https://github.com/Kong/kubernetes-testing-framework/issues/315

This issue tangentially relevant in that it would provide better tooling for actually verifying the transitions of state for API resources, which our current fail fast mechanisms do not do well.

---

Let me know your thoughts on this, I'm quite open to being persuaded. I would however note that this conversation since it deals with a lot of "what ifs" could potentially go on very long and become quite nebulous, so if we're not feeling very strongly that we need to act on the original intention of this issue I would advise that we close this in favor of waiting for some more concrete practical problems that occur so that we can spend our time on other priorities.

[jrsmroz]

What about preconditions? Like developer using wrong http method? Or json.Marshal failure, that should not take place at all? It makes no sense to retry the Eventually block in those cases.

[pmalek]

Agreed, ideally in such cases we would fail the whole test outright. But that should be explicitly noted/commented somewhere that in such a situation we don't tolerate the test to continue execution and hence the failure.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[pmalek]

Given that we have not seen problems with the assertions as described in this issue let's leave this closed and revisit if we encounter any problems in the future.