Skip to content

Conversation

dbgold17
Copy link
Contributor

@dbgold17 dbgold17 commented Jul 15, 2025

What

fixes: https://github.com/airbytehq/airbyte-internal-issues/issues/13521

This PR attempts to make FAST tests respect the empty_streams key in acceptance-test-config.yml files by choosing not to sync data from those streams. Historically the empty_streams key was used not only to denote streams that don't have any data to sync but also streams that will fail syncs for a variety of reasons. Running tests against these streams has been leading to false positive failures. This PR attempts to replicate the behavior of CATs with a bit less sophistication, fitting with the goals of the FAST test suite.

This implementation somewhat messy due to how the test suite currently parses these files - extracting only a subset of test scenarios to run all operations (spec, check, read) against.

I see this as a temporary fix until we are able to redesign how we want to configure FASTs and deprecate these files.

Summary by CodeRabbit

  • New Features

    • Added support for specifying empty streams with optional bypass reasons in connector test scenarios. Scenarios can now identify streams expected to be empty and exclude them from read tests.
  • Bug Fixes

    • Improved test behavior to correctly filter out empty streams during connector acceptance tests.
  • Refactor

    • Enhanced merging of duplicate test scenarios to preserve all empty stream information.
    • Updated function signatures and internal logic for better configuration handling and code clarity.
    • Removed @dataclass decorator from a custom exception class for simplicity.
  • Chores

    • Removed unused imports for cleaner codebase.

Copy link

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

Testing This CDK Version

You 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@dbgold17/fast-tests-skip-streams#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 dbgold17/fast-tests-skip-streams

Helpful Resources

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /poe <command> - Runs any poe command in the CDK environment

📝 Edit this welcome message.

Copy link

github-actions bot commented Jul 15, 2025

PyTest Results (Fast)

3 695 tests  ±0   3 684 ✅ ±0   6m 23s ⏱️ +5s
    1 suites ±0      11 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit b264d07. ± Comparison against base commit 7d045f7.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 15, 2025

PyTest Results (Full)

3 698 tests  ±0   3 687 ✅ ±0   18m 13s ⏱️ +9s
    1 suites ±0      11 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit b264d07. ± Comparison against base commit 7d045f7.

♻️ This comment has been updated with latest results.

@dbgold17 dbgold17 requested a review from aaronsteers July 15, 2025 23:24
@dbgold17 dbgold17 marked this pull request as ready for review July 15, 2025 23:24
Copy link
Contributor

coderabbitai bot commented Jul 15, 2025

📝 Walkthrough

Walkthrough

The changes introduce a new nested Pydantic model for empty streams in test scenarios, update how acceptance test configurations are loaded and merged, and add logic to filter out empty streams during tests. Additionally, unused imports are removed and a function signature is simplified by eliminating an explicit parameter.

Changes

File(s) Change Summary
airbyte_cdk/test/models/scenario.py Added nested AcceptanceTestEmptyStream Pydantic model and new empty_streams field to ConnectorTestScenario.
airbyte_cdk/test/standard_tests/connector_base.py Removed unused imports.
airbyte_cdk/test/standard_tests/docker_base.py Replaced config path property with a loader/validator, updated scenario merging logic, filtered out empty streams in tests, and removed unused imports.
airbyte_cdk/test/standard_tests/source_base.py Modified test_basic_read to filter out streams listed in empty_streams from the read test.
airbyte_cdk/utils/connector_paths.py Removed connector_directory parameter from resolve_connector_name_and_directory function, making it a local variable instead.
airbyte_cdk/test/entrypoint_wrapper.py Removed @dataclass decorator from AirbyteEntrypointException class.

Sequence Diagram(s)

sequenceDiagram
    participant TestRunner
    participant DockerConnectorTestSuite
    participant AcceptanceTestConfig
    participant Scenario
    participant Source

    TestRunner->>DockerConnectorTestSuite: get_scenarios()
    DockerConnectorTestSuite->>AcceptanceTestConfig: Load and validate config
    AcceptanceTestConfig-->>DockerConnectorTestSuite: Return scenarios (with empty_streams)
    DockerConnectorTestSuite->>DockerConnectorTestSuite: Merge duplicate scenarios, combine empty_streams

    TestRunner->>DockerConnectorTestSuite: test_docker_image_build_and_read(scenario)
    DockerConnectorTestSuite->>Scenario: Filter out streams in empty_streams
    DockerConnectorTestSuite->>Source: Read remaining streams
Loading

Suggested labels

testing

Suggested reviewers

  • aaronsteers

Would you consider adding some documentation or usage examples for the new empty_streams field in test scenarios, wdyt?

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🔭 Outside diff range comments (1)
airbyte_cdk/utils/connector_paths.py (1)

87-105: Update documentation for the breaking change in resolve_connector_name_and_directory signature

