improves e2e test reliability with client handling and retry helpers #3058
Conversation
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3058 +/- ##
=======================================
Coverage 56.51% 56.51%
=======================================
Files 334 334
Lines 33170 33170
=======================================
Hits 18747 18747
Misses 12839 12839
Partials 1584 1584 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/XL
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
ChrisJBurns
force-pushed
the
fixes-e2e-tests
branch
from
9da5c68 to
a778399
Compare
github-actions
bot
added
size/M
ChrisJBurns
changed the title
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
github-actions
bot
added
size/M
github-actions
bot
added
size/M
github-actions
bot
added
size/M
| func CreateAuthenticatedMCPClientWithRetry( | ||
| nodePort int32, | ||
| clientName string, | ||
| httpClient *http.Client, | ||
| timeout time.Duration, | ||
| pollingInterval time.Duration, | ||
| ) (*InitializedMCPClient, error) { | ||
| return CreateInitializedMCPClientWithRetry( | ||
| nodePort, | ||
| clientName, | ||
| timeout, | ||
| pollingInterval, | ||
| transport.WithHTTPBasicClient(httpClient), |
There was a problem hiding this comment.
Nit, feel free to ignore: I would delete this and have the caller create the option transport.WithHTTPBasicClient(httpClient),.
| } | ||
|
|
||
| It("should list and call tools from all backends with discovered auth", func() { | ||
| It("should list and call tools from all backends with discovered auth", FlakeAttempts(2), func() { |
There was a problem hiding this comment.
I would strongly prefer not introducing FlakeAttempts because it will prolong CI and normalize handling flakiness with more retries. If this pattern proliferates, we'll have a lot of low value, high cost tests.
There was a problem hiding this comment.
Oops, good spot, I forgot to take these out when the other changes were added 😅
|
needs a rebase atop @jerm-dro 's PR |
Improves e2e test reliability by fixing MCP client handling and adding robust retry helpers.
Key changes:
Start()orInitialize(). AddedCreateInitializedMCPClientWithRetryandCreateAuthenticatedMCPClientWithRetryhelpers that create a fresh client on each retry attempt instead of reusing a failed client.time.SleepwithWaitForHealthy: Polls the/healthendpoint to verify server readiness instead of arbitrary sleep delays.WaitForToolsDiscoveredandWaitForSpecificToolsDiscoveredensure backends are fully connected before running tool-related assertions.FlakeAttempts(2)to the auth discovery test for additional resilience.