feat: Add --no-creds option to connector and image test commands #698
Conversation
- Add --no-creds flag to airbyte-cdk connector test command - Add --no-creds flag to airbyte-cdk image test command - Skip tests marked with 'requires_creds' when --no-creds is used - For connector test: use '-m not requires_creds' filter - For image test: combine with existing marker using 'image_tests and not requires_creds' - Maintains backward compatibility when flag is not used Co-Authored-By: AJ Steers <[email protected]>
👋 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@devin/1754415435-add-no-creds-option#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 devin/1754415435-add-no-creds-optionHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a --no-creds CLI option to both airbyte-cdk connector test and airbyte-cdk image test commands, allowing users to skip tests that require credentials. This is useful for CI environments, forks, and local development where credentials may not be available.
- Added
--no-credsflag to both connector and image test commands - Implemented pytest marker filtering to skip tests marked with
requires_creds - Maintained backward compatibility when the flag is not used
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| airbyte_cdk/cli/airbyte_cdk/_connector.py | Added --no-creds option and logic to filter out requires_creds tests |
| airbyte_cdk/cli/airbyte_cdk/_image.py | Added --no-creds option and modified marker filter to exclude requires_creds tests |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Pytest
User->>CLI: Run test command with or without --no-creds
CLI->>CLI: Check if --no-creds is set
alt --no-creds is set
CLI->>Pytest: Run tests excluding "requires_creds" markers
else --no-creds not set
CLI->>Pytest: Run all tests (including those requiring creds)
end
Pytest-->>CLI: Test results
CLI-->>User: Display results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
By the way, would you like me to help draft an example usage snippet for the new Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
airbyte_cdk/cli/airbyte_cdk/_image.py (1)
113-113: Solid implementation with a slightly different approach!The logic correctly filters out credential-requiring tests when the flag is enabled. I notice this uses a ternary operator to modify the marker expression directly (
"image_tests and not requires_creds"), while the connector implementation usesextend(["-m", "not requires_creds"]). Both approaches work correctly - wdyt about keeping them consistent, or is the inline approach preferable here since you're already constructing the marker expression?Also applies to: 134-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/cli/airbyte_cdk/_connector.py(2 hunks)airbyte_cdk/cli/airbyte_cdk/_image.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: the files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from ...
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
Applied to files:
airbyte_cdk/cli/airbyte_cdk/_connector.pyairbyte_cdk/cli/airbyte_cdk/_image.py
⏰ 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). (10)
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-shopify
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/cli/airbyte_cdk/_connector.py (2)
126-131: Nice implementation of the--no-credsflag!The CLI option follows established click patterns and has clear, descriptive help text. The default value ensures backward compatibility. LGTM!
137-137: Clean implementation of the credential filtering logic!The function signature update and pytest marker filtering logic look great. Using
extend(["-m", "not requires_creds"])correctly excludes tests marked with therequires_credsmarker when the flag is enabled. The implementation maintains backward compatibility and follows established patterns in the codebase.Also applies to: 157-158
airbyte_cdk/cli/airbyte_cdk/_image.py (1)
103-108: Consistent CLI option implementation!The
--no-credsflag definition matches the implementation in_connector.py, which provides a consistent user experience across both commands. Nice work maintaining consistency!
|
Note: I still need to plumb the |
Manual Testing Results for Dynamic
|
| Connector | Total Tests | Tests with --no-creds | Filtered Out | Status |
|---|---|---|---|---|
| source-microsoft-lists | 8 | 2 | 6 | ✅ PASS |
| source-intercom | 20 | 8 | 12 | ✅ PASS |
Conclusion
The dynamic requires_creds implementation is working correctly:
- Automatic Detection: No manual marking needed - scenarios automatically get markers based on config paths
- Granular Control: Each scenario gets its own marker based on its specific config path
- Perfect Integration: Works seamlessly with existing
--no-credsCLI options - Path-Based Logic: Correctly identifies
secrets/paths as requiring credentials
The implementation eliminates the need for manual test marking while providing accurate, scenario-specific credential detection.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
airbyte_cdk/test/standard_tests/pytest_hooks.py (1)
164-170: Update error message to reflect the method requirement, wdyt?The error message still references the
'scenarios'attribute, but the code now requires a'get_scenarios'method. Should we update this for clarity?- f"Test class {test_class} does not have a 'scenarios' attribute. " - "Please define the 'scenarios' attribute in the test class." + f"Test class {test_class} does not have a 'get_scenarios' method. " + "Please define the 'get_scenarios' method in the test class."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/test/models/scenario.py(1 hunks)airbyte_cdk/test/standard_tests/pytest_hooks.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/test/models/scenario.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: when modifying the `yamldeclarativesource` class in `airbyte_cdk/sources/declarative/yaml_declarativ...
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Applied to files:
airbyte_cdk/test/standard_tests/pytest_hooks.py
⏰ 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). (1)
- GitHub Check: Check: source-shopify
🔇 Additional comments (1)
airbyte_cdk/test/standard_tests/pytest_hooks.py (1)
175-189: Nice implementation of conditional marker application!The logic for wrapping scenarios in
pytest.paramobjects with conditionalrequires_credsmarkers looks solid and aligns perfectly with the PR objectives. This approach enables the--no-credsCLI option to filter out credential-requiring tests effectively.
2 participants
feat: Add --no-creds option to connector and image test commands
Summary
Adds a
--no-credsCLI option to bothairbyte-cdk connector testandairbyte-cdk image testcommands that skips tests marked with therequires_credspytest marker. This enables running tests without requiring credentials to be configured, which is useful for CI environments, forks, and local development where credentials may not be available.Key Changes:
--no-credsflag toairbyte-cdk connector testcommand--no-credsflag toairbyte-cdk image testcommand--no-credsis used, tests marked withrequires_credsare skipped via pytest marker filtering-m "not requires_creds"filter-m "image_tests and not requires_creds"Review & Testing Checklist for Human
requires_credstests - verify that--no-credsproperly skips those tests while running others"image_tests and not requires_creds"combination for image tests--collect-onlyand--pytest-arg--no-credsto ensure existing behavior is unchangedRecommended Test Plan:
@pytest.mark.requires_credsairbyte-cdk connector test <connector> --collect-only(should show requires_creds tests)airbyte-cdk connector test <connector> --no-creds --collect-only(should skip requires_creds tests)airbyte-cdk image test <connector> --no-creds --collect-only(should show only image_tests that don't require creds)Diagram
%%{ init : { "theme" : "default" }}%% graph TB CLI1["airbyte_cdk/cli/airbyte_cdk/<br/>_connector.py"]:::major-edit CLI2["airbyte_cdk/cli/airbyte_cdk/<br/>_image.py"]:::major-edit SHARED["airbyte_cdk/cli/airbyte_cdk/<br/>_connector.py<br/>run_connector_tests()"]:::context PYTEST_INI["pytest.ini<br/>requires_creds marker"]:::context PYPROJECT["pyproject.toml<br/>existing pattern"]:::context CLI1 -->|"calls with<br/>extra_pytest_args"| SHARED CLI2 -->|"calls with<br/>extra_pytest_args"| SHARED CLI1 -.->|"adds -m not requires_creds<br/>when --no-creds used"| PYTEST_INI CLI2 -.->|"adds -m image_tests and<br/>not requires_creds"| PYTEST_INI PYPROJECT -.->|"existing pattern:<br/>pytest-fast excludes<br/>requires_creds"| PYTEST_INI subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#F5F5F5Notes
pyproject.tomlwherepytest-fastalready excludesrequires_credstestsrequires_credsmarker is already defined inpytest.inirun_connector_tests()function, so the implementation is consistentLink to Devin session: https://app.devin.ai/sessions/9fc0a40482c64cbc8c865fb7f009f415
Requested by: @aaronsteers
Summary by CodeRabbit
--no-credscommand-line option to both the connector and image test commands, allowing users to easily skip tests that require credentials.