Skip to content

test: improve TestMultiClusterAllocationFromLocal flakiness.#4350

Merged
markmandel merged 2 commits intoagones-dev:mainfrom
markmandel:flaky/TestMultiClusterAllocationFromLocal
Nov 27, 2025
Merged

test: improve TestMultiClusterAllocationFromLocal flakiness.#4350
markmandel merged 2 commits intoagones-dev:mainfrom
markmandel:flaky/TestMultiClusterAllocationFromLocal

Conversation

@markmandel
Copy link
Copy Markdown
Collaborator

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Not sure that I fixed it, but made some improvements and added some extra logging as well:

  • Change assert.NoError to require.NoError to stop test execution on allocation failure
  • Add error wrapping with response body context in executeAllocation helper
  • Fix createRequest to check error before setting headers
  • Add Accept header to allocation requests

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

N/A

Not sure that I fixed it, but made some improvements and added some
extra logging as well:

- Change assert.NoError to require.NoError to stop test execution on
  allocation failure
- Add error wrapping with response body context in executeAllocation
  helper
- Fix createRequest to check error before setting headers
- Add Accept header to allocation requests
@markmandel markmandel added the area/tests Unit tests, e2e tests, anything to make sure things don't break label Nov 26, 2025
@github-actions github-actions bot added kind/cleanup Refactoring code, fixing up documentation, etc size/S labels Nov 26, 2025
@agones-bot
Copy link
Copy Markdown
Collaborator

Build Succeeded 🥳

Build Id: 0a26512b-9379-434a-a064-67e8d9526c80

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:

git fetch https://github.com/googleforgames/agones.git pull/4350/head:pr_4350 && git checkout pr_4350
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.54.0-dev-05d72bf

Copy link
Copy Markdown
Collaborator

@vicentefb vicentefb left a comment

Choose a reason for hiding this comment

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

lgtm

}

r.Header.Set("Content-Type", k8sruntime.ContentTypeJSON)
r.Header.Set("Accept", k8sruntime.ContentTypeJSON)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure we want this? Either the response is always json (even errors), and we never hit the err = json.Unmarshal(jsn, ret) on line 783, or we expect errors to be in a non-json format like a plain text stack trace, so we want those errors to fall through and get caught and wrapped and returned.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not sure we want this? Either the response is always json (even errors), and we never hit the err = json.Unmarshal(jsn, ret) on line 783

It'll still fail, since the format of the error json won't match the struct - and even if it did go through, the result would end up not aligning with the expected result.

The flakiness may be a result of the underlying K8s system thinking sometimes we don't want JSON -- so this is just to make sure that's not the actual issue.

@agones-bot
Copy link
Copy Markdown
Collaborator

Build Succeeded 🥳

Build Id: ec51208d-5c40-48d6-b8ca-088a95f806da

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:

git fetch https://github.com/googleforgames/agones.git pull/4350/head:pr_4350 && git checkout pr_4350
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.54.0-dev-1aef956

@markmandel markmandel merged commit ff79d20 into agones-dev:main Nov 27, 2025
4 checks passed
@markmandel markmandel deleted the flaky/TestMultiClusterAllocationFromLocal branch November 27, 2025 14:52
mnthe pushed a commit to mnthe/agones that referenced this pull request Mar 23, 2026
…dev#4350)

Not sure that I fixed it, but made some improvements and added some
extra logging as well:

- Change assert.NoError to require.NoError to stop test execution on
  allocation failure
- Add error wrapping with response body context in executeAllocation
  helper
- Fix createRequest to check error before setting headers
- Add Accept header to allocation requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants