Fixed flaky TestGameServerWithPortsMappedToMultipleContainers #1458
Conversation
|
Build Succeeded 👏 Build Id: 0996fc2a-1b0a-4e87-8ed4-603949296d42 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 6c6dd98a-6a7c-4d36-96f3-6306c2196928 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
roberthbailey
requested review from
markmandel
and removed request for
roberthbailey and
dzlier-gcp
test/e2e/gameserver_test.go
Outdated
|
|
||
| go func() { | ||
| expectedMsg := "Ping 1" | ||
| errPoll := wait.PollImmediate(interval, timeOut, func() (done bool, err error) { |
There was a problem hiding this comment.
Love the polling - seems like a good solution!
test/e2e/gameserver_test.go
Outdated
| pollCh1 := make(chan error) | ||
| pollCh2 := make(chan error) | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
Is there a reason this couldn't be 2 polling statements, one after another? (i.e. don't run it in goroutines)
Then it would make this test much simpler, and remove the need for channels. I don't think we have to test for both UDP endpoints at the same time.
WDYT?
There was a problem hiding this comment.
The main idea why I've introduced 2 goroutines is that in a worst-case scenario if we ping ports one by one, this single test will run 2 mins at least - there are 2 timeouts, 60 sec for each ping. According to my tests, there is a little chance of this scenario, but I decided to take it into account. I've tried to make this code as simple as possible.
Do you think we need to call polling statements one after another anyway?
There was a problem hiding this comment.
Since failure should be an exception not the regular path, I would have them one after the other, for sake of simplicity of the testing code. This would make it easier to maintain going forward.
One optimisation you could make that would cut things down time in failure scenarios, is that if the first one fails, do a assert.FailNow(); to fast fail the test and stop the second one from even starting.
There was a problem hiding this comment.
Ok, good suggestion!
|
@markmandel Applied your review notes. |
|
Build Succeeded 👏 Build Id: a4627933-f8f5-4ccd-a7c3-cd01a91ac0b9 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
test/e2e/gameserver_test.go
Outdated
| return err == nil && strings.Contains(res, expectedMsg2), nil | ||
| }) | ||
|
|
||
| assert.Nil(t, errPoll, "expected no errors after polling a port: %s", secondPort) |
There was a problem hiding this comment.
| assert.Nil(t, errPoll, "expected no errors after polling a port: %s", secondPort) | |
| assert.NoError(t, errPoll, "expected no errors after polling a port: %s", secondPort) |
Just displays the details of the passed in error message. Other than that, this looks good to go 👍
|
Build Failed 😱 Build Id: 8845fad4-d53d-4c02-a0db-45bb77a62017 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: b8054085-b471-4c77-b955-6d763f2f3b42 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
fix fix2 fix Minor fix
|
Build Succeeded 👏 Build Id: c054af4a-aa0e-44fe-9f91-a209731d821e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
markmandel
added
area/tests
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akremsa, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes #1450
I've run this test more than 100 times and found that both pings can fail.
Fix proof:
Game port №1:
Game port №2:
Suggestion: Is there any opportunity to stop tests immediately by pressing ctr+c/ctr+z if we run them inside a container? It can take some time for some goroutines to finish their work.