Source-acceptance-test: improvements #3888
Merged
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
vovavovavovavova
requested review from
keu,
htrueman,
po3na4skld,
vitaliizazmic,
yevhenii-ldv and
Zirochkaa
and removed request for
michel-tricot and
sherifnada
keu
reviewed
Jun 4, 2021
| @@ -51,11 +51,12 @@ def future_state_fixture(future_state_path) -> Path: | |||
|
|
|||
| @pytest.fixture(name="cursor_paths") | |||
| def cursor_paths_fixture(inputs, configured_catalog_for_incremental) -> Mapping[str, Any]: | |||
| cursor_paths = getattr(inputs, "cursor_paths") | |||
| cursor_paths = getattr(inputs, "cursor_paths", None) | |||
There was a problem hiding this comment.
Suggested change
| cursor_paths = getattr(inputs, "cursor_paths", None) | |
| cursor_paths = getattr(inputs, "cursor_paths", {}) |
keu
reviewed
Jun 4, 2021
| result = {} | ||
|
|
||
| for stream in configured_catalog_for_incremental.streams: | ||
| path = cursor_paths.get(stream.stream.name, [stream.cursor_field[-1]]) | ||
| default_ = [stream.cursor_field[-1]] |
There was a problem hiding this comment.
then you don't need to change anythin here
sherifnada
approved these changes
Jun 4, 2021
| stream.cursor_field = stream.stream.default_cursor_field[:] | ||
| else: | ||
| pytest.fail( | ||
| "Configured catalog should have `cursor_field` " "either `default_cursor_field` specified for all incremental streams" |
There was a problem hiding this comment.
suggested wording improvement
Suggested change
| "Configured catalog should have `cursor_field` " "either `default_cursor_field` specified for all incremental streams" | |
| f"All incremental streams should either have `cursor_field` declared in the configured catalog or `default_cursor_field` specified in the catalog output by discover. Stream {stream.stream.name} does not have either property defined." |
| @@ -51,7 +51,7 @@ def future_state_fixture(future_state_path) -> Path: | |||
|
|
|||
| @pytest.fixture(name="cursor_paths") | |||
| def cursor_paths_fixture(inputs, configured_catalog_for_incremental) -> Mapping[str, Any]: | |||
| cursor_paths = getattr(inputs, "cursor_paths") | |||
| cursor_paths = getattr(inputs, "cursor_paths") or {} | |||
There was a problem hiding this comment.
what is the impact of this change?
There was a problem hiding this comment.
cursor_paths = getattr(inputs, "cursor_paths") returns None if cursor_paths are commented in yaml. This case will fail few lines below on cursor_paths.get
vovavovavovavova
changed the title
vovavovavovavova
changed the title
vovavovavovavova
force-pushed
the
valdemar/#3467_sat_improvements
branch
from
00091f8
to
64f75ec
Compare
sherifnada
approved these changes
Jun 7, 2021
|
/publish connector=bases/source-acceptance-test
|
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.
Closes #3467
How
By solving listed issues described in the issue.
Please note, that solving (3) issue makes old
acceptance-test-config.ymlfiles incompatible, because it contains oldstate_pathfield and does not contain requiredfuture_state_path. Config field need to be overwritten.