fix(tests): resolves false-positive failures in FAST tests (defer failure expectations) #587
Conversation
…s false-positive failures)
PyTest Results (Fast)3 656 tests - 2 3 646 ✅ - 2 5m 36s ⏱️ -8s Results for commit 2434501. ± Comparison against base commit 99a1a96. This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)3 659 tests - 2 3 649 ✅ - 2 17m 31s ⏱️ +21s Results for commit 2434501. ± Comparison against base commit 99a1a96. This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…fast-standard-tests-(resolves-false-positive-failures)
aaronsteers
changed the title
…fast-standard-tests-(resolves-false-positive-failures)
aaronsteers
changed the title
|
/format-fix |
📝 WalkthroughWalkthroughThis change standardizes test scenario handling by introducing an Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant Scenario
participant JobRunner
participant EntrypointWrapper
TestSuite->>Scenario: Get config dict (with connector_root)
TestSuite->>JobRunner: run_test_job(..., connector_root, test_scenario)
JobRunner->>EntrypointWrapper: _run_command(..., expected_outcome)
EntrypointWrapper-->>JobRunner: EntrypointOutput
JobRunner-->>TestSuite: EntrypointOutput (assertions based on ExpectedOutcome)
Suggested labels
Would you like me to help review the scenario filtering logic for any edge cases or discuss further refinements to the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/test/standard_tests/connector_base.py (1)
167-184: Consider optimizing duplicate detection for better performance?The explicit filtering loop is much more readable than a complex list comprehension! However, the duplicate checking on lines 178-182 creates an O(n²) scenario since it scans the entire
test_scenarioslist for each new scenario.Would you consider using a set to track seen config paths for better performance, wdyt?
test_scenarios: list[ConnectorTestScenario] = [] + seen_config_paths: set[str] = set() for category in categories: if ( category not in all_tests_config["acceptance_tests"] or "tests" not in all_tests_config["acceptance_tests"][category] ): continue for test in all_tests_config["acceptance_tests"][category]["tests"]: if "config_path" not in test: # Skip tests without a config_path continue if "iam_role" in test["config_path"]: # We skip iam_role tests for now, as they are not supported in the test suite. continue scenario = ConnectorTestScenario.model_validate(test) - if scenario.config_path and scenario.config_path in [ - s.config_path for s in test_scenarios - ]: + if scenario.config_path and scenario.config_path in seen_config_paths: # Skip duplicate scenarios based on config_path continue + if scenario.config_path: + seen_config_paths.add(scenario.config_path) test_scenarios.append(scenario)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/test/standard_tests/connector_base.py(2 hunks)airbyte_cdk/test/standard_tests/util.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (2)
airbyte_cdk/test/standard_tests/util.py (1)
70-70: LGTM! Proper classmethod implementation fix.This correctly wraps the lambda in
classmethod()to handle theclsparameter properly, aligning with howget_connector_root_dir()is called elsewhere in the codebase.airbyte_cdk/test/standard_tests/connector_base.py (1)
119-119: Good addition of required parameter.Adding the
connector_rootparameter aligns with the updatedrun_test_jobsignature and ensures proper path resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/test/entrypoint_wrapper.py (1)
27-27: Remove unused langsmith import?The pipeline failure indicates this import is unused. Since it's flagged as a transitive dependency issue, would you mind removing it unless it's needed for some specific functionality? wdyt?
-from langsmith import expect🧰 Tools
🪛 GitHub Actions: Dependency Analysis
[error] 27-27: 'langsmith' imported but it is a transitive dependency (DEP003) reported by deptry
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
airbyte_cdk/test/entrypoint_wrapper.py(6 hunks)airbyte_cdk/test/standard_tests/_job_runner.py(5 hunks)airbyte_cdk/test/standard_tests/models/scenario.py(5 hunks)airbyte_cdk/test/standard_tests/source_base.py(6 hunks)airbyte_cdk/test/utils/reading.py(2 hunks)unit_tests/sources/declarative/file/test_file_stream.py(3 hunks)unit_tests/sources/mock_server_tests/test_resumable_full_refresh.py(2 hunks)unit_tests/test/test_entrypoint_wrapper.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- unit_tests/test/test_entrypoint_wrapper.py
🚧 Files skipped from review as they are similar to previous changes (3)
- airbyte_cdk/test/standard_tests/source_base.py
- airbyte_cdk/test/standard_tests/models/scenario.py
- airbyte_cdk/test/standard_tests/_job_runner.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
unit_tests/sources/mock_server_tests/test_resumable_full_refresh.py (1)
airbyte_cdk/test/standard_tests/models/scenario.py (1)
ExpectedOutcome(20-50)
🪛 GitHub Actions: Dependency Analysis
airbyte_cdk/test/entrypoint_wrapper.py
[error] 27-27: 'langsmith' imported but it is a transitive dependency (DEP003) reported by deptry
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (15)
unit_tests/sources/mock_server_tests/test_resumable_full_refresh.py (2)
30-30: Good addition of theExpectedOutcomeimport!The import correctly supports the enum-based approach for expressing test expectations.
348-348: Excellent migration to the enum-based expectation!The change from
expecting_exception=Truetoexpected_outcome=ExpectedOutcome.EXPECT_EXCEPTIONmakes the test intention much clearer. This aligns perfectly with the goal of standardizing expected outcome handling across the test suite.airbyte_cdk/test/utils/reading.py (3)
9-9: Proper import addition for the new enum interface!The import correctly supports the standardized outcome handling.
23-23: Excellent interface update with sensible default!The parameter signature change from boolean to enum improves clarity, and the default value of
EXPECT_SUCCESSis appropriate for a utility function. This maintains backward compatibility expectations while adopting the new standardized approach.
27-27: Clean parameter forwarding to the updated interface!The parameter is correctly passed through to the underlying
readfunction using the new enum-based approach.unit_tests/sources/declarative/file/test_file_stream.py (4)
20-20: Great import addition for the standardized outcome handling!The import correctly supports the new enum-based approach for test expectations.
58-59: Excellent use of keyword-only parameter with sensible default!The function signature update properly adopts the enum-based approach, and using a keyword-only parameter (
*,) is a good practice for optional parameters like this. The default value ofEXPECT_SUCCESSis appropriate.
68-68: Clean parameter forwarding in the read function call!The function call correctly uses the new enum-based parameter.
72-81: Well-implemented discover function interface update!The function signature correctly adopts the new
ExpectedOutcomeparameter with appropriate default, and the function call properly forwards the parameter to the underlying entrypoint function.airbyte_cdk/test/entrypoint_wrapper.py (6)
48-48: Perfect import addition for the standardized outcome handling!The import correctly supports the new enum-based approach throughout this core module.
162-165: Excellent function signature modernization!The updates include both the new enum-based parameter and modern type hint (
list[str]instead ofList[str]). The interface is much clearer about expected outcomes.
182-186: Clean logic update using the enum's method!The change from
not expecting_exceptiontoexpected_outcome.expect_success()is much more readable and aligns with the enum's design. The logic flow remains the same while being more explicit about expectations.
192-192: Good use of keyword argument in constructor call!Using the keyword argument for
uncaught_exceptionimproves code clarity.
198-205: Excellent interface update with improved documentation!The function signature correctly adopts the keyword-only parameter approach, and the docstring clearly explains the new parameter's purpose. This makes the API much more user-friendly.
223-231: Perfect consistency in the read function interface!The function signature and docstring updates maintain consistency with the discover function, providing a uniform API across the module.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/test/models/scenario.py (1)
87-93: Update the docstring to match the new return type?The docstring still mentions "Returns True if the scenario expects an exception, False if it does not, and None if there is no set expectation" but the method now returns an
ExpectedOutcomeenum, not boolean/None values. Should we update this to reflect the actual return type, wdyt?- """Whether the test scenario expects an exception to be raised. - - Returns True if the scenario expects an exception, False if it does not, - and None if there is no set expectation. - """ + """Get the expected outcome for this test scenario. + + Returns an ExpectedOutcome enum indicating whether the scenario expects + an exception, success, or allows any outcome. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
airbyte_cdk/test/entrypoint_wrapper.py(5 hunks)airbyte_cdk/test/models/__init__.py(1 hunks)airbyte_cdk/test/models/outcome.py(1 hunks)airbyte_cdk/test/models/scenario.py(5 hunks)airbyte_cdk/test/standard_tests/_job_runner.py(6 hunks)airbyte_cdk/test/standard_tests/connector_base.py(3 hunks)airbyte_cdk/test/standard_tests/declarative_sources.py(2 hunks)airbyte_cdk/test/standard_tests/models/__init__.py(0 hunks)airbyte_cdk/test/standard_tests/source_base.py(8 hunks)airbyte_cdk/test/utils/reading.py(2 hunks)unit_tests/sources/declarative/file/test_file_stream.py(3 hunks)unit_tests/sources/mock_server_tests/test_resumable_full_refresh.py(2 hunks)unit_tests/test/test_entrypoint_wrapper.py(3 hunks)
💤 Files with no reviewable changes (1)
- airbyte_cdk/test/standard_tests/models/init.py
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/test/models/init.py
🚧 Files skipped from review as they are similar to previous changes (9)
- unit_tests/sources/mock_server_tests/test_resumable_full_refresh.py
- airbyte_cdk/test/utils/reading.py
- airbyte_cdk/test/standard_tests/declarative_sources.py
- unit_tests/sources/declarative/file/test_file_stream.py
- unit_tests/test/test_entrypoint_wrapper.py
- airbyte_cdk/test/standard_tests/connector_base.py
- airbyte_cdk/test/standard_tests/source_base.py
- airbyte_cdk/test/entrypoint_wrapper.py
- airbyte_cdk/test/standard_tests/_job_runner.py
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
airbyte_cdk/test/models/outcome.py (3)
15-24: Nice enum design with clear semantics!The tri-state model with
EXPECT_EXCEPTION,EXPECT_SUCCESS, andALLOW_ANYis much more expressive than boolean flags. Usingauto()is also a good practice here. This should make the test expectations much clearer, wdyt?
25-37: Solid error handling in the conversion method!The
from_status_strmethod properly handles the conversion from string status to enum values, and the KeyError -> ValueError transformation with a clear error message is exactly what we want here.
39-45: Clean helper methods for querying expectations.These boolean helper methods provide a nice, readable API for checking the expected outcome state. The implementation is straightforward and correct.
airbyte_cdk/test/models/scenario.py (3)
29-31: Great addition for PyTest parameterization!Making the class frozen and hashable is exactly what's needed for PyTest parameterization. This should eliminate any issues with using scenarios as test parameters.
52-79: Solid path resolution logic with the new connector_root parameter.The addition of the
connector_rootparameter and the path resolution logic for relative paths looks correct. Converting relative paths to absolute ones by resolving against the connector root is the right approach here.
107-140: Excellent fluent API for scenario manipulation!These three new methods (
without_expected_outcome,with_expecting_failure,with_expecting_success) provide a clean, fluent interface for deriving scenarios with different expectations. The conditional logic to avoid unnecessary copying when the status is already set is a nice optimization. This should make test scenario handling much more flexible!
aaronsteers
deleted the
aj/test--remove-expected-to-fail-tests-from-fast-standard-tests-(resolves-false-positive-failures)
branch
Resolves: https://github.com/airbytehq/airbyte-internal-issues/issues/13230
Eliminate tests that are expected to fail from the fast standard tests to prevent false-positive failures in the test suite. This change improves the reliability of test results.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.