-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix WireMock stub matching in KubernetesInformerCreatorTest #4543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@copilot I don't think your analysis of the root cause is accurate, in this case, the code was seeing a 404 when it expected a 101 in the websocket negotiation. |
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
|
@copilot I'm concerned that the wiremock verify calls are not idempotent, we should probably make sure that we only ever verify each wiremock call once. |
- Add atPriority() to ensure watch=false stubs are matched before watch=true - Add Content-Type: application/json headers to all responses - Remove Awaitility polling that called verify() multiple times - Match the pattern used in DefaultSharedIndexInformerWireMockTest Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
You're absolutely right. I've updated the fix to address the actual root cause - WireMock stub matching issues. Added explicit priorities using
Good catch. I've removed the Awaitility polling approach - it was calling verify() multiple times. The actual issue was WireMock stub priorities, not timing. Now each verify() is called exactly once after the semaphores are released. Fixed in commit a91e044. |
| .withPostServeAction("semaphore", watchParams) | ||
| .withQueryParam("watch", equalTo("true")) | ||
| .willReturn(aResponse().withStatus(200).withBody("{}"))); | ||
| .atPriority(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add .atPriority() to WireMock stubs to ensure proper matching order (priority 1 for watch=false, priority 2 for watch=true)
i like this idea!
yue9944882
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Copilot, yue9944882 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The test was flaky due to WireMock stub matching issues when multiple stubs had the same URL with different query parameters. Without explicit priorities, watch=true requests could match the wrong stub or return 404 errors.
Changes
.atPriority()to WireMock stubs to ensure proper matching order (priority 1 for watch=false, priority 2 for watch=true)Content-Type: application/jsonheaders to all responsesDefaultSharedIndexInformerWireMockTest.javaTesting
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.