test(cloud): deflake httpbin-dependent URL construction tests#1075
Conversation
…l dependency Co-Authored-By: AJ Steers <aj@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784013784-deflake-httpbin-cloud-tests' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784013784-deflake-httpbin-cloud-tests'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughThe cloud API utility integration tests now use ChangesCloud API utility test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration_tests/cloud/test_cloud_api_util.py (1)
410-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
urlparsefor a more robust path assertion.To make this assertion a bit more robust and consistent with the previous test, how about using
urlparseto verify the path exactly? This avoids any potential false positives if the string somehow ended up in a query parameter. wdyt?♻️ Proposed refactor
- assert "/api/public/v1/sources" in sent_url, ( - f"Expected path prefix '/api/public/v1' to be preserved, got '{sent_url}'" - ) + parsed = urlparse(sent_url) + assert parsed.path == "/api/public/v1/sources", ( + f"Expected path '/api/public/v1/sources', got '{parsed.path}'" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_tests/cloud/test_cloud_api_util.py` around lines 410 - 411, Update the URL assertion in the relevant cloud API test to parse sent_url with the existing urlparse utility and compare its path exactly against /api/public/v1/sources, avoiding substring matching against query parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration_tests/cloud/test_cloud_api_util.py`:
- Around line 410-411: Update the URL assertion in the relevant cloud API test
to parse sent_url with the existing urlparse utility and compare its path
exactly against /api/public/v1/sources, avoiding substring matching against
query parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7566748-e829-43c7-93c8-2b867735237d
📒 Files selected for processing (1)
tests/integration_tests/cloud/test_cloud_api_util.py
Co-Authored-By: AJ Steers <aj@airbyte.io>
|
☑️ Resolved in 5d64c2e (re: CodeRabbit nitpick on the path assertion). Switched the path-prefix test to exact-match via sent_path = urlparse(sent_url).path
assert sent_path == "/api/public/v1/sources", ... |
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in the branch is 67%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in the branch is 67%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in the branch is 72%. The coverage in the branch is 71%. Show a code coverage summary of the most impacted files.
Updated |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Removes flaky integration tests that depended on the public httpbin.org service by mocking the HTTP layer with responses, while still asserting the SDK’s real URL construction behavior (including query params and path-prefix handling).
Changes:
- Replaced live
httpbin.orgcalls withresponses-mocked404and200responses for deterministic offline runs. - Strengthened the path-prefix test by asserting the actual request path sent (
responses.calls[0].request.url), not just the returned value. - Cleaned up test output by removing debug
printstatements and centralizing URL parsing utilities at module scope.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1784013784-deflake-httpbin-cloud-tests
branch
Summary
test_404_error_includes_request_url_contextandtest_url_construction_with_path_prefixintests/integration_tests/cloud/test_cloud_api_util.pypointed the SDK at the live publichttpbin.orgto simulate API responses. Whenever httpbin was overloaded it returned502/503instead of the expected404/200, failing the "All" pytest matrix jobs intermittently — seen on #1072 (assert 503 == 404, thenassert 502 == 404on a different runner across reruns).These are the only two tests in the file that don't take live-Cloud fixtures, so they run in the credential-free matrix jobs — exactly where the external dependency caused non-deterministic failures unrelated to the code under test.
Fix: mock the HTTP layer with
responses(already a dev dependency) instead of hitting httpbin. This preserves the original intent — verifying the SDK's real URL construction — while removing the external dependency:404forGET .../sourcesand assert the capturedrequest_urlcontext (host,/sourcespath,workspaceIdsquery param) exactly as before.200empty-list response for.../api/public/v1/sourcesand additionally assert the actual sent URL preserved the/api/public/v1prefix viaresponses.calls[0].request.url, which is stronger than the priorresult == []check.Both tests now run offline in ~0.2s. No production code changed.
Requested by AJ Steers after the flaky failure was noticed on #1072.
Link to Devin session: https://app.devin.ai/sessions/5bfc7d5ac9544cf1be05a9b1631cc890
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.