Tests: Simplify smoke-tests DSL and update documentation #775
Conversation
👋 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@aj/ci/tests/new-smoke-tests-dsl-config-dialect#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 aj/ci/tests/new-smoke-tests-dsl-config-dialectHelpful 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 simplifies the smoke test DSL by replacing the existing complex acceptance test configuration with a cleaner, metadata.yaml-based approach. The changes modernize test scenario definitions and improve the overall testing experience.
- Replaces complex acceptance test configuration with simplified smoke test DSL in metadata.yaml
- Updates scenario handling to use new
expect_failurefield instead of status-based expectations - Refactors stream filtering logic to use new callback-based approach instead of explicit empty stream lists
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| airbyte_cdk/test/standard_tests/source_base.py | Updates test methods to use new scenario properties and stream filtering |
| airbyte_cdk/test/standard_tests/docker_base.py | Adds metadata.yaml parsing and updates scenario handling for new DSL format |
| airbyte_cdk/test/standard_tests/declarative_sources.py | Updates default scenario creation to use new required fields |
| airbyte_cdk/test/standard_tests/_job_runner.py | Simplifies job execution logic by removing outcome expectations |
| airbyte_cdk/test/models/scenario.py | Major refactor introducing new ConnectorTestScenario model and legacy support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ) | ||
|
|
||
| if scenario.expected_outcome.expect_success() and not result.records: | ||
| if scenario.expect_failure and not result.records: |
There was a problem hiding this comment.
The logic is inverted. This should check if not scenario.expect_failure and not result.records: to assert that records are expected when the scenario should succeed.
| if scenario.expect_failure and not result.records: | |
| if not scenario.expect_failure and not result.records: |
Copilot uses AI. Check for mistakes.
| ) | ||
| for stream in discovered_catalog.streams | ||
| if stream.name in streams_list | ||
| if scenario.get_streams_filter()(stream.name) |
There was a problem hiding this comment.
This filter is applied incorrectly. The condition should also check if stream.name in streams_list to respect the existing stream filtering logic from read_from_streams.
| if scenario.get_streams_filter()(stream.name) | |
| if stream.name in streams_list and scenario.get_streams_filter()(stream.name) |
Copilot uses AI. Check for mistakes.
| if "config_file" not in definition: | ||
| raise ValueError("Smoke test scenario definition must include a 'config_file' field.") |
There was a problem hiding this comment.
This validation is too strict. According to the documentation, scenarios can use config_settings without a config_file, but this code requires config_file to always be present.
| if "config_file" not in definition: | |
| raise ValueError("Smoke test scenario definition must include a 'config_file' field.") | |
| if "config_file" not in definition and "config_settings" not in definition: | |
| raise ValueError("Smoke test scenario definition must include either a 'config_file' or 'config_settings' field.") |
Copilot uses AI. Check for mistakes.
📝 WalkthroughWalkthroughIntroduces a new smoke-test oriented ConnectorTestScenario and LegacyAcceptanceTestScenario, updates scenario construction and config handling, replaces expected_outcome logic with a single expect_failure flag, adds metadata.yaml-driven scenario loading, and centralizes stream filtering via get_streams_filter across job runner, docker test suite, and source tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Test Runner
participant DTS as DockerConnectorTestSuite
participant Meta as metadata.yaml
participant Legacy as LegacyAcceptanceTestScenario
participant CTS as ConnectorTestScenario
participant Src as Source Under Test
Dev->>DTS: get_scenarios()
alt metadata.yaml has smokeTests
DTS->>Meta: read smokeTests
Meta-->>DTS: definitions
DTS->>CTS: from_metadata_smoke_test_definition(...)
DTS-->>Dev: [CTS...]
else legacy path
DTS->>Legacy: model_validate(legacy_config)
Legacy->>CTS: as_test_scenario()
DTS-->>Dev: [CTS...]
end
loop per scenario
Dev->>Src: check(config from CTS.get_config_dict)
alt CTS.expect_failure
note right of Dev: Expect error on check
Src-->>Dev: error/exception
else
Src-->>Dev: OK
Dev->>Src: discover()
Src-->>Dev: catalog
Dev->>Dev: filter streams via CTS.get_streams_filter()
Dev->>Src: read(filtered catalog)
alt CTS.expect_failure
note right of Dev: Any success is unexpected
else
Src-->>Dev: records
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (5)airbyte_cdk/test/standard_tests/declarative_sources.py (1)
airbyte_cdk/test/standard_tests/_job_runner.py (2)
airbyte_cdk/test/standard_tests/docker_base.py (1)
airbyte_cdk/test/standard_tests/source_base.py (2)
airbyte_cdk/test/models/scenario.py (1)
🪛 GitHub Actions: Lintersairbyte_cdk/test/standard_tests/docker_base.py[warning] 1-1: Ruff format would reformat the file. Changes suggested in diff shown. airbyte_cdk/test/models/scenario.py[warning] 1-1: Ruff format would reformat the file. Changes suggested in diff shown. ⏰ 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). (12)
🔇 Additional comments (22)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
PyTest Results (Fast)3 772 tests ±0 3 760 ✅ ±0 6m 2s ⏱️ -13s Results for commit 5ca54c9. ± Comparison against base commit 25132b6. This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.This pull request removes 1 skipped test and adds 1 skipped test. Note that renamed tests count towards both. |
PyTest Results (Full)3 775 tests ±0 3 763 ✅ ±0 10m 59s ⏱️ -3s Results for commit 5ca54c9. ± Comparison against base commit 25132b6. This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.This pull request removes 1 skipped test and adds 1 skipped test. Note that renamed tests count towards both. |
| expect_records: AcceptanceTestExpectRecords | None = None | ||
| file_types: AcceptanceTestFileTypes | None = None | ||
| status: Literal["succeed", "failed", "exception"] | None = None | ||
| suggested_streams: list[str] | None = Field(default=None, kw_only=True) |
There was a problem hiding this comment.
it feels strange to support this if we want to encourage the suggested streams to be defined in a single place elsewhere in the metadata file. Do we need this?
There was a problem hiding this comment.
👍 suggestedStreams is actually already a thing and it happens to already be defined in metadata.yaml.
I'll find an example to post here
There was a problem hiding this comment.
Oh. Do you mean it's weird to define inside the scenario?
Good point, this isn't intended to be redefined - only passed in by the caller that has access to the full file. I'll clarify that
There was a problem hiding this comment.
yeah I figured the caller would only need the boolean suggested_streams_only to tell the test suite whether to pull the suggested streams out of the metadata file
| suggested_streams: list[str] | None = Field(default=None, kw_only=True) | ||
| """List of suggested stream names for the connector (if provided).""" | ||
|
|
||
| configured_catalog_path: Path | None = Field(default=None, kw_only=True) |
There was a problem hiding this comment.
how does passing a configured catalog intersect with some of the other options to specify which streams to sync?
Also, IIRC you were a fan of not including the option to specify a catalog. In what cases would this be specified?
| class AcceptanceTestEmptyStream(BaseModel): | ||
| name: str | ||
| bypass_reason: str | None = None | ||
| expect_failure: bool = Field(default=False, kw_only=True) |
There was a problem hiding this comment.
I think we had previously discussed the possibility of specifying which step [spec, check, discover, read] we expected to fail. I'm all for keeping it simple, so I prefer this, but I wonder if others on the team want that additional flexibility?
There was a problem hiding this comment.
I considered that too. Thanks for raising.
My concern is that tests today are not always clear in regards to where the failure should happen. If we wanted to add more control later, we could still do so.
We could do so either by extending to accept bool | Literal["check", "discover", "read"] or maybe even bool | dict if we wanted to accept something like expect_failure.message_contains`.
Because the bool alone handles what I think the immediate current migration scenarios would be from existing test definitions, I'm inclined to keep it there unless anyone wants to advocate for a specific spec for this first iteration. (Assuming this starting specification is suitably forwards compatible.)
|
|
||
| @property | ||
| def expected_outcome(self) -> ExpectedOutcome: | ||
| """Whether the test scenario expects an exception to be raised. |
There was a problem hiding this comment.
the docstring doesn't seem to match what the method is called
There was a problem hiding this comment.
Good catch. This is carry over, I think. I'll clean it up.
Replace the existing configuration DSL with a simpler smoke-tests DSL and add relevant documentation to support the changes.
Summary by CodeRabbit