Flaky Test: TestGameServerAllocationReturnLabels#4435
Flaky Test: TestGameServerAllocationReturnLabels#4435Sivasankaran25 wants to merge 12 commits intogoogleforgames:mainfrom
Conversation
|
Build Succeeded 🥳 Build Id: a765df12-e1ab-4f81-82d0-68838557c782 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 Failed 😭 Build Id: 07ca1545-b7cb-4a5c-b16f-477889d0e21a Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 🥳 Build Id: 91fde476-2248-4b2f-93e8-d90926e17378 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
left a comment
markmandel
left a comment
There was a problem hiding this comment.
I think this is heading in the right direction, but isn't quite there yet.
| require.Equal(t, allocationv1.GameServerAllocationAllocated, gsa.Status.State) | ||
| require.NotEmpty(t, gsa.Status.GameServerName) | ||
|
|
||
| require.Eventually(t, func() bool { |
There was a problem hiding this comment.
I recently found https://pkg.go.dev/github.com/stretchr/testify@v1.11.1/require#EventuallyWithT - which for more complicated test conditions is definitely a better option for sure.
Then we can maintain the checks for labels and annotations, and have them reported in a way that's easier to read.
| gsa, err = framework.AgonesClient. | ||
| AllocationV1(). | ||
| GameServerAllocations(framework.Namespace). | ||
| Create(ctx, gsa.DeepCopy(), metav1.CreateOptions{}) |
There was a problem hiding this comment.
Does this fix the flake where this comes back with an Unallocated state? Should the eventually be wrapping the allocation call?
|
Build Succeeded 🥳 Build Id: a8926610-4191-4114-9f63-e09b02fa3b2c 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: 6eeb1759-c8c0-462d-9e56-621621c7e542 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: |
| if !assert.NoError(c, err, "API call to allocate should not fail") { | ||
| return | ||
| } |
There was a problem hiding this comment.
| if !assert.NoError(c, err, "API call to allocate should not fail") { | |
| return | |
| } | |
| require.NoError(c, err, "API call to allocate should not fail") |
No reason for the if conditional if you want to fail fast - that's what requires is for.
Same for all the others below.
| return | ||
| } | ||
|
|
||
| assert.Equal(t, allocationv1.GameServerAllocationAllocated, gsa.Status.State) |
There was a problem hiding this comment.
we lost all these assertions on the GameServerAllocation -- which is the point of the test, to make sure that labels and annotations are passed back to the allocation. These should still be in there.
|
Build Succeeded 🥳 Build Id: a20f2fe5-3d7b-4fb1-85e0-a2caa641a1f2 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: |
| require.NoError(t, err) | ||
| assert.Equal(t, flt.ObjectMeta.Name, gs.ObjectMeta.Labels[agonesv1.FleetNameLabel]) | ||
| require.NoError(c, err, "API call to allocate should not fail") | ||
| require.Equal(c, allocationv1.GameServerAllocationAllocated, currGsa.Status.State, "Allocation should be 'Allocated' state") |
There was a problem hiding this comment.
The original code asserted against gsa.Status.Metadata (the allocation's returned metadata):
assert.Equal(t, t.Name(), gsa.Status.Metadata.Labels[role])
assert.Equal(t, flt.ObjectMeta.Name, gsa.Status.Metadata.Labels[agonesv1.FleetNameLabel])
assert.Equal(t, annotationValue, gsa.Status.Metadata.Annotations[annotationKey])The new code instead checks gs.ObjectMeta (the underlying GameServer's own metadata):
require.Equal(c, t.Name(), gs.ObjectMeta.Labels[role], ...)
require.Equal(c, flt.ObjectMeta.Name, gs.ObjectMeta.Labels[agonesv1.FleetNameLabel], ...)
require.Equal(c, annotationValue, gs.ObjectMeta.Annotations[annotationKey], ...)The point of the test is to verify that labels/annotations are returned in the allocation response (Status.Metadata), not just that they exist on the GameServer itself.
Those are subtly different things. The assertions on currGsa.Status.Metadata are still missing
|
Build Failed 😭 Build Id: 9744db1a-eee2-472b-87c6-0057f2c584bc Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 🥳 Build Id: 91a38dc9-a917-47a9-b96e-f6d981512459 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: |
3 participants
What type of PR is this?
/kind bug
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #4431
Special notes for your reviewer: