/test

I personally feel the current implementation to be better aligned with the rest of the tests that also leverage testify helpers, but the proposed alternative looks reasonable to me. A few hunks are missing the actual check though, and would cause the test to incorrectly succeed also in case of failures.

I dont have personal preference if we should leverage testify with double negatives or proposed alternative. I opened this just becase I got the feedback from #44545 (comment) as this is called technical debt. I dont want to introduce the debt if possible from my PR. If this PR is just optional, I can just close it.

I personally feel the current implementation to be better aligned with the rest of the tests that also leverage testify helpers, but the proposed alternative looks reasonable to me. A few hunks are missing the actual check though, and would cause the test to incorrectly succeed also in case of failures.

I dont have personal preference if we should leverage testify with double negatives or proposed alternative. I opened this just becase I got the feedback from #44545 (comment) as this is called technical debt. I dont want to introduce the debt if possible from my PR. If this PR is just optional, I can just close it.

/cc @joestringer It would be good to get your opinion as well, as you commented on the original PR.

My intended feedback was effectively: "Don't assert that something is not negative". And if you're having trouble parsing that sentence, that's exactly my point ;). To put another way, prefer if assert.Error(...) {...} over if !assert.NoError(...) {...}.

The if assert.XXX(...) { ... } pattern seems pretty common, expected for users of testify, and is slightly more terse than if ... { assert.XXX(...) }. Either is fine but if we're already using testify this way, I'd suggest just keeping the current pattern to minimize diff.

The one other thing I'd add is that I have a mild preference for (a) / (b) over (c), just in order to minimize cognitive burden (one function call per line rather than multiple):
(a)

err := upsertBGPCC(ctx, f, clusterConfig)
if assert.Error(c, err) {
    // Expects an error, continues the loop. Iteration continues, test will succeed
    continue
}
// No error: execution continues, test will fail

(b)

if err := upsertBGPCC(ctx, f, clusterConfig); err != nil {
    // Error is completely normal, expected behavior. Iteration continues, test will succeed
    continue
}
// No error: Also normal and expected, test will succeed

(c)

if err := upsertBGPCC(ctx, f, clusterConfig); assert.Error(c, err) {
    continue
}

(Comments unnecessary, just including them for explanation here)

To put another way, prefer if assert.Error(...) {...} over if !assert.NoError(...) {...}

The two assertions are not equivalent, and lead to a completely different behavior though, as mentioned here.

Consider the following snippet, that mimics the different hunks touched by this PR. The get function mimics for instance a k8s client Get, which may return an error if the target object hasn't been created yet, or the object itself.

func TestError(t *testing.T) {
	var cnt = 0
	get := func() (*int, error) {
		if cnt = cnt + 1; cnt < 3 {
			return nil, errors.New("error")
		}
		return &cnt, nil
	}

	require.EventuallyWithT(t, func(c *assert.CollectT) {
		var get, err = get()
		if !assert.NoError(c, err) {
			return
		}

		assert.Equal(c, 3, *get)
	}, 10*time.Millisecond, 1*time.Millisecond)

	assert.Equal(t, 3, cnt, "[get] should have been called three times")
}

The if !assert.NoError(c, err) bit (a) checks that no error occurred, and (b) causes the condition function to return early if an error occurred, because the subsequent checks may otherwise panic due to the returned object being nil. Given that the assert.NoError assertion fails if an error occurred, require.EventuallyWithT will invoke the condition function again during the next interval, as expected.

Replacing it with if assert.Error(c, err) would instead check that (a) an error occurred, and (b) cause the condition function to return early if an error occurred. However, require.EventuallyWithT will at that point exist successfully if an error occurred (given that assert.Error succeeded), ignoring any subsequent check. At the same time, the check would never succeed if get doesn't return an error.

For instance, the following test (the only difference is the proposed change):

func TestError(t *testing.T) {
	var cnt = 0
	get := func() (*int, error) {
		if cnt = cnt + 1; cnt < 3 {
			return nil, errors.New("error")
		}
		return &cnt, nil
	}

	require.EventuallyWithT(t, func(c *assert.CollectT) {
		var get, err = get()
		if assert.Error(c, err) {
			return
		}

		assert.Equal(c, 3, *get)
	}, 10*time.Millisecond, 1*time.Millisecond)

	assert.Equal(t, 3, cnt, "[get] should have been called three times")
}

fails due to:

        	Error:      	Not equal: 
        	            	expected: 3
        	            	actual  : 1
        	Test:       	TestError
        	Messages:   	[get] should have been called three times

which shows that '[get]got only called once, asrequire.EventuallyWithTreturned immediately due to theassert.Error` check being true.

The correct alternative would be something along the lines of (as also partially done in this PR):

var get, err = get()
if err != nil {
	c.Errorf("Unexpected error")
	return
}

assert.Equal(c, 3, *get)

Which has the downside of mixing bare checks and testify, and leading to worse failure messages if not handled carefully (e.g., manually printing the actual error).

I see. This makes me think the double-negative pattern is even more problematic. What about just splitting the assert.NoError() onto a dedicated line without any conditional checks, then do a standard conditional check? I think this would be simpler to read and debug if it ever fails.

This makes me think the double-negative pattern is even more problematic.

IMO the correct way of reading it is along the lines of "if the given assertion failed, then return", which avoids any double-negative confusion.

What about just splitting the assert.NoError() onto a dedicated line without any conditional checks, then do a standard conditional check? I think this would be simpler to read and debug if it ever fails.

Sure, I'm not opposed if you feel that is easier to read.