feat: Add pytest warnings-as-errors configuration similar to PyAirbyte #765
Conversation
- Add warnings-as-errors configuration to pytest.ini following PyAirbyte pattern - Include detailed inline comments explaining each warning type and locations - Fix jsonschema import deprecation in transform.py to use protocols import - Ignore specific acceptable warnings: ExperimentalClassWarning, IncrementalMixin deprecation, jsonschema import deprecation, unknown pytest markers - Successfully tested with pytest --collect-only (3750 tests collected) Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
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. 📝 WalkthroughWalkthroughChanged how the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Transform as transform.py
participant JSONSchema as jsonschema lib
participant Fallback as protocols module
Note over Transform,JSONSchema: import attempt (primary)
Transform->>JSONSchema: from jsonschema.validators import Validator
alt primary succeeds
JSONSchema-->>Transform: Validator (success)
else primary ImportError / attribute missing
Note over Transform,Fallback: fallback guarded by ImportError
Transform->>Fallback: from jsonschema.protocols import Validator
alt fallback succeeds
Fallback-->>Transform: Validator (fallback)
else fallback ImportError
Fallback-->>Transform: ImportError raised
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes. Would you like to pin a minimum Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
👋 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/1758063256-pytest-warnings-as-errors#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/1758063256-pytest-warnings-as-errorsHelpful 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 implements a "warnings as errors" configuration for pytest, following the PyAirbyte repository pattern, and fixes a jsonschema import deprecation warning. The changes enhance test reliability by treating Python warnings as errors while maintaining specific ignore patterns for acceptable warnings.
- Enable warnings-as-errors for pytest with detailed ignore patterns for 4 categories of acceptable warnings
- Fix jsonschema import deprecation by updating to use the protocols module
- Add comprehensive documentation for each warning type and its source locations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pytest.ini | Adds warnings-as-errors configuration with specific ignore patterns for experimental features, deprecations, and pytest markers |
| airbyte_cdk/sources/utils/transform.py | Updates jsonschema import to use protocols module and improves exception handling specificity |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| except: | ||
| from jsonschema import Validator | ||
| except ImportError: | ||
| from jsonschema.protocols import Validator |
There was a problem hiding this comment.
The fallback import path may be incorrect. The original code imported from jsonschema directly, but the new fallback imports from jsonschema.protocols. If jsonschema.validators is unavailable, jsonschema.protocols might also be unavailable in older versions. Consider falling back to from jsonschema import Validator to maintain backward compatibility.
| from jsonschema.protocols import Validator | |
| from jsonschema import Validator |
Copilot uses AI. Check for mistakes.
PyTest Results (Fast)218 tests 208 ✅ 51s ⏱️ For more details on these failures, see this check. Results for commit 3e99b78. ♻️ This comment has been updated with latest results. |
PyTest Results (Full)3 750 tests 3 308 ✅ 11m 48s ⏱️ For more details on these failures, see this check. Results for commit 766ef94. ♻️ This comment has been updated with latest results. |
- Resolves CI test failures caused by DeprecationWarning from logger.warn() method - Warning occurs in unit_tests/test_logger.py:81 where test intentionally uses deprecated method - Follows established pattern of detailed inline comments for each ignored warning Co-Authored-By: AJ Steers <[email protected]>
![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 (3)
pytest.ini (3)
12-15: Prefer class-based filter over message text for ExperimentalClassWarning.Message text is brittle; could we target the warning class instead to avoid breakage if wording changes, wdyt?
Apply:
- ignore:This class is experimental. Use at your own risk. + ignore::airbyte_cdk.sources.source.ExperimentalClassWarning
16-20: Narrow the IncrementalMixin deprecation ignore to specific modules.To prevent masking other deprecations, would you scope the ignore with a module regex for the two known files, wdyt?
Example:
- ignore:Deprecated as of CDK version 0.87.0. Deprecated in favor of the `CheckpointMixin`:DeprecationWarning + ignore:Deprecated as of CDK version 0.87.0. Deprecated in favor of the `CheckpointMixin`:DeprecationWarning:airbyte_cdk\.sources\.file_based\.stream\.default_file_based_stream + ignore:Deprecated as of CDK version 0.87.0. Deprecated in favor of the `CheckpointMixin`:DeprecationWarning:unit_tests\.sources\.mock_server_tests\.mock_source_fixture
25-27: Scope the logger.warn deprecation ignore to the one test (or mark it in-test).Global ignore can hide real usages; would you move this to the test via
@pytest.mark.filterwarnings(...)or at least add a module regex here, wdyt?Inline option in unit_tests/test_logger.py:
import pytest pytestmark = pytest.mark.filterwarnings("ignore:The 'warn' method is deprecated, use 'warning' instead:DeprecationWarning")Or narrow in pytest.ini:
- ignore:The 'warn' method is deprecated, use 'warning' instead:DeprecationWarning + ignore:The 'warn' method is deprecated, use 'warning' instead:DeprecationWarning:unit_tests\.test_logger$
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/utils/transform.py(1 hunks)pytest.ini(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte_cdk/sources/utils/transform.py (1)
airbyte_cdk/sources/declarative/validators/validator.py (1)
Validator(9-18)
⏰ 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)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Build and Inspect Python Package
- GitHub Check: Ruff Lint Check
- GitHub Check: Dependency Analysis with Deptry
- GitHub Check: MyPy Check
- GitHub Check: Ruff Format Check
- GitHub Check: preview_docs
🔇 Additional comments (5)
pytest.ini (4)
8-10: Good call: make warnings fail the build.This aligns well with PyAirbyte and will catch regressions early. wdyt?
If helpful, I can scan for common warning sources (e.g., logger.warn, deprecated imports) across the repo to ensure no hidden offenders remain. Want a quick script?
31-34: Nice addition of requires_creds marker.Thanks for documenting it here. Do you want to scan for any other ad hoc markers to declare while we’re here, wdyt?
I can extend the script above to open a checklist for any undeclared markers you want to keep.
21-24: Remove pytest.ini ignore for jsonschema Validator deprecation?rg shows only submodule imports in airbyte_cdk/sources/utils/transform.py (lines 21–23):
from jsonschema.validators import Validatorwith aprotocolsfallback; no top-levelfrom jsonschema import Validatorfound, so the ignore appears stale — wdyt?
29-30: Declare missing pytest markers before removing blanket ignoreFile: pytest.ini lines 29-30 — there’s an
ignore::pytest.PytestUnknownMarkWarning. My scan shows many used markers (e.g. requires_creds, image_tests, slow, asyncio) but no declared markers; can we add the missing markers to pytest.ini (or setup.cfg/tox.ini) and then drop the ignore so typoed/new markers fail fast, wdyt?airbyte_cdk/sources/utils/transform.py (1)
22-23: Prefer jsonschema.protocols.Validator with legacy fallbackAdd a fallback import order (protocols → validators → top-level) to avoid ImportError across jsonschema versions — can you confirm the repo's pinned jsonschema range (I couldn't find it in common manifests)? wdyt?
File: airbyte_cdk/sources/utils/transform.py (lines 22-23)
-try: - from jsonschema.validators import Validator -except ImportError: - from jsonschema.protocols import Validator +try: + # Preferred path on modern jsonschema (v4+) + from jsonschema.protocols import Validator +except ImportError: + try: + # Some 4.x releases export Validator here + from jsonschema.validators import Validator + except ImportError: + # Legacy fallback for older environments; deprecation is handled by pytest filters + from jsonschema import Validator # noqa: F401rg -n "jsonschema\s*[<>=~!]" -S pyproject.toml setup.cfg setup.py requirements*.txt || true
…ors configuration - Systematically identified and documented all warnings triggered during test execution - Added HttpRequester field deprecation warnings (path and url_base deprecated in favor of url) - Added detailed inline comments explaining each ignored warning and its source - Follows PyAirbyte pattern of treating warnings as errors with explicit ignore list - Resolves CI test failures across all Python versions (3.10, 3.11, 3.12, 3.13) - Verified locally: test collection (3750 tests), specific test execution, and lint checks all pass Co-Authored-By: AJ Steers <[email protected]>
- Add PytestUnraisableExceptionWarning for unclosed resource warnings in tests - Add PluggyTeardownRaisedWarning for pytest plugin teardown issues - Completes comprehensive filterwarnings configuration for warnings-as-errors - Should resolve remaining 442 CI test failures Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pytest.ini (5)
13-15: Prefer category-scoped ignore over message text for ExperimentalClassWarning.Message text can drift and over-match; the prior category-based ignore is tighter. Replace the message matcher with the warning class, wdyt?
- ignore:This class is experimental. Use at your own risk. + ignore::airbyte_cdk.sources.source.ExperimentalClassWarning
16-20: Scope IncrementalMixin deprecation ignores to the emitting modules.Limiting by module avoids suppressing unrelated DeprecationWarnings that share similar text. Proposed tweak, wdyt?
- ignore:Deprecated as of CDK version 0.87.0. Deprecated in favor of the `CheckpointMixin`:DeprecationWarning + ignore:Deprecated as of CDK version 0.87.0.*CheckpointMixin:DeprecationWarning:airbyte_cdk.sources.file_based.stream.default_file_based_stream + ignore:Deprecated as of CDK version 0.87.0.*CheckpointMixin:DeprecationWarning:unit_tests.sources.mock_server_tests.mock_source_fixture
21-24: Make the jsonschema deprecation ignore less brittle.The exact message text may change between jsonschema releases. Consider a regex that’s resilient but still specific, wdyt?
- ignore:Importing Validator directly from the jsonschema package is deprecated:DeprecationWarning:airbyte_cdk.sources.utils.transform + ignore:Importing Validator.*deprecated:DeprecationWarning:airbyte_cdk.sources.utils.transform
37-44: Be cautious ignoring PytestUnraisableExceptionWarning and PluggyTeardownRaisedWarning.These can mask real leaks or teardown issues. Could we scope them to the known emitting modules/tests or time-box them with a follow-up to fix root causes? For example:
- ignore::pytest.PytestUnraisableExceptionWarning + ignore::pytest.PytestUnraisableExceptionWarning:unit_tests.destinations.test_destinationWe could keep PluggyTeardownRaisedWarning for now but plan to remove once stabilized. Wdyt?
33-35: Declare missing pytest markers (image_tests, limit_memory) and remove the ignore for PytestUnknownMarkWarning?Declared in pytest.ini: slow, asyncio, requires_creds (lines 46–48). Undeclared but used: image_tests, limit_memory — add those to the [markers] section and then remove the ignore entry:
- ignore::pytest.PytestUnknownMarkWarningwdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pytest.ini(1 hunks)
⏰ 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: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
🔇 Additional comments (3)
pytest.ini (3)
8-10: Good call enabling warnings-as-errors.This raises the bar on quality and will catch regressions early. Nice. Wdyt?
25-27: Ack: logger.warn deprecation intentionally ignored for a targeted test.This is scoped and documented; SGTM. Wdyt?
29-31: Ack: HttpRequester field deprecation ignore.Given known deprecations in builder tests, this seems pragmatic and well-scoped. Wdyt?
feat: Add pytest warnings-as-errors configuration similar to PyAirbyte
Summary
This PR implements a "warnings as errors" configuration for pytest in the airbyte-python-cdk repository, following the same pattern used in the PyAirbyte repository. The changes include:
erroras the default action for Python warnings during test execution, with specific ignore rules for acceptable warningsairbyte_cdk/sources/utils/transform.pyto use the newjsonschema.protocolsimport path and improved exception handlingThe configuration successfully handles 4 categories of warnings:
Review & Testing Checklist for Human
Risk Level: 🟡 Moderate - This changes global test behavior and touches core utility code
poetry run poe pytest) to ensure no unexpected failures from warnings-as-errorswarnings.warn("test")) and confirming it causes test failureNotes
pytest --collect-only(3750 tests collected successfully) and limited test executionexcept:toexcept ImportError:- verify this doesn't miss other exception typesSession Details:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests