fix: noproxy that breaks regression tests #777
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@maxi297/fix_bug_in_no_proxy#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch maxi297/fix_bug_in_no_proxyHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
|
/autofix
|
📝 WalkthroughWalkthroughsend_request now ensures Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Would you like me to add a short note in the client docs about the explicit empty-proxies default for clarity, wdyt? Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
unit_tests/sources/streams/http/test_http_client.py (1)
1-1: Fix the formatting issue detected by ruff.The linter detected that this file needs reformatting. Please run
ruff formaton this file to fix the code style issues.Run the following command to fix:
ruff format unit_tests/sources/streams/http/test_http_client.py
🧹 Nitpick comments (1)
unit_tests/sources/streams/http/test_http_client.py (1)
750-764: Consider cleaning up the environment variable and strengthening the assertion.A few suggestions to make this test more robust:
The
no_proxyenvironment variable is set but never cleaned up, which could affect other tests. Consider using@patch.dict("os.environ", {"no_proxy": "another.com"})decorator instead (similar to the test at line 766), wdyt?The variable name
xis not descriptive—mayberequest, responseto match the return type?The assertion
assert xjust checks truthiness. Would it be clearer to assert on the response more specifically, likeassert response.status_code == 200orassert response.json()["test"] == "a response"?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/streams/http/http_client.py(1 hunks)unit_tests/sources/streams/http/test_http_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte_cdk/sources/streams/http/http_client.py (1)
airbyte_cdk/sources/streams/http/http.py (1)
request_kwargs(242-253)
unit_tests/sources/streams/http/test_http_client.py (1)
airbyte_cdk/sources/streams/http/http_client.py (2)
name(533-534)send_request(536-579)
🪛 GitHub Actions: Linters
unit_tests/sources/streams/http/test_http_client.py
[error] 1-1: ruff format check detected that the file would be reformatted. Run 'ruff format' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
airbyte_cdk/sources/streams/http/http_client.py (1)
565-565: LGTM! The fix correctly prevents the NoneType exception.Defaulting
proxiesto an empty dict when not provided ensures thatmerge_environment_settingsalways receives a dict, which prevents the exception whenno_proxyis set. This is a minimal and targeted fix that directly addresses the regression from PR 755.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/streams/http/test_http_client.py (1)
750-762: Clean up the environment variable and improve variable naming, wdyt?This test validates the no_proxy fix, which is great! A couple of suggestions:
The
no_proxyenvironment variable is set but never cleaned up, which could interfere with other tests. Consider using@patch.dict("os.environ", {"no_proxy": "another.com"})as a decorator (similar to line 764) to ensure automatic cleanup.The variable name
xon line 759 is quite generic. Consider using something more descriptive likerequest_response_tupleorresult.Apply this diff to improve the test:
+@patch.dict("os.environ", {"no_proxy": "another.com"}) def test_given_noproxy_for_another_url_when_send_request_then_do_not_break(requests_mock): http_client = HttpClient(name="test", logger=MagicMock(), use_cache=True) - os.environ["no_proxy"] = "another.com" requests_mock.register_uri( "GET", "https://google.com/", json={"test": "a response"}, ) - x = http_client.send_request("GET", "https://google.com/", request_kwargs={}) + result = http_client.send_request("GET", "https://google.com/", request_kwargs={}) - assert x + assert result + assert isinstance(result, tuple) + assert len(result) == 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/streams/http/test_http_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_tests/sources/streams/http/test_http_client.py (1)
airbyte_cdk/sources/streams/http/http_client.py (2)
name(533-534)send_request(536-579)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
unit_tests/sources/streams/http/test_http_client.py (1)
764-781: LGTM! The test properly validates REQUESTS_CA_BUNDLE handling.This test is well-structured with proper environment variable patching and clear assertions. The use of
@patch.dictensures clean teardown, and mocking_send_with_retryis an effective way to verify that environment settings are merged correctly.Optional suggestion: The test name mentions "environment variables" (plural), but it only verifies
REQUESTS_CA_BUNDLE. If you'd like to test additional environment variables (likeHTTP_PROXY,HTTPS_PROXY, etc.), you could parameterize this test or add separate test cases, wdyt?
What
Following this change, regression tests (or anything run having
noproxyas a env and querying another domain than what is defined innoproxywill cause a NoneType exceptionHow
Pass a dict if proxies is not defined
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Other