We’ve checked all internal callers of resolve_connector_name_and_directory and found none passing the removed connector_directory parameter:

  • airbyte_cdk/cli/airbyte_cdk/_connector.py
  • airbyte_cdk/cli/airbyte_cdk/_image.py
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py

Since this is a public API change, could you please update the changelog or release notes to document the removal of the connector_directory parameter (and consider bumping the major version to reflect the breaking change)? wdyt?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7d045f7 and e92f948.

📒 Files selected for processing (5)
  • airbyte_cdk/test/models/scenario.py (1 hunks)
  • airbyte_cdk/test/standard_tests/connector_base.py (0 hunks)
  • airbyte_cdk/test/standard_tests/docker_base.py (5 hunks)
  • airbyte_cdk/test/standard_tests/source_base.py (1 hunks)
  • airbyte_cdk/utils/connector_paths.py (1 hunks)
💤 Files with no reviewable changes (1)
  • airbyte_cdk/test/standard_tests/connector_base.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, ignore all `__init__.py` files when providing a recommended reviewing order.
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the `declarative_component_schema.py` file is auto-generated from `declarative_component_schema.yaml` and should be ignored in the recommended reviewing order.
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.
airbyte_cdk/utils/connector_paths.py (2)
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
airbyte_cdk/test/models/scenario.py (2)
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py:21-29
Timestamp: 2025-01-13T23:39:15.457Z
Learning: The CustomPageIncrement class in unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py is imported from another connector definition and should not be modified in this context.
airbyte_cdk/test/standard_tests/source_base.py (3)
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.
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
airbyte_cdk/test/standard_tests/docker_base.py (4)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#90
File: Dockerfile:16-21
Timestamp: 2024-12-02T18:36:04.346Z
Learning: Copying files from `site-packages` in the Dockerfile maintains compatibility with both the old file structure that manifest-only connectors expect and the new package-based structure where SDM is part of the CDK.
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.
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: pyproject.toml:108-111
Timestamp: 2024-11-15T00:58:15.446Z
Learning: In the project, the `run` function is defined in `airbyte_cdk/cli/source_declarative_manifest/_run.py` and is imported into the module's `__init__.py`.
🧬 Code Graph Analysis (2)
airbyte_cdk/test/standard_tests/source_base.py (2)
airbyte_cdk/test/entrypoint_wrapper.py (1)
  • catalog (237-241)
airbyte_cdk/test/utils/reading.py (1)
  • catalog (12-14)
airbyte_cdk/test/standard_tests/docker_base.py (1)
airbyte_cdk/test/models/scenario.py (1)
  • ConnectorTestScenario (27-184)
🪛 GitHub Actions: Linters
airbyte_cdk/test/standard_tests/docker_base.py

[error] 66-80: Ruff formatting check failed. The file needs reformatting to comply with style rules.


[error] 130-140: Ruff formatting check failed. The file needs reformatting to comply with style rules.

⏰ 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). (4)
  • GitHub Check: Check: source-shopify
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (1)
airbyte_cdk/test/models/scenario.py (1)

47-57: Well-designed flexible empty streams configuration!

The implementation elegantly supports both simple string lists for stream names and detailed configurations with bypass reasons. This provides good backward compatibility while enabling richer test scenario definitions.

@dbgold17 dbgold17 changed the title Dbgold17/fast tests skip streams fix: fast tests conditionally skip streams Jul 15, 2025
@github-actions github-actions bot added bug Something isn't working security labels Jul 15, 2025
@dbgold17
Copy link
Contributor Author

/format-fix

@dbgold17
Copy link
Contributor Author

Just got tests passing on source-monday locally which expects the suite to bypass the teams stream!

I'm going to test this out on a couple other connectors in the next day or so

@dbgold17 dbgold17 requested a review from brianjlai July 17, 2025 00:29
Copy link
Contributor

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, @dbgold17! Thanks for creating this. One comment on deduping, but honestly even without the deduping, the code should work correct (deduped in a later step).

@aaronsteers
Copy link
Contributor

Just got tests passing on source-monday locally which expects the suite to bypass the teams stream!

I'm going to test this out on a couple other connectors in the next day or so

Awesome! 🎉

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 606553e and b24e6a2.

📒 Files selected for processing (3)
  • airbyte_cdk/test/models/scenario.py (1 hunks)
  • airbyte_cdk/test/standard_tests/docker_base.py (5 hunks)
  • airbyte_cdk/test/standard_tests/source_base.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • airbyte_cdk/test/standard_tests/source_base.py
  • airbyte_cdk/test/models/scenario.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
