feat: add unit test fixtures for manifest-only connectors to CDK #121
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new file, Changes
Suggested reviewers
What do you think about reaching out to these reviewers for their insights? Wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 1
🧹 Outside diff range and nitpick comments (3)
airbyte_cdk/test/utils/manifest_only_fixtures.py (3)
11-16: Documentation looks great! Consider adding a minimal example?The documentation clearly explains how to import the fixtures. What do you think about adding a minimal example showing how these fixtures might be used in a test case? Something like:
def test_my_connector(components_module, manifest_path): assert components_module.MyComponent # example usage assert manifest_path.exists() # example usagewdyt? 🤔
32-52: Robust implementation with graceful error handling! Consider adding debug logs?The implementation handles all edge cases gracefully, but what do you think about adding some debug logs to help troubleshoot when things go wrong? Something like:
if not components_path.exists(): + logger.debug(f"No components.py found at {components_path}") return None components_spec = importlib.util.spec_from_file_location("components", components_path) if components_spec is None: + logger.debug("Failed to create module spec for components.py") return NoneThis could make it easier to understand why a component module wasn't loaded. wdyt? 🤔
1-57: Excellent architectural choices! 🎯The session-scoped fixtures are well-designed for reusability and efficiency. The separation of concerns between directory resolution, component loading, and manifest path management is clean and maintainable.
A few thoughts on the architecture:
- The fixtures are independent yet composable
- Session scope ensures optimal performance
- Error handling is graceful and non-blocking
This will make testing manifest-only connectors much more straightforward!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
airbyte_cdk/test/utils/manifest_only_fixtures.py(1 hunks)
🔇 Additional comments (1)
airbyte_cdk/test/utils/manifest_only_fixtures.py (1)
19-30: Clean implementation of directory resolution! 👍
The fixture elegantly handles both local development and CI environments. The session scope is perfect for this use case since the directory won't change during the test run.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
What
Adds reusable test fixtures to the CDK to support unit testing in manifest-only connectors. The goal is to provide centralized fixtures for loading custom components and manifest files so we don't have to copy/paste them for each individual connector to enable unit tests.
How
manifest_only_fixtures.pyinairbyte_cdk/test/utils/containing reusable pytest fixturesconnector_dir: Provides the root directory of the connector being testedcomponents_module: Dynamically loads the connector's custom components modulemanifest_path: Provides the path to the connector's manifest fileUser Impact
This PR only adds the discoverable fixtures for use by manifest-only connector test suites. There should be no effect/impact on CDK functionality in production. The code to run manifest-only unit tests will be added to airbyte-ci's test command in a separate PR
Summary by CodeRabbit