fix(tests): Handle stale secret reference errors in cloud integration tests #885
Conversation
… tests The shared CI workspace contains destinations with deleted/stale secret references, causing the Airbyte Cloud API to return 500 errors when listing destinations. This is an infrastructure issue, not a code bug. This fix adds targeted error handling to catch the specific SDKError with status 500 and 'Secret reference' in the message, and skips the test with an informative message instead of failing. Affected tests: - test_list_destinations (test_cloud_api_util.py) - test_deploy_destination (test_cloud_workspaces.py) Co-Authored-By: AJ Steers <aj@airbyte.io>
Original prompt from AJ Steers |
🤖 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. Testing 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/1764662151-fix-cloud-test-secret-reference' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1764662151-fix-cloud-test-secret-reference'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughTwo integration test files are modified to handle CI infrastructure edge cases by wrapping deployment-related test calls with error handling that skips tests when encountering stale secret reference errors, while preserving existing behavior for successful executions and other exceptions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Potential review focus:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration_tests/cloud/test_cloud_api_util.py (1)
21-21: Consider extracting the constant to a shared module to avoid duplication?The
STALE_SECRET_REFERENCE_ERRORconstant is defined identically in bothtests/integration_tests/cloud/test_cloud_api_util.pyandtests/integration_tests/cloud/test_cloud_workspaces.py. To follow DRY principles, you could extract it to a shared test utilities module, wdyt?For example, you could create a shared module like
tests/integration_tests/cloud/test_helpers.py:# tests/integration_tests/cloud/test_helpers.py from airbyte_api.errors import SDKError STALE_SECRET_REFERENCE_ERROR = "Secret reference" """Substring to detect stale secret reference errors in the CI workspace. When the shared CI workspace contains destinations with deleted/stale secret references, the Airbyte Cloud API returns a 500 error. This is an infrastructure issue, not a code bug. """Then import it in both test files:
from tests.integration_tests.cloud.test_helpers import STALE_SECRET_REFERENCE_ERRORAlso applies to: 31-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration_tests/cloud/test_cloud_api_util.py(3 hunks)tests/integration_tests/cloud/test_cloud_workspaces.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 347
File: tests/integration_tests/fixtures/registry.json:47-47
Timestamp: 2024-08-31T00:58:32.484Z
Learning: When reviewing changes in test fixtures, especially renaming, consider that they might be due to fixing copy-paste errors and may not impact core codepaths.
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 347
File: tests/integration_tests/fixtures/registry.json:47-47
Timestamp: 2024-10-08T15:34:31.026Z
Learning: When reviewing changes in test fixtures, especially renaming, consider that they might be due to fixing copy-paste errors and may not impact core codepaths.
🧬 Code graph analysis (1)
tests/integration_tests/cloud/test_cloud_workspaces.py (1)
airbyte/cloud/workspaces.py (1)
deploy_destination(195-248)
⏰ 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). (6)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (2)
tests/integration_tests/cloud/test_cloud_workspaces.py (1)
9-9: LGTM on the new imports!The
pytestandSDKErrorimports are correctly added to support the error handling logic.Also applies to: 15-15
tests/integration_tests/cloud/test_cloud_api_util.py (1)
96-109: The error handling is correctly applied only tolist_destinations.The stale secret reference error is specific to destinations in the CI workspace infrastructure. The docstring in
test_cloud_workspaces.pyconfirms this: "When the shared CI workspace contains destinations with deleted/stale secret references, the Airbyte Cloud API returns a 500 error. This is an infrastructure issue, not a code bug." Sincelist_destinationsqueries all destinations in the workspace, it exposes stale references, whereaslist_sources,create_destination, and other operations don't encounter this particular infrastructure issue. No additional error handling is needed elsewhere.Likely an incorrect or invalid review comment.
| try: | ||
| cloud_destination = cloud_workspace.deploy_destination( | ||
| name="test-destination-deleteme", | ||
| destination=deployable_dummy_destination, | ||
| random_name_suffix=True, | ||
| ) | ||
| except SDKError as e: | ||
| if e.status_code == 500 and STALE_SECRET_REFERENCE_ERROR in str(e): | ||
| pytest.skip( | ||
| "CI workspace contains destinations with stale secret references. " | ||
| "This is an infrastructure issue, not a code bug." | ||
| ) | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the codebase structure and locate where these methods are defined
git ls-files | head -20Repository: airbytehq/PyAirbyte
Length of output: 755
🏁 Script executed:
# Search for deploy_source and deploy_destination method definitions
rg "def deploy_source" -A 30Repository: airbytehq/PyAirbyte
Length of output: 5698
🏁 Script executed:
# Search for deploy_destination method definitions
rg "def deploy_destination" -A 30Repository: airbytehq/PyAirbyte
Length of output: 3934
🏁 Script executed:
# Look for STALE_SECRET_REFERENCE_ERROR definition and usage
rg "STALE_SECRET_REFERENCE_ERROR" -B 2 -A 2Repository: airbytehq/PyAirbyte
Length of output: 1847
🏁 Script executed:
# Read the full test file to see all deployment test methods
cat -n tests/integration_tests/cloud/test_cloud_workspaces.pyRepository: airbytehq/PyAirbyte
Length of output: 4557
🏁 Script executed:
# Check if deploy_source is used elsewhere with similar error handling
rg "deploy_source" tests/integration_tests/cloud/test_cloud_workspaces.py -B 3 -A 10Repository: airbytehq/PyAirbyte
Length of output: 1695
Add error handling to deploy_destination in test_deploy_connection
The deploy_destination call at line 95 can fail with the same stale secret reference error as test_deploy_destination, but lacks error handling. Apply the same try/except pattern from lines 31-43 to handle this infrastructure issue consistently.
(Note: deploy_source does not need this handling—the error is specific to destinations with stale secret references.)
🤖 Prompt for AI Agents
In tests/integration_tests/cloud/test_cloud_workspaces.py around lines 31-43 and
near line 95, add the same try/except handling used for deploy_destination at
lines 31-43 to the deploy_destination call in test_deploy_connection (around
line 95): wrap the deploy_destination invocation in a try block, catch SDKError
as e, check if e.status_code == 500 and STALE_SECRET_REFERENCE_ERROR is in
str(e), call pytest.skip with the same message about CI workspace stale secret
references, and re-raise the exception for other errors so the test behaves
consistently with test_deploy_destination.
Summary
Adds targeted error handling to cloud integration tests that fail when the shared CI workspace contains destinations with stale/deleted secret references. When the Airbyte Cloud API returns a 500 error with "Secret reference ... does not exist", the tests now skip with an informative message instead of failing.
This is a workaround for an infrastructure issue in the CI workspace, not a code bug. The affected tests are:
test_list_destinationsintest_cloud_api_util.pytest_deploy_destinationintest_cloud_workspaces.pyBoth tests call
list_destinations(directly or indirectly viadeploy_destination), which paginates through all destinations in the workspace and fails when it encounters one with a broken secret reference.Review & Testing Checklist for Human
status_code == 500AND"Secret reference"in message) is specific enough to not accidentally mask unrelated API errorsSTALE_SECRET_REFERENCE_ERRORconstant should be extracted to a shared test utilities module19d7a891-8e0e-40ac-8a8c-5faf8d11e47c)Test plan: Wait for CI to pass. The previously failing tests should now either pass (if the workspace is clean) or skip (if stale secrets are still present).
Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.