Skip to content

Fix flaky scale test#11506

Closed
jhrotko wants to merge 1 commit into
docker:mainfrom
jhrotko:fix-flaky-test
Closed

Fix flaky scale test#11506
jhrotko wants to merge 1 commit into
docker:mainfrom
jhrotko:fix-flaky-test

Conversation

@jhrotko

@jhrotko jhrotko commented Feb 15, 2024

Copy link
Copy Markdown
Contributor

Problem

In compose there is one flaky test related to scale command. We are removing the latest created container when scaling down

sort.Slice(containers, func(i, j int) bool {
return containers[i].Created < containers[j].Created
})

And in the test, might not necessarily be the 3rd service (front-3)
First we scale up two front services, and we do not know for sure which of front-2 or front-3 is going to be created first.

t.Log("scale up 2 services")
res = c.RunDockerComposeCmd(t, "--project-directory", "fixtures/scale", "scale", "front=3", "back=2")
out = res.Combined()
checkServiceContainer(t, out, "scale-basic-tests-front", "Running", 2)
checkServiceContainer(t, out, "scale-basic-tests-front", "Started", 1)
checkServiceContainer(t, out, "scale-basic-tests-back", "Running", 1)
checkServiceContainer(t, out, "scale-basic-tests-back", "Started", 1)

t.Log("scale down 2 services")
res = c.RunDockerComposeCmd(t, "--project-directory", "fixtures/scale", "scale", "front=2", "back=1")
out = res.Combined()
checkServiceContainer(t, out, "scale-basic-tests-front", "Running", 2)
assert.Check(t, !strings.Contains(out, "Container scale-basic-tests-front-3 Running"), res.Combined())

Running locally the commands this is a possible state:
image

Running scale_test.go several times it's possible to get failures locally.
The proposed fix, keeps track of the containers creation order, and asserts if the last created container is the one removed, instead of expecting scale-basic-tests-front-3 to be the one removed when scaling down.
Note:
in Go there is no lookahead forward/behind, so the regex rule had to be more hacky.

Related issue
#11473

Work Item
https://docker.atlassian.net/browse/COMP-435

(not mandatory) A picture of a cute animal, if possible in relation to what you did
image

@jhrotko jhrotko marked this pull request as ready for review February 15, 2024 15:59
@ndeloof

ndeloof commented Feb 15, 2024

Copy link
Copy Markdown
Contributor

might relate to #11473

@jhrotko

jhrotko commented Feb 16, 2024

Copy link
Copy Markdown
Contributor Author

/retest-failures

Signed-off-by: jhrotko <joana.hrotko@docker.com>
@codecov

codecov Bot commented Feb 16, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (de3da82) 58.23% compared to head (d714a2e) 58.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11506      +/-   ##
==========================================
+ Coverage   58.23%   58.34%   +0.11%     
==========================================
  Files         134      134              
  Lines       11546    11546              
==========================================
+ Hits         6724     6737      +13     
+ Misses       4156     4147       -9     
+ Partials      666      662       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jhrotko jhrotko self-assigned this Feb 16, 2024
@laurazard

Copy link
Copy Markdown
Member

So I don't have full context, but I was looking at

func TestScaleUpAndDownPreserveContainerNumber(t *testing.T) {
and #11473 and I got the idea that when scaling down, under normal circumstances we shouldn't fall back to using created time and should be using the container number, so this would be deterministic.

Did I get this wrong, or did we do something wrong with #11473 and we're falling back to creation date?

@milas milas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No concern with the actual code changes but same question as @laurazard - shouldn't the fix from #11473 mean this is not necessary anymore?

@jhrotko jhrotko closed this Feb 20, 2024
@jhrotko

jhrotko commented Feb 20, 2024

Copy link
Copy Markdown
Contributor Author

@laurazard @milas I retested with @ndeloof and it seems it is not flaky anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants