Feat: connector-builder-test-read CLI with custom components support (do not merge)
#329
Conversation
aaronsteers
changed the title
connector-builder-test-read CLI with custom components supportconnector-builder-test-read CLI with custom components support (do not merge)
📝 WalkthroughWalkthroughThis pull request introduces a new CLI command for running test reads in the Airbyte Connector Builder. It refactors the run entry point for modularity, replaces hardcoded values with imported constants, removes copyright notices from one file, and enhances several class and module docstrings. Additionally, it adds a comprehensive declarative testing framework with new protocols, models, test suite base classes, pytest fixtures, and a custom pytest plugin. The script entry in the project configuration is updated accordingly. Changes
Sequence Diagram(s)Connector Builder CLI FlowsequenceDiagram
participant U as User
participant CLI as Connector CLI
participant RUN as run()
participant HANDLER as handle_request()
U->>CLI: Execute "connector-builder-test-read"
CLI->>RUN: Call run() (args from sys.argv)
RUN->>HANDLER: Process CLI arguments
HANDLER-->>RUN: Return result (or error message)
RUN-->>CLI: Output result
Test Job Runner Execution FlowsequenceDiagram
participant T as Test Runner
participant JR as run_test_job()
participant CO as Connector Object
participant EP as _run_command()
T->>JR: Invoke run_test_job() with connector, verb, test_instance, catalog
JR->>CO: Validate/Create connector instance
alt Catalog provided
JR->>JR: Write catalog to temporary JSON file
end
JR->>EP: Call _run_command() with arguments
EP-->>JR: Return job result or error details
JR-->>T: Return EntrypointOutput
Suggested labels
Suggested reviewers
Would you like any adjustments to the diagrams or additional clarification on any section, wdyt? ✨ Finishing Touches
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: 13
🧹 Nitpick comments (8)
airbyte_cdk/test/fixtures/general.py (2)
1-10: Hey there! Mind if we enhance the docstring a bit? 🤔The docstring shows great usage examples, but what do you think about adding a brief description of what the fixture actually does? Something like:
"""General fixtures for pytest. +Returns the parent directory of the test fixtures, useful for resolving test data files +and other test-related resources relative to this location. + Usage: ```python from airbyte_cdk.test.fixtures.general import * # OR: from airbyte_cdk.test.fixtures.general import connector_test_dir"""
wdyt? <details> <summary>🧰 Tools</summary> <details> <summary>🪛 GitHub Actions: Linters</summary> [warning] 1-1: File would be reformatted. Please format the code according to the style guide. </details> </details> --- `13-14`: **Quick thought about the fixture's scope 🤔** Since this path won't change during test execution, we could optimize it by setting the fixture's scope to 'session'. Would you be interested in that? Here's how: ```diff [email protected] [email protected](scope="session") def connector_test_dir(): return Path(__file__).parentThis would compute the path only once per test session rather than for each test. wdyt?
airbyte_cdk/test/declarative/utils/job_runner.py (1)
18-18: Add type parameters fordict.The type checker flags the generic
dictwithout parameters. Would you like to specify it asdict[str, object](ordict[str, Any]) for clarity and correctness? WDYT?- catalog: dict | None = None, + from typing import Any + catalog: dict[str, Any] | None = None,🧰 Tools
🪛 GitHub Actions: Linters
[error] 18-18: Missing type parameters for generic type 'dict'
airbyte_cdk/test/declarative/connector_interfaces.py (1)
9-9: Add a return type annotation tolaunch.The linter flags a missing return type. If no return is expected, would you like to annotate it as
-> None? WDYT?- def launch(cls, args: list[str] | None): ... + def launch(cls, args: list[str] | None) -> None: ...🧰 Tools
🪛 GitHub Actions: Linters
[error] 9-9: Function is missing a return type annotation
airbyte_cdk/test/declarative/models/scenario.py (1)
27-34: Consider moving nested models to separate classes?The nested models
AcceptanceTestExpectRecordsandAcceptanceTestFileTypescould be moved outside the main class for better reusability and cleaner structure. This would also make it easier to import and use these models independently. Wdyt?airbyte_cdk/test/pytest_config/plugin.py (1)
33-41: Consider using proper logging instead of print statements?Using print statements for setup/teardown messages might not be ideal in a testing framework. Would you consider using Python's logging module instead? This would provide better control over log levels and output formatting. Something like:
+import logging + +logger = logging.getLogger(__name__) + def pytest_runtest_setup(item: pytest.Item) -> None: - print(f"Setting up test: {item.name}") + logger.info("Setting up test: %s", item.name) def pytest_runtest_teardown(item: pytest.Item, nextitem: pytest.Item | None) -> None: - print(f"Tearing down test: {item.name}") + logger.info("Tearing down test: %s", item.name)🧰 Tools
🪛 GitHub Actions: Linters
[error] 33-33: Function is missing a type annotation
[error] 38-38: Function is missing a type annotation
airbyte_cdk/test/declarative/test_suites/source_base.py (2)
46-47: Consider using a more specific exceptionInstead of using a bare
AssertionError, would it be better to use a custom exception or provide more context? This would make error handling more explicit and maintainable.What do you think about this approach?
- raise AssertionError("Expected records but got none.") # noqa: TRY003 + raise ValueError(f"No records returned for instance '{instance.instance_name}'. Expected at least one record.")
114-116: Fix incomplete commentThe comment appears to be cut off and doesn't complete the thought.
How about completing the comment like this?
- # Set expected status to "failed" to ensure the test fails if the connector. + # Set expected status to "failed" to ensure the test fails if the connector succeeds with an invalid catalog.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
airbyte_cdk/cli/connector_builder_test_read/__init__.py(1 hunks)airbyte_cdk/cli/connector_builder_test_read/connector_builder_handler.py(2 hunks)airbyte_cdk/cli/connector_builder_test_read/run.py(1 hunks)airbyte_cdk/connector_builder/__init__.py(0 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(1 hunks)airbyte_cdk/test/declarative/__init__.py(1 hunks)airbyte_cdk/test/declarative/connector_interfaces.py(1 hunks)airbyte_cdk/test/declarative/models/__init__.py(1 hunks)airbyte_cdk/test/declarative/models/scenario.py(1 hunks)airbyte_cdk/test/declarative/test_suites/__init__.py(1 hunks)airbyte_cdk/test/declarative/test_suites/connector_base.py(1 hunks)airbyte_cdk/test/declarative/test_suites/destination_base.py(1 hunks)airbyte_cdk/test/declarative/test_suites/source_base.py(1 hunks)airbyte_cdk/test/declarative/utils/job_runner.py(1 hunks)airbyte_cdk/test/fixtures/auto.py(1 hunks)airbyte_cdk/test/fixtures/general.py(1 hunks)airbyte_cdk/test/pytest_config/plugin.py(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- airbyte_cdk/connector_builder/init.py
✅ Files skipped from review due to trivial changes (4)
- airbyte_cdk/test/declarative/init.py
- airbyte_cdk/test/declarative/test_suites/init.py
- airbyte_cdk/sources/declarative/manifest_declarative_source.py
- airbyte_cdk/test/fixtures/auto.py
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/test/fixtures/general.py
[warning] 1-1: File would be reformatted. Please format the code according to the style guide.
airbyte_cdk/cli/connector_builder_test_read/__init__.py
[warning] 1-1: File would be reformatted. Please format the code according to the style guide.
airbyte_cdk/test/declarative/models/scenario.py
[error] 44-44: Incompatible return value type (got 'bool | None', expected 'bool')
airbyte_cdk/cli/connector_builder_test_read/run.py
[error] 78-78: Returning Any from function declared to return 'AirbyteMessage'
[error] 83-83: Returning Any from function declared to return 'AirbyteMessage'
[error] 112-112: Missing positional argument 'args' in call to 'run'
airbyte_cdk/test/declarative/connector_interfaces.py
[error] 9-9: Function is missing a return type annotation
airbyte_cdk/test/declarative/test_suites/connector_base.py
[error] 50-50: Returning Any from function declared to return 'Connector'
[error] 50-50: 'ConnectorTestSuiteBase' has no attribute 'connector_factory'
[error] 55-55: Self argument missing for a non-static method (or an invalid type for self)
[error] 83-83: Function is missing a type annotation
[error] 92-92: Name 'self' is not defined
[error] 109-109: Argument 1 to 'join' of 'str' has incompatible type 'list[AirbyteMessage]'; expected 'Iterable[str]'
airbyte_cdk/test/declarative/utils/job_runner.py
[error] 18-18: Missing type parameters for generic type 'dict'
[error] 29-29: Argument 2 to 'isinstance' has incompatible type ''; expected '_ClassInfo'
[error] 45-45: List item 0 has incompatible type 'Literal['--config']'; expected 'Literal['read', 'check', 'discover']'
[error] 45-45: List item 1 has incompatible type 'str'; expected 'Literal['read', 'check', 'discover']'
[error] 62-62: List item 0 has incompatible type 'Literal['--catalog']'; expected 'Literal['read', 'check', 'discover']'
[error] 62-62: List item 1 has incompatible type 'str'; expected 'Literal['read', 'check', 'discover']'
[error] 68-68: Argument 'source' to '_run_command' has incompatible type 'Connector'; expected 'Source'
[error] 69-69: Argument 'args' to '_run_command' has incompatible type 'list[Literal['read', 'check', 'discover']]'; expected 'list[str]'
[error] 69-69: Item 'None' of 'Any | None' has no attribute 'error'
airbyte_cdk/test/pytest_config/plugin.py
[error] 6-6: Function is missing a type annotation
[error] 11-11: Function is missing a type annotation
[error] 15-15: Function is missing a type annotation
[error] 24-24: Function is missing a type annotation
[error] 33-33: Function is missing a type annotation
[error] 38-38: Function is missing a type annotation
🪛 GitHub Actions: Generate Docs
airbyte_cdk/cli/connector_builder_test_read/run.py
[error] 12-12: ModuleNotFoundError: No module named 'airbyte_cdk.connector_builder'
airbyte_cdk/test/declarative/test_suites/connector_base.py
[error] 12-12: ModuleNotFoundError: No module named 'airbyte_connector_tester'
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
airbyte_cdk/test/declarative/models/__init__.py (1)
1-7: Looks good!Importing
AcceptanceTestScenarioand exporting it via__all__is clear and organized.airbyte_cdk/test/declarative/test_suites/destination_base.py (1)
7-12: LGTM! Well-documented base class.The class structure and documentation are clear and concise. Good job on providing comprehensive docstrings that explain the purpose and inheritance hierarchy.
airbyte_cdk/cli/connector_builder_test_read/__init__.py (1)
1-18: Format code according to style guide.The linter indicates that this file needs reformatting. Would you like me to help with the formatting? I can generate a properly formatted version if you'd like.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] 1-1: File would be reformatted. Please format the code according to the style guide.
airbyte_cdk/cli/connector_builder_test_read/connector_builder_handler.py (1)
55-57: LGTM! Good use of constantsNice improvement replacing the hardcoded string with the imported
INJECTED_MANIFESTconstant. This makes the code more maintainable and less prone to typos.
| @pytest.fixture | ||
| def connector_test_dir(): | ||
| return Path(__file__).parent |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing imports and formatting issues 🔍
The code looks good functionally, but we're missing some imports. Mind if we add them and fix the formatting?
+import pytest
+from pathlib import Path
@pytest.fixture
def connector_test_dir():
- return Path(__file__).parent
+ return Path(__file__).parentThis would resolve both the missing imports and the formatting warning from the pipeline. What do you think?
Committable suggestion skipped: line range outside the PR's diff.
| # Because it *also* can fail, we have ot redundantly wrap it in a try/except block. | ||
|
|
||
| result: entrypoint_wrapper.EntrypointOutput = entrypoint_wrapper._run_command( # noqa: SLF001 # Non-public API | ||
| source=connector_obj, |
There was a problem hiding this comment.
Align _run_command parameter with expected Source type.
_run_command expects a Source, but here we pass a Connector. Would you like to cast or refactor to ensure type compatibility, for example by changing the code to pass a Source object or updating _run_command to accept a Connector? WDYT?
🧰 Tools
🪛 GitHub Actions: Linters
[error] 68-68: Argument 'source' to '_run_command' has incompatible type 'Connector'; expected 'Source'
| connector_obj = connector() | ||
| elif isinstance(connector, Connector): | ||
| connector_obj = connector | ||
| elif isinstance(connector, Callable): |
There was a problem hiding this comment.
Use a built-in check for callable objects instead of isinstance(..., Callable).
Typing Callable is not recognized as a valid class in isinstance(). Would you like to replace elif isinstance(connector, Callable): with elif callable(connector): to address the pipeline error? WDYT?
- elif isinstance(connector, Callable):
+ elif callable(connector):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif isinstance(connector, Callable): | |
| elif callable(connector): |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 29-29: Argument 2 to 'isinstance' has incompatible type ''; expected '_ClassInfo'
| args = [verb] | ||
| if test_instance.config_path: | ||
| args += ["--config", str(test_instance.config_path)] | ||
|
|
||
| catalog_path: Path | None = None | ||
| if verb not in ["discover", "check"]: | ||
| if catalog: | ||
| # Write the catalog to a temp json file and pass the path to the file as an argument. | ||
| catalog_path = ( | ||
| Path(tempfile.gettempdir()) | ||
| / "airbyte-test" | ||
| / f"temp_catalog_{uuid.uuid4().hex}.json" | ||
| ) | ||
| catalog_path.parent.mkdir(parents=True, exist_ok=True) | ||
| catalog_path.write_text(orjson.dumps(catalog).decode()) | ||
| elif test_instance.configured_catalog_path: | ||
| catalog_path = Path(test_instance.configured_catalog_path) | ||
|
|
||
| if catalog_path: | ||
| args += ["--catalog", str(catalog_path)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider changing args to list[str] to accommodate flags like --config and --catalog.
Currently, args is inferred from [verb] which is a Literal["read","check","discover"]. Adding ["--config", ...] breaks the type. Would you like to unify the type by declaring args as list[str] from the start? WDYT?
- args = [verb]
+ args: list[str] = [verb]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args = [verb] | |
| if test_instance.config_path: | |
| args += ["--config", str(test_instance.config_path)] | |
| catalog_path: Path | None = None | |
| if verb not in ["discover", "check"]: | |
| if catalog: | |
| # Write the catalog to a temp json file and pass the path to the file as an argument. | |
| catalog_path = ( | |
| Path(tempfile.gettempdir()) | |
| / "airbyte-test" | |
| / f"temp_catalog_{uuid.uuid4().hex}.json" | |
| ) | |
| catalog_path.parent.mkdir(parents=True, exist_ok=True) | |
| catalog_path.write_text(orjson.dumps(catalog).decode()) | |
| elif test_instance.configured_catalog_path: | |
| catalog_path = Path(test_instance.configured_catalog_path) | |
| if catalog_path: | |
| args += ["--catalog", str(catalog_path)] | |
| args: list[str] = [verb] | |
| if test_instance.config_path: | |
| args += ["--config", str(test_instance.config_path)] | |
| catalog_path: Path | None = None | |
| if verb not in ["discover", "check"]: | |
| if catalog: | |
| # Write the catalog to a temp json file and pass the path to the file as an argument. | |
| catalog_path = ( | |
| Path(tempfile.gettempdir()) | |
| / "airbyte-test" | |
| / f"temp_catalog_{uuid.uuid4().hex}.json" | |
| ) | |
| catalog_path.parent.mkdir(parents=True, exist_ok=True) | |
| catalog_path.write_text(orjson.dumps(catalog).decode()) | |
| elif test_instance.configured_catalog_path: | |
| catalog_path = Path(test_instance.configured_catalog_path) | |
| if catalog_path: | |
| args += ["--catalog", str(catalog_path)] |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 45-45: List item 0 has incompatible type 'Literal['--config']'; expected 'Literal['read', 'check', 'discover']'
[error] 45-45: List item 1 has incompatible type 'str'; expected 'Literal['read', 'check', 'discover']'
[error] 62-62: List item 0 has incompatible type 'Literal['--catalog']'; expected 'Literal['read', 'check', 'discover']'
[error] 62-62: List item 1 has incompatible type 'str'; expected 'Literal['read', 'check', 'discover']'
| @property | ||
| def expect_exception(self) -> bool: | ||
| return self.status and self.status == "failed" |
There was a problem hiding this comment.
Fix type safety in expect_exception property.
The property could return None when self.status is None, which doesn't match the declared bool return type. Wdyt about this fix?
@property
def expect_exception(self) -> bool:
- return self.status and self.status == "failed"
+ return bool(self.status and self.status == "failed")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def expect_exception(self) -> bool: | |
| return self.status and self.status == "failed" | |
| @property | |
| def expect_exception(self) -> bool: | |
| - return self.status and self.status == "failed" | |
| + return bool(self.status and self.status == "failed") |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 44-44: Incompatible return value type (got 'bool | None', expected 'bool')
| @pytest.mark.parametrize( | ||
| "instance", | ||
| self._get_acceptance_tests("connection"), | ||
| ids=lambda instance: instance.instance_name, |
There was a problem hiding this comment.
Fix parametrize decorator usage
The decorator is using undefined self when calling _get_acceptance_tests.
What about using cls instead?
@pytest.mark.parametrize(
"instance",
- self._get_acceptance_tests("connection"),
+ _get_acceptance_tests("connection"),
ids=lambda instance: instance.instance_name,
)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Linters
[error] 92-92: Name 'self' is not defined
| @classmethod | ||
| def _get_acceptance_tests( | ||
| category: str, | ||
| accept_test_config_path: Path = ACCEPTANCE_TEST_CONFIG_PATH, | ||
| ) -> list[AcceptanceTestScenario]: | ||
| all_tests_config = yaml.safe_load(accept_test_config_path.read_text()) | ||
| if "acceptance_tests" not in all_tests_config: | ||
| raise ValueError(f"Acceptance tests config not found in {accept_test_config_path}") | ||
| if category not in all_tests_config["acceptance_tests"]: | ||
| return [] | ||
| if "tests" not in all_tests_config["acceptance_tests"][category]: | ||
| raise ValueError(f"No tests found for category {category}") | ||
|
|
||
| return [ | ||
| AcceptanceTestScenario.model_validate(test) | ||
| for test in all_tests_config["acceptance_tests"][category]["tests"] | ||
| if "iam_role" not in test["config_path"] | ||
| ] |
There was a problem hiding this comment.
Fix classmethod signature
The _get_acceptance_tests classmethod is missing the cls parameter. This will cause issues when accessing class attributes.
How about this fix?
@classmethod
def _get_acceptance_tests(
+ cls,
category: str,
accept_test_config_path: Path = ACCEPTANCE_TEST_CONFIG_PATH,
) -> list[AcceptanceTestScenario]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @classmethod | |
| def _get_acceptance_tests( | |
| category: str, | |
| accept_test_config_path: Path = ACCEPTANCE_TEST_CONFIG_PATH, | |
| ) -> list[AcceptanceTestScenario]: | |
| all_tests_config = yaml.safe_load(accept_test_config_path.read_text()) | |
| if "acceptance_tests" not in all_tests_config: | |
| raise ValueError(f"Acceptance tests config not found in {accept_test_config_path}") | |
| if category not in all_tests_config["acceptance_tests"]: | |
| return [] | |
| if "tests" not in all_tests_config["acceptance_tests"][category]: | |
| raise ValueError(f"No tests found for category {category}") | |
| return [ | |
| AcceptanceTestScenario.model_validate(test) | |
| for test in all_tests_config["acceptance_tests"][category]["tests"] | |
| if "iam_role" not in test["config_path"] | |
| ] | |
| @classmethod | |
| def _get_acceptance_tests( | |
| cls, | |
| category: str, | |
| accept_test_config_path: Path = ACCEPTANCE_TEST_CONFIG_PATH, | |
| ) -> list[AcceptanceTestScenario]: | |
| all_tests_config = yaml.safe_load(accept_test_config_path.read_text()) | |
| if "acceptance_tests" not in all_tests_config: | |
| raise ValueError(f"Acceptance tests config not found in {accept_test_config_path}") | |
| if category not in all_tests_config["acceptance_tests"]: | |
| return [] | |
| if "tests" not in all_tests_config["acceptance_tests"][category]: | |
| raise ValueError(f"No tests found for category {category}") | |
| return [ | |
| AcceptanceTestScenario.model_validate(test) | |
| for test in all_tests_config["acceptance_tests"][category]["tests"] | |
| if "iam_role" not in test["config_path"] | |
| ] |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 55-55: Self argument missing for a non-static method (or an invalid type for self)
| @abc.abstractmethod | ||
| def new_connector(self, **kwargs: dict[str, Any]) -> Connector: | ||
| """Create a new connector instance. | ||
|
|
||
| By default, this returns a new instance of the connector class. Subclasses | ||
| may override this method to generate a dynamic connector instance. | ||
| """ | ||
| return self.connector_factory() |
There was a problem hiding this comment.
Fix implementation of abstract method new_connector
The current implementation has a few issues:
- The method is marked as abstract but provides an implementation
- It uses undefined
connector_factory - The return type annotation is missing in the implementation
What do you think about this implementation instead?
@abc.abstractmethod
def new_connector(self, **kwargs: dict[str, Any]) -> Connector:
"""Create a new connector instance.
By default, this returns a new instance of the connector class. Subclasses
may override this method to generate a dynamic connector instance.
"""
- return self.connector_factory()
+ raise NotImplementedError("Subclasses must implement new_connector")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @abc.abstractmethod | |
| def new_connector(self, **kwargs: dict[str, Any]) -> Connector: | |
| """Create a new connector instance. | |
| By default, this returns a new instance of the connector class. Subclasses | |
| may override this method to generate a dynamic connector instance. | |
| """ | |
| return self.connector_factory() | |
| @abc.abstractmethod | |
| def new_connector(self, **kwargs: dict[str, Any]) -> Connector: | |
| """Create a new connector instance. | |
| By default, this returns a new instance of the connector class. Subclasses | |
| may override this method to generate a dynamic connector instance. | |
| """ | |
| - return self.connector_factory() | |
| + raise NotImplementedError("Subclasses must implement new_connector") |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 50-50: Returning Any from function declared to return 'Connector'
[error] 50-50: 'ConnectorTestSuiteBase' has no attribute 'connector_factory'
| if __name__ == "__main__": | ||
| run() |
There was a problem hiding this comment.
Fix function call to match signature
The run() function is called without the required args parameter. While the function has a default value, it's better to be explicit.
Would this be clearer?
if __name__ == "__main__":
- run()
+ run(None) # Explicitly pass None to use default sys.argv[1:]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if __name__ == "__main__": | |
| run() | |
| if __name__ == "__main__": | |
| run(None) # Explicitly pass None to use default sys.argv[1:] |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 112-112: Missing positional argument 'args' in call to 'run'
| source-declarative-manifest = "airbyte_cdk.cli.source_declarative_manifest:run" | ||
| connector-builder-test-read = "airbyte_cdk.cli.connector_builder_test_read:run" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new CLI command setup.
The script entry looks good! Just want to verify a few things to ensure everything is properly wired up. 🔍
Let's check if the module and entry point exist:
Hey @aaronsteers, would you mind confirming if you've added tests for this new CLI command? If not, I'd be happy to help draft some test cases. wdyt? 🤔
🏁 Script executed:
#!/bin/bash
# Description: Verify the module setup for the new CLI command
# Check if the module exists
echo "Checking for the module..."
fd "connector_builder_test_read" --type f
# Check for the run function
echo -e "\nChecking for the run function..."
rg -A 5 "def run" --type py | rg "connector_builder_test_read"
# Check for tests
echo -e "\nChecking for tests..."
fd "test.*connector_builder_test_read" --type fLength of output: 865
Missing tests for the new CLI command.
The new module at airbyte_cdk/cli/connector_builder_test_read/run.py properly defines the run function, so the CLI entry looks correctly set up. However, it appears there are no tests for this functionality. Would you mind adding some tests for the new CLI command? wdyt?
Resolves: https://github.com/airbytehq/airbyte-internal-issues/issues/11360
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests