Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(tests): flaky test in BucketFallbackMapper tests #104

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

volmedo
Copy link
Member

@volmedo volmedo commented Feb 1, 2025

Fixes #103

The original implementation launched an HTTP server for the test concurrently in a different goroutine. This poses a race condition between starting the server and running the tests. Sometimes, the test was run before the server was ready and the test failed.

Finding the cause has been tricky because, in case of an error in the HEAD request, BucketFallbackMapper swallows the original error and returns ErrKeyNotFound, masking the real cause of the issue. This error handling has been improved to avoid this.

To fix the test, the regular HTTP server being used has been replaced with an httptest.Server. It simplifies the test code as handling its shutdown is easier, but the reason it fixes the issue is that httptest.Server prepares the network listener synchronously, and then starts the server concurrently. That means that, even in the case the test is run before the server is ready to server requests, the listener will be able to buffer them and prevent the test from failing.

@volmedo volmedo self-assigned this Feb 1, 2025
@volmedo volmedo mentioned this pull request Feb 1, 2025
Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/aws/bucketfallbackmapper.go 25.00% 2 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
pkg/aws/bucketfallbackmapper.go 76.92% <25.00%> (-4.71%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM - and less code! I wonder why there was a new server for every test case before? I assume it's fine that there's just one?

@volmedo
Copy link
Member Author

volmedo commented Feb 3, 2025

yeah, I guess each test case having its own server aligns with the guideline of isolating the tests as much as possible, but the handler is always the same, so I think it's ok in this case.

@volmedo volmedo merged commit 0433f4b into main Feb 3, 2025
8 of 9 checks passed
@volmedo volmedo deleted the vic/fix/flaky-test branch February 3, 2025 11:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestBucketFallbackMapper/200_status_code_on_head_generates_claim is flaky
2 participants