Skip to content

test: stabilize proxy tests#1762

Open
jpnurmi wants to merge 3 commits into
masterfrom
jpnurmi/test/wait-for-proxy
Open

test: stabilize proxy tests#1762
jpnurmi wants to merge 3 commits into
masterfrom
jpnurmi/test/wait-for-proxy

Conversation

@jpnurmi
Copy link
Copy Markdown
Collaborator

@jpnurmi jpnurmi commented May 27, 2026

Proxy-related tests have been failing quite often lately

Replace the hardcoded 0.5s sleep with a wait loop that gives mitmdump up to 10s to get a response from the mock server. This should help in busy CI environments where runners may be under load and processes can be delayed.

@jpnurmi jpnurmi force-pushed the jpnurmi/test/wait-for-proxy branch from 4e5b9f3 to 95b285f Compare May 27, 2026 16:05
Comment thread tests/proxy.py Outdated
Comment thread tests/proxy.py
Comment on lines +146 to +153
if expected_proxy_logsize != 0:
# request passed through successfully
wait_for_stdout(
proxy_process,
lambda text: "POST" in text and "200 OK" in text,
timeout,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The wait_for_stdout call in proxy_test_finally hardcodes a check for "200 OK", causing tests that expect non-200 responses (like 407) to time out.
Severity: HIGH

Suggested Fix

The wait_for_stdout predicate should be made more flexible. Instead of hardcoding a check for "200 OK", it could be parameterized or adjusted to handle different expected outcomes, such as successful requests or failed authentication. One option is to pass the expected status string or a custom predicate function into proxy_test_finally.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: tests/proxy.py#L146-L153

Potential issue: The new logic in `proxy_test_finally` waits for "POST" and "200 OK" in
the proxy's stdout when `expected_proxy_logsize` is not zero. However, the
`test_proxy_auth` test case expects a "407 Proxy Authentication Required" response, not
a "200 OK". Because the predicate in `wait_for_stdout` can never be satisfied in this
scenario, the test will wait for the full timeout period and then fail with a
`TimeoutError`. This makes the test helper inflexible for scenarios involving
non-successful HTTP responses.

Also affects:

  • tests/proxy.py:110~110

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bc69cde. Configure here.

Comment thread tests/proxy.py
proxy_process.terminate()
stdout_bytes, _ = proxy_process.communicate()
stdout = stdout_bytes.decode("utf-8", errors="replace")
proxy_process.wait(timeout=timeout)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Proxy process leaks when pre-termination assertions fail

Medium Severity

The assert wait_for(...) on line 142 and wait_for_stdout(...) on line 148 can both raise exceptions (AssertionError and TimeoutError respectively) before proxy_process.terminate() on line 154 is reached. Since proxy_test_finally is the sole cleanup mechanism for the proxy process (called from finally blocks with no other atexit/fixture fallback), a failure in either wait leaks the mitmdump process. The old code always called terminate() + communicate() before any assertions, guaranteeing cleanup regardless of test outcome.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bc69cde. Configure here.

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.

1 participant