SAT: improve check_if_cursor_field_was_changed to make it less strict
#15835
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alafanechere
changed the title
alafanechere
changed the title
check_if_cursor_field_was_changed to make it less radical
alafanechere
changed the title
check_if_cursor_field_was_changed to make it less radicalcheck_if_cursor_field_was_changed to make it less strict
alafanechere
commented
Aug 22, 2022
| @@ -944,7 +944,7 @@ def as_pytest_param(self): | |||
| # Checking that all transitions in FAILING_SPEC_TRANSITIONS have should_fail == True to prevent typos | |||
| assert all([transition.should_fail for transition in FAILING_SPEC_TRANSITIONS]) | |||
| # Checking that all transitions in VALID_SPEC_TRANSITIONS have should_fail = False to prevent typos | |||
| assert not all([transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) | |||
| assert not any([transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) | |||
There was a problem hiding this comment.
This assertion was not working correctly.
There was a problem hiding this comment.
perhaps assert all([not transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) is more readable?
Phlair
reviewed
Aug 22, 2022
| @@ -227,5 +227,6 @@ def check_if_stream_was_removed(self, diff: DeepDiff): | |||
|
|
|||
| def check_if_cursor_field_was_changed(self, diff: DeepDiff): | |||
| """Check if a default cursor field value was changed.""" | |||
| if diff: | |||
| invalid_changes = {"values_changed", "iterable_item_added", "iterable_item_removed"} | |||
| if diff and set(diff.keys()).issubset(invalid_changes): | |||
There was a problem hiding this comment.
could this now pass in a case where we have a new stream but also breaking cursor field change on another stream?
There was a problem hiding this comment.
Thank for the suggestion, I added a corresponding test case and discovered the check was not robust enough: improved in 3231e6d
| @@ -944,7 +944,7 @@ def as_pytest_param(self): | |||
| # Checking that all transitions in FAILING_SPEC_TRANSITIONS have should_fail == True to prevent typos | |||
| assert all([transition.should_fail for transition in FAILING_SPEC_TRANSITIONS]) | |||
| # Checking that all transitions in VALID_SPEC_TRANSITIONS have should_fail = False to prevent typos | |||
| assert not all([transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) | |||
| assert not any([transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) | |||
There was a problem hiding this comment.
perhaps assert all([not transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) is more readable?
| @@ -1197,7 +1223,7 @@ def test_validate_previous_configs(previous_connector_spec, actual_connector_spe | |||
| # Checking that all transitions in FAILING_CATALOG_TRANSITIONS have should_fail == True to prevent typos | |||
| assert all([transition.should_fail for transition in FAILING_CATALOG_TRANSITIONS]) | |||
| # Checking that all transitions in VALID_CATALOG_TRANSITIONS have should_fail = False to prevent typos | |||
| assert not all([transition.should_fail for transition in VALID_CATALOG_TRANSITIONS]) | |||
| assert not any([transition.should_fail for transition in VALID_CATALOG_TRANSITIONS]) | |||
|
/publish connector=bases/source-acceptance-test auto-bump-version=false
if you have connectors that successfully published but failed definition generation, follow step 4 here |
alafanechere
deleted the
augustin/sat/fix-test-discovery-backward-compat
branch
13 tasks
rodireich
pushed a commit
that referenced
this pull request
Aug 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Closing #15692
The current implementation of
check_if_cursor_field_was_changedis too strict: it fails if a new stream is added to the catalog:.How
airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py
Lines 228 to 232 in 26a4e19
Recommended reading order
test_backward_compatibility.pybackward_compatibility.py🚨 User Impact 🚨
PRs adding new streams to a connector catalog should have their acceptance test pass now.