Fixing flaky Health_Connect test

[marcin-krystianc]

Health_Connect failed from time to time,
It seems that the "connect" related information is not immediately updated. Hence we need to refer to blocking queries (i.e. set the waitindex >0).

[IvanKolchanov]

@marcin-krystianc
Everything looks great, I just have two questions/suggestions.
First, the original Go code was setting up the service through a proxy and checking that only the proxy will show up in the result and that the port will be correct, which is not exactly crucial, and I am not completely sure what their intention was, but might be a thing we want to do/test too, as they probably put it in for a reason. I had the same approach when doing Ingress() method.
https://github.com/hashicorp/consul/blob/v1.9.17/api/health_test.go#L488

Second, couldn't the while loop theoretically be running forever if server gets silly for some reason? Should we put some kind of fail check for that, a timeout or something. Or maybe just not use a while loop, but make a normal (not blocking query request) and if it fails just make another one, but a blocking query, using the LastIndex we got. That should probably be enough, and if the information is not coming in after 5 minutes of default blocking query, then something is definetely wrong.

I might be misunderstanding/missing something there, feel free to correct me! :)

[marcin-krystianc]

@IvanKolchanov
Thanks for taking a look.
I've updated the test assertions, so we also check that there is only a single response and that the port number is correct.

Regarding the possibility of falling into an infinite loop, I don't think it is possible with the current code.
We either make progress (wait index increments Assert.True(checks.LastIndex > q.WaitIndex);) or we fail due to a timeout (new CancellationTokenSource(TimeSpan.FromMinutes(1))).

With your suggestion:

Or maybe just not use a while loop, but make a normal (not blocking query request), and if it fails just make another one, but a blocking query, using the LastIndex we got.

We could still have a flaky test, as it is very unlikely (but still possible) that the other tests running concurrently will change the state of the consul server, and the blocking query will return, although the information we are interested in is still not there.
Hence repeating the blocking query in a while loop seems necessary.

[IvanKolchanov]

@IvanKolchanov Thanks for taking a look. I've updated the test assertions, so we also check that there is only a single response and that the port number is correct.

Regarding the possibility of falling into an infinite loop, I don't think it is possible with the current code. We either make progress (wait index increments Assert.True(checks.LastIndex > q.WaitIndex);) or we fail due to a timeout (new CancellationTokenSource(TimeSpan.FromMinutes(1))).

With your suggestion:

Or maybe just not use a while loop, but make a normal (not blocking query request), and if it fails just make another one, but a blocking query, using the LastIndex we got.

We could still have a flaky test, as it is very unlikely (but still possible) that the other tests running concurrently will change the state of the consul server, and the blocking query will return, although the information we are interested in is still not there. Hence repeating the blocking query in a while loop seems necessary.

I kinda missed that the timeout in cancellation token would cause the test to fail, so yep, it makes sense to me now!
Glad I was able to help!