Skip to content

test(proxy): drop env-leaking runtime_proxy assertion in clear test#1362

Open
obchain wants to merge 2 commits intotinyhumansai:mainfrom
obchain:fix/1356-flaky-proxy-runtime-assertion
Open

test(proxy): drop env-leaking runtime_proxy assertion in clear test#1362
obchain wants to merge 2 commits intotinyhumansai:mainfrom
obchain:fix/1356-flaky-proxy-runtime-assertion

Conversation

@obchain
Copy link
Copy Markdown
Contributor

@obchain obchain commented May 8, 2026

Summary

Fixes #1356.

openhuman::tools::implementations::system::proxy_config::tests::set_null_proxy_url_clears_existing_value was flaky in CI: it asserted runtime_proxy.http_proxy was null after clearing the configured value, but runtime_proxy reflects the resolved proxy and falls back to the process env (HTTP_PROXY / http_proxy). Runners with those vars set produced a non-null resolved value and failed the assertion despite the set { http_proxy: null } call working correctly.

This PR keeps only the proxy.http_proxy (configured-state) assertion and documents why the runtime field is not asserted, so the env dependency is not re-introduced. Took the simpler of the two options the issue suggested — no env-mutation, no serial_test gate.

Repro

HTTP_PROXY=http://127.0.0.1:7890 cargo test --manifest-path Cargo.toml \
  openhuman::tools::implementations::system::proxy_config::tests::set_null_proxy_url_clears_existing_value
  • Before: assertion failed (parsed["runtime_proxy"]["http_proxy"].is_null()).
  • After: passes.

Test plan

  • cargo test ... set_null_proxy_url_clears_existing_value — passes in clean env.
  • HTTP_PROXY=http://127.0.0.1:7890 cargo test ... set_null_proxy_url_clears_existing_value — passes (was the failing repro).
  • CI green on this PR (sole exception is a transient Markdown Link Check infra flake on github.com unrelated to the diff).

PR Submission Checklist

  • N/A: no user-facing UI changes.
  • N/A: no new RPC surface or schema.
  • Coverage: change is inside an existing test; diff is a few lines and trivially covered.
  • Docs: one-line in-test comment documents the env-resolution behavior.

`runtime_proxy.http_proxy` resolves through HTTP_PROXY / http_proxy env
vars when the configured proxy is null, so CI runners with those vars
set fail the assertion despite `set { http_proxy: null }` working
correctly. Keep only the `proxy.http_proxy` check (configured state)
and document the env-resolution behavior so the dependency is not
re-introduced.

Fixes tinyhumansai#1356
@obchain obchain requested a review from a team May 8, 2026 06:38
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Test set_null_proxy_url_clears_existing_value assertion scope narrowed from both configured and resolved proxy state to configured proxy state only, with comments documenting why resolved proxy state is environment-dependent.

Changes

Proxy Config Test Flakiness Fix

Layer / File(s) Summary
Test Assertion Adjustment
src/openhuman/tools/impl/system/proxy_config_tests.rs
Assertion on parsed["runtime_proxy"]["http_proxy"] removed; assertion on parsed["proxy"]["http_proxy"] retained. Comments added explaining that runtime_proxy.http_proxy can resolve from HTTP_PROXY environment variable when configured value is null.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A flaky test finds its rest,
Env vars cease to be a guest,
We assert what we control with care,
And document the rest with flair.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing the runtime_proxy assertion from a proxy configuration test to address environment variable leakage.
Linked Issues check ✅ Passed The PR fulfills all coding requirements from issue #1356: removes the flaky runtime_proxy assertion, retains the configured proxy assertion, and includes documentation of why runtime_proxy is not asserted.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the flaky test in issue #1356; no unrelated modifications to other files or unrelated functionality are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

Flaky Rust test: proxy_config::set_null_proxy_url_clears_existing_value

1 participant