test: fix flaky TestConnectAndWait/connect_goroutine_exits_after_EOF

[Benehiko]

Closes #5837

- What I did
The test currently measures the reduction in the number of goroutines, which is strange as there is a callback function called by ConnectAndWait once the io.EOF is reached.

- How I did it

- How to verify it

go test -race -run=TestConnectAndWait/connect_goroutine_exits_after_EOF -count=1000 -v -failfast ./cli-plugins/socket/...

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.32%. Comparing base (2d74733) to head (ce9a64c).
Report is 16 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5927      +/-   ##
==========================================
- Coverage   59.33%   59.32%   -0.02%     
==========================================
  Files         358      358              
  Lines       29783    29783              
==========================================
- Hits        17672    17669       -3     
- Misses      11142    11145       +3     
  Partials      969      969              

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vvoland]

IIRC we specifically wanted to assert that goroutine is stopped.

Wondering if just increasing the timeout would help with the flakiness?

[Benehiko]

i honestly don't know why we would want to do that. The goroutine will stop once the callback function returns.

[Benehiko]

also the failure is not always on the poll, it's on the first assertion on line 190 https://github.com/docker/cli/pull/5927/files/ce9a64c8f4524e6f339f48b1892374c0a5e1580e#diff-2b3ea721470acb354ca37a32dc40b7c0ae55bd43397a084adbd922fca5964e20L190

[vvoland]

Yeah in this case I think we should also have a poll.WaitOn instead of a straight assert (because the goroutine may not be spawned immediately).

[vvoland]

The goroutine will stop once the callback function returns.

Unless there's a bug in ConnectAndWait 🙈

[Benehiko]

if the code is changed and the implementation is bad, then there is no guarantee that a test would catch the regression anyway. Focusing on a hypothetical case won't solve the fact that this test fails multiple times per day.

Either we re-design the ConnectAndWait to be more resilient or we change the test as I've done here. We cannot predict when a goroutine will run or when it will exit. The changes I make here are more robust from the test perspective.

[vvoland]

But it no longer tests what the test claims to: connect goroutine exits after EOF.

Maybe it's not possible to reliably test that, okay, but let's try to fix the actual test before actually changing it into completely different test.

[vvoland]

Opened: #5932

If it's still flaky after that, I'm 100% agreeing on changing this test according to your proposal :)

[Benehiko]

But it no longer tests what the test claims to: connect goroutine exits after EOF.

That's not true, it still tests that the goroutine exits on EOF.

The callback is only called once we receive an EOF here:

cli/cli-plugins/socket/socket.go

Lines 163 to 166 in ce9a64c

	if errors.Is(err, io.EOF) {
	cb()
	return
	}

After the callback is called the goroutine will exit due to the return.

[vvoland]

No, it tests that the callback gets called after an EOF.

After the callback is called the goroutine will exit due to the return.

Yes, that's true with the current code, but tests are also there to detect an unwanted change in future. If this function gets changed in future and introduces a bug where the callback gets called, but there's no return out of the goroutine then this test would be able to detect it.

[Benehiko]

In favor of #5932