Skip to content

test: fix flaky TestConnectAndWait/connect_goroutine_exits_after_EOF #5927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

test: fix flaky TestConnectAndWait/connect_goroutine_exits_after_EOF #5927

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions cli-plugins/socket/socket_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"io/fs"
"net"
"os"
"runtime"
"strings"
"sync/atomic"
"testing"
Expand Down Expand Up @@ -175,27 +174,25 @@ func TestConnectAndWait(t *testing.T) {
}
})

// TODO: this test cannot be executed with `t.Parallel()`, due to
// relying on goroutine numbers to ensure correct behaviour
t.Run("connect goroutine exits after EOF", func(t *testing.T) {
srv, err := NewPluginServer(nil)
assert.NilError(t, err, "failed to setup server")

defer srv.Close()

t.Setenv(EnvKey, srv.Addr().String())
numGoroutines := runtime.NumGoroutine()

ConnectAndWait(func() {})
assert.Equal(t, runtime.NumGoroutine(), numGoroutines+1)
ch := make(chan struct{})
ConnectAndWait(func() {
close(ch)
})

srv.Close()

poll.WaitOn(t, func(t poll.LogT) poll.Result {
if runtime.NumGoroutine() > numGoroutines+1 {
return poll.Continue("waiting for connect goroutine to exit")
}
return poll.Success()
}, poll.WithDelay(1*time.Millisecond), poll.WithTimeout(10*time.Millisecond))
Copy link
Collaborator

@vvoland vvoland Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we specifically wanted to assert that goroutine is stopped.

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

Copy link
Member Author

@Benehiko Benehiko Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@Benehiko Benehiko Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@vvoland vvoland Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@vvoland vvoland Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goroutine will stop once the callback function returns.

Unless there's a bug in ConnectAndWait 🙈

Copy link
Member Author

@Benehiko Benehiko Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@vvoland vvoland Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@vvoland vvoland Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened: #5932

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

Copy link
Member Author

@Benehiko Benehiko Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@vvoland vvoland Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

select {
case <-ch:
case <-time.After(1 * time.Second):
t.Fatal("callback not called after 1s")
}
})
}
Loading