Skip to content

[27.x backport] tests/run: fix flaky RunAttachTermination test - #5326

Merged
laurazard merged 1 commit into
docker:27.xfrom
vvoland:5303-27.x
Aug 7, 2024
Merged

[27.x backport] tests/run: fix flaky RunAttachTermination test#5326
laurazard merged 1 commit into
docker:27.xfrom
vvoland:5303-27.x

Conversation

@vvoland

Copy link
Copy Markdown
Collaborator

- What I did

This test was just incorrect (and testing incorrect behavior): it was checking that docker run exited with a context canceled error after signalling the CLI/cancelling the command's context, but this was incorrect (and was fixed in
991b130 - which was when this test started failing).

However, since this test assertion was happening inside of a goroutine, it would sometimes pass if it didn't get to run before the test suite terminated. It was flaky because sometimes the assertion inside the goroutine did get to execute, but after the test finished execution, which is a big no-no.

As an aside, assertions inside goroutines are generally bad, and goveteven has a check for this (but it only catches t.Fatal and t.FailNow calls and not assert.Xx).

- How I did it

Fixed RunAttachTermination to test for the correct behavior, and generally cleaned the tests up a bit. Also added another test for the general/not SIGINT'ed case.

- How to verify it

go test -v -count=1 -run=TestRunAttach ./cli/command/container/...

- Description for the changelog

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

This test was just incorrect (and testing incorrect
behavior): it was checking that `docker run` exited with a `context
canceled` error after signalling the CLI/cancelling the command's
context, but this was incorrect (and was fixed in
991b130 - which was when this test
started failing).
However, since this test assertion was happening inside of a goroutine,
it would sometimes pass if this assertion didn't get to run before the
test suite terminated. It was flaky because sometimes this assertion
inside the goroutine did get to execute, but after the test finished
execution, which is a big no-no.
As an aside, assertions inside goroutines are generally bad, and `govet`
even has a linter for this (but it only catches `t.Fatal` and `t.FailNow`
calls and not `assert.Xx`.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(cherry picked from commit eac8357)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.49%. Comparing base (1cf3637) to head (c7f3031).
Report is 1 commits behind head on 27.x.

Additional details and impacted files
@@ Coverage Diff @@## 27.x #5326 +/- ##
==========================================
- Coverage 61.50% 61.49% -0.02% 
==========================================
Files 299 299 Lines 20867 20867 ==========================================
- Hits 12835 12832 -3 - Misses 7116 7120 +4 + Partials 916 915 -1 

@laurazard

Copy link
Copy Markdown
Member

Ahh I forgot about backporting this, thanks.

@laurazard
laurazard merged commit 5761e66 into docker:27.xAug 7, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@vvoland@codecov-commenter@laurazard@austinvazquez