Skip to content

test: migrate test api error not found json#51216

Merged
thaJeztah merged 1 commit intomoby:masterfrom
sameergupta4873:50159-migrate-test-api-error-not-found-json
Oct 21, 2025
Merged

test: migrate test api error not found json#51216
thaJeztah merged 1 commit intomoby:masterfrom
sameergupta4873:50159-migrate-test-api-error-not-found-json

Conversation

@sameergupta4873
Copy link
Contributor

@sameergupta4873 sameergupta4873 commented Oct 18, 2025

- What I did
Migrated the TestAPIErrorNotFoundJSON to integration tests in reference to the integration-cli migration epic #50159

- How I did it
Using test on integration-cli/docker_api_test.go

- How to verify it
Run the integration test

- Human readable description for the release notes

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

@sameergupta4873 sameergupta4873 force-pushed the 50159-migrate-test-api-error-not-found-json branch from f6889bb to c9778cc Compare October 18, 2025 17:39
Comment on lines +12 to +13
// getErrorMessage returns the error message from an error API response
func getErrorMessage(t *testing.T, body []byte) string {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should consider adding a small function to the request test-utility to help with decoding JSON responses (check status code, content-type, then unmarshal into a variable that's passed. 🤔

Doesn't have to be for this PR, but I could have a quick peek

@sameergupta4873 sameergupta4873 marked this pull request as ready for review October 20, 2025 06:28
@sameergupta4873
Copy link
Contributor Author

Thanks for the review, @thaJeztah!
Yes, that makes sense—having a small helper in request for decoding JSON responses would definitely simplify similar tests.

@robmry
Copy link
Contributor

robmry commented Oct 20, 2025

Both add the same getErrorMessage, and will need the same updates to use the ReadJSONResponse helper.

@thaJeztah
Copy link
Member

@sameergupta4873 #51220 was merged, so you can rebase this PR (and use the new utility) 👍

@sameergupta4873 sameergupta4873 force-pushed the 50159-migrate-test-api-error-not-found-json branch 3 times, most recently from e858aab to 0e6868d Compare October 21, 2025 08:24
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah added this to the 29.0.0 milestone Oct 21, 2025
@thaJeztah thaJeztah added status/4-merge kind/refactor PR's that refactor, or clean-up code and removed area/dependencies labels Oct 21, 2025
@sameergupta4873 sameergupta4873 force-pushed the 50159-migrate-test-api-error-not-found-json branch from 0e6868d to 084f9b8 Compare October 21, 2025 08:41
@sameergupta4873
Copy link
Contributor Author

Hi @thaJeztah, I’ve the correct usage of ctx in TestAPIErrorNotFoundJSON. All changes are now fixed. Could you please approve again? Thanks!

Signed-off-by: Sameer Gupta <sameergupta4873@gmail.com>
@sameergupta4873 sameergupta4873 force-pushed the 50159-migrate-test-api-error-not-found-json branch from 084f9b8 to 62a71a8 Compare October 21, 2025 08:48
@thaJeztah
Copy link
Member

oh! good catch; missed that!

still LGTM ; I gave CI a kick 👍

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

All green; let's merge! Thank you for contributing!

@thaJeztah thaJeztah merged commit 1ff6a96 into moby:master Oct 21, 2025
184 of 195 checks passed
Sign up for free to 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.

3 participants