Skip to content

improve container-client tests#6217

Merged
anonrig merged 4 commits intomainfrom
yagiz/improve-container-client
Mar 2, 2026
Merged

improve container-client tests#6217
anonrig merged 4 commits intomainfrom
yagiz/improve-container-client

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Mar 2, 2026

Extracted from #4371

@anonrig anonrig requested a review from gabivlj March 2, 2026 15:04
@anonrig anonrig requested review from a team as code owners March 2, 2026 15:04
@anonrig anonrig enabled auto-merge March 2, 2026 15:05
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

Minor test improvements (safer shell commands, Promise.withResolvers, timeout for WebSocket test). One concern about removing the monitor() call.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 2, 2026

I'm Bonk, and I've done a quick review of your PR.

This PR improves container-client tests by: making the Docker cleanup command safer (pipe to xargs -r instead of subshell), fixing the test target name in the README, removing noisy retry logs, modernizing to Promise.withResolvers, and adding a 5-second timeout to the WebSocket message wait.

Issues found (1):

  1. Missing monitor() call after start() in testInterceptWebSocket — The PR removes container.monitor().finally(...) entirely, but the file header explicitly warns that monitor() must be called after start() to avoid losing track of container lifetime. Every other test method follows this pattern. Posted a suggestion to keep the monitor() call while dropping the .finally() log.

github run

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Looks good!

@anonrig anonrig disabled auto-merge March 2, 2026 15:09
@anonrig anonrig enabled auto-merge (squash) March 2, 2026 15:09
@anonrig anonrig merged commit 5722c9f into main Mar 2, 2026
34 of 37 checks passed
@anonrig anonrig deleted the yagiz/improve-container-client branch March 2, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants