test: fix flaky TestRunAttachTermination#5926
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5926 +/- ##
==========================================
- Coverage 59.33% 59.28% -0.05%
==========================================
Files 358 358
Lines 29783 29830 +47
==========================================
+ Hits 17672 17685 +13
- Misses 11142 11173 +31
- Partials 969 972 +3 🚀 New features to boost your workflow:
|
Benehiko
force-pushed
the
fix-attach-test-flake
branch
from
4aecba8 to
ed8df97
Compare
|
@Benehiko could you add something similar to the PR's description in the commit message so that the commit message describes the changes made? |
This patch fixes the `TestRunAttachTermination` flaky runs. It seems like we weren't halting on the `waitFunc` so if the process was fast enough to setup the signal handler and execute `waitExitOrRemoved`. We now instead wait for the `killCh` channel to close inside the mocked `waitFunc`. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Benehiko
force-pushed
the
fix-attach-test-flake
branch
from
ed8df97 to
966b441
Compare
|
@thaJeztah PTAL :) |
|
|
||
| assert.NilError(t, syscall.Kill(syscall.Getpid(), syscall.SIGINT)) | ||
| // end stream from "container" so that we'll detach | ||
| conn.Close() |
There was a problem hiding this comment.
What was the reason for removing conn.Close? @Benehiko
There was a problem hiding this comment.
it is already closed when the test is cleaned up. https://github.com/docker/cli/pull/5926/files/966b44183f525a8cca6caeaff2dc5b3f156fd06e#diff-ca68c02755e22b34c98b7f2ce9f70fd31bd8c098d647a29f318df6b3fe2633c1L166-L168
There was a problem hiding this comment.
Is it still being flaky? From my testing closing the connection here didn't add anything.
There was a problem hiding this comment.
See: #5957
It's needed for the cmdErrC to actually receive the event. Removing it depended on the bug that was introduced.
I'm wondering if the flakiness was actually caused by the bug fixed by the mentioned PR 🤔
5 participants
Fixes
TestRunAttachTerminationflaky runs. It seems like we weren't halting on thewaitFuncso if the process was fast enough to setup the signal handler and executewaitExitOrRemoved.https://github.com/docker/cli/blob/master/cli/command/container/run.go#L199
- What I did
Added a line waiting for the
killChchannel to close inside the mockedwaitFunc.- How I did it
- How to verify it
go test -race -run=TestRunAttachTermination -count=1000 -v -failfast ./cli/command/container/...- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)