airbyte_cdk/test/standard_tests/docker_base.py (7)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#90
File: Dockerfile:16-21
Timestamp: 2024-12-02T18:36:04.346Z
Learning: Copying files from `site-packages` in the Dockerfile maintains compatibility with both the old file structure that manifest-only connectors expect and the new package-based structure where SDM is part of the CDK.
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py:21-29
Timestamp: 2025-01-13T23:39:15.457Z
Learning: The CustomPageIncrement class in unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py is imported from another connector definition and should not be modified in this context.
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, ignore all `__init__.py` files when providing a recommended reviewing order.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: pyproject.toml:108-111
Timestamp: 2024-11-15T00:58:15.446Z
Learning: In the project, the `run` function is defined in `airbyte_cdk/cli/source_declarative_manifest/_run.py` and is imported into the module's `__init__.py`.
🧬 Code Graph Analysis (1)
airbyte_cdk/test/standard_tests/docker_base.py (1)
airbyte_cdk/test/models/scenario.py (1)
  • ConnectorTestScenario (27-188)
🪛 GitHub Actions: Linters
airbyte_cdk/test/standard_tests/docker_base.py

[error] 85-85: mypy: Returning Any from function declared to return "dict[str, object]" [no-any-return]

⏰ 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-hardcoded-records
  • GitHub Check: Check: destination-motherduck
  • GitHub Check: Check: source-intercom
  • GitHub Check: Check: source-shopify
  • GitHub Check: Analyze (python)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: SDM Docker Image Build
🔇 Additional comments (3)
airbyte_cdk/test/standard_tests/docker_base.py (3)

87-117: Excellent deduplication logic!

This implementation elegantly handles the scenario merging with proper immutability patterns. I particularly like how you're taking the union of empty_streams to ensure comprehensive coverage. The use of set() for deduplication and model_copy() for immutable updates follows best practices perfectly.


129-163: Great integration of the new empty streams logic!

The addition of "basic_read" to the categories and the deduplication call nicely ties together the PR objectives. This ensures FAST tests now properly respect the empty_streams configuration from acceptance-test-config.yml files, which should eliminate those false positive failures you mentioned.


366-369: Clean empty streams filtering implementation!

This filtering logic is much cleaner than the set difference approach. The list comprehension with .name attribute access properly handles the AcceptanceTestEmptyStream objects and aligns perfectly with the PR's goal of conditionally skipping problematic streams in FAST tests.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
airbyte_cdk/test/standard_tests/docker_base.py (1)

66-85: Skip - Already addressed in past reviews

🧹 Nitpick comments (1)
airbyte_cdk/test/standard_tests/docker_base.py (1)

13-13: Remove unused cast import?

I notice cast is imported but doesn't appear to be used in the modified code. Would you consider removing it from the imports to keep them clean, wdyt?

-from typing import Any, Literal, cast
+from typing import Any, Literal
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b24e6a2 and b264d07.

📒 Files selected for processing (1)
  • airbyte_cdk/test/standard_tests/docker_base.py (6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
airbyte_cdk/test/standard_tests/docker_base.py (7)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#90
File: Dockerfile:16-21
Timestamp: 2024-12-02T18:36:04.346Z
Learning: Copying files from `site-packages` in the Dockerfile maintains compatibility with both the old file structure that manifest-only connectors expect and the new package-based structure where SDM is part of the CDK.
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.
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py:21-29
Timestamp: 2025-01-13T23:39:15.457Z
Learning: The CustomPageIncrement class in unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py is imported from another connector definition and should not be modified in this context.
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, ignore all `__init__.py` files when providing a recommended reviewing order.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: pyproject.toml:108-111
Timestamp: 2024-11-15T00:58:15.446Z
Learning: In the project, the `run` function is defined in `airbyte_cdk/cli/source_declarative_manifest/_run.py` and is imported into the module's `__init__.py`.
🧬 Code Graph Analysis (1)
airbyte_cdk/test/standard_tests/docker_base.py (1)
airbyte_cdk/test/models/scenario.py (1)
  • ConnectorTestScenario (27-188)
⏰ 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-intercom
  • GitHub Check: Check: source-pokeapi
  • GitHub Check: Check: destination-motherduck
  • GitHub Check: Check: source-shopify
  • GitHub Check: Check: source-hardcoded-records
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/test/standard_tests/docker_base.py (3)

87-117: Well-implemented deduplication logic!

The implementation correctly handles the merging of scenarios with the same config_path while respecting the immutability of the ConnectorTestScenario model. The use of set() to deduplicate empty_streams is a nice touch.


129-163: Good implementation of empty_streams support!

The changes correctly extend the scenario loading to include the "basic_read" section where empty_streams are defined, and the deduplication ensures we get a unified view of empty streams across all test categories.


366-370: Correct implementation of empty streams filtering!

The filtering logic properly handles the AcceptanceTestEmptyStream objects by extracting their names before filtering. This aligns well with the PR's objective to skip syncing data from streams marked as empty.

@dbgold17 dbgold17 merged commit 7504a59 into main Jul 18, 2025
25 checks passed
@dbgold17 dbgold17 deleted the dbgold17/fast-tests-skip-streams branch July 18, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants