CAT: add validation for stream statuses #34675

artem1205 · 2024-01-30T18:31:32Z

What

Resolve #33565

How

add validation for stream statuses

🚨 User Impact 🚨

no breaking changes

Pre-merge Actions

Updating the Python CDK

Airbyter

Before merging:

Pull Request description explains what problem it is solving
Code change is unit tested
Build and my-py check pass
Smoke test the change on at least one affected connector
- On Github: Run this workflow, passing --use-local-cdk --name=source-<connector> as options
- Locally: airbyte-ci connectors --use-local-cdk --name=source-<connector> test
PR is reviewed and approved

After merging:

Publish the CDK
- The CDK does not follow proper semantic versioning. Choose minor if this the change has significant user impact or is a breaking change. Choose patch otherwise.
- Write a thoughtful changelog message so we know what was updated.
Merge the platform PR that was auto-created for updating the Connector Builder's CDK version
- This step is optional if the change does not affect the connector builder or declarative connectors.

vercel · 2024-01-30T18:31:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Feb 8, 2024 9:00pm

artem1205 · 2024-01-31T11:15:39Z

Source Google sheets FAILED as was predicted in #33565 (comment) :

airbyte-ci connectors --name=source-google-sheets test --only-step="acceptance" --acceptance.-k=test_read

Source Klaviyo: SUCCEEDED

# Conflicts: # airbyte-integrations/bases/connector-acceptance-test/CHANGELOG.md

natikgadzhi

This looks good to me. @maxi297 does this unblock your work for Stripe? Should we merge it today then?

natikgadzhi · 2024-02-02T19:19:25Z

airbyte-integrations/bases/connector-acceptance-test/unit_tests/test_asserts.py

@@ -136,7 +131,7 @@ def test_verify_records_schema(configured_catalog: ConfiguredAirbyteCatalog):
 )
 def test_verify_records_schema_with_fail_on_extra_columns(configured_catalog: ConfiguredAirbyteCatalog, json_schema, record, should_fail):
    """Test that fail_on_extra_columns works correctly with nested objects, array of objects"""
-    configured_catalog.streams[0].stream.json_schema =json_schema


Out of curiosity, what formatter do you personally use, and what formatter does most of the team have?

@alafanechere works on setting up requirements on new PRs including type checks with mypy, and I'm considering a formatter as well.

what formatter do you personally use?

Mainly airbyte-ci format fix all (but it is needed to remove unit_test folder from excluded list in pyproject.toml

@natikgadzhi we already have a repo wide formatter (airbyte-ci format fix all/python/js/java) which runs in CI.
We can double the same rules at the connector level if we want to localize the formatting.

maxi297

I would like to understand the deploy process for this change

maxi297 · 2024-02-05T13:49:54Z

airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/config.py

@@ -188,6 +188,7 @@ class BasicReadTestConfig(BaseConfig):
    )
    expect_records: Optional[ExpectedRecordsConfig] = Field(description="Expected records from the read")
    validate_schema: bool = Field(True, description="Ensure that records match the schema of the corresponding stream")
+    validate_stream_statuses: bool = Field(True, description="Ensure that all streams emit status messages")


This means that it is a breaking change. What is the plan for updating the sources so that CATs pass after this is merged?

We expect that all certified connectors will pass this test.

What is the plan for updating the sources so that CATs pass after this is merged?

Our connector-health engineer will take care of it.

Can you be more explicit about this? I want to understand is CATs will be red for a while. We have a goal for Q1 which is to have CATs be green for certified connectors. This includes having a process which ensure that connectors are passing CATs

maxi297 · 2024-02-05T15:49:01Z

To answer @natikgadzhi on #34675 (review): No it does not as the change that affected my PR is not related to this. I've tried to remove the validation but not CATs are failing for another reason. I'll work on unblocking some people on other issues and I'll come back to that

maxi297

Can you please address those concerns?

maxi297 · 2024-02-07T13:37:45Z

airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/config.py

@@ -188,6 +188,7 @@ class BasicReadTestConfig(BaseConfig):
    )
    expect_records: Optional[ExpectedRecordsConfig] = Field(description="Expected records from the read")
    validate_schema: bool = Field(True, description="Ensure that records match the schema of the corresponding stream")
+    validate_stream_statuses: bool = Field(True, description="Ensure that all streams emit status messages")


Can you be more explicit about this? I want to understand is CATs will be red for a while. We have a goal for Q1 which is to have CATs be green for certified connectors. This includes having a process which ensure that connectors are passing CATs

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py

maxi297 · 2024-02-07T13:44:19Z

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py

+        if not inputs.validate_stream_statuses and test_strictness_level is Config.TestStrictnessLevel.high:
+            pytest.fail("High strictness level error: validate_stream_statuses must be set to true in the basic read test configuration.")
+        else:
+            return inputs.validate_stream_statuses


There is a grooming note that mentions that we should not apply this by default for community connectors. Right now, the default value for validate_stream_statuses is True. Does isn't that contradictory?

fixed, this test will not run if ql < 400

With the new change, does that mean that if we want to enable this for ql < 400, we just can't? It feels like we should be able to test this even if the connector is ql < 400

in this case, do we need to set it to False as default, and force to True for certified (ql > 400) connectors?

Can we set it to "None" so that we have three states:

Not explicitly defined which means ql > 400 are enabled but not the rest

False would fail for ql > 400 but skip for non-certified

True would work for everyone

Good point!
fixed.

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py

airbyte-integrations/bases/connector-acceptance-test/unit_tests/test_core.py

artem1205 · 2024-02-07T20:40:32Z

@maxi297 ,

I want to understand is CATs will be red for a while. We have a goal for Q1 which is to have CATs be green for certified connectors. This includes having a process which ensure that connectors are passing CATs

I think all certified connectors should be good with this test, otherwise we had to check them and fix or skip this test.

maxi297

One last comment and I think I'm good

maxi297 · 2024-02-08T17:10:46Z

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py

+        if not inputs.validate_stream_statuses and test_strictness_level is Config.TestStrictnessLevel.high:
+            pytest.fail("High strictness level error: validate_stream_statuses must be set to true in the basic read test configuration.")
+        else:
+            return inputs.validate_stream_statuses


With the new change, does that mean that if we want to enable this for ql < 400, we just can't? It feels like we should be able to test this even if the connector is ql < 400

# Conflicts: # airbyte-integrations/bases/connector-acceptance-test/CHANGELOG.md

maxi297

🚢

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py

CAT: add validation for stream statuses

aaa6e58

CAT: fix AdditionalPropertiesValidator

6bf54af

maxi297 mentioned this pull request Jan 30, 2024

✨ Source Stripe: Enable concurrency on incremental syncs for balance_transactions, events, files, file_links and shipping_rates #34619

Merged

artem1205 added 4 commits January 30, 2024 20:57

CAT: add unit tests

2b49825

CAT: format test

fa03b42

CAT: add test

e1d73f7

CAT: format

46f30c0

artem1205 self-assigned this Jan 30, 2024

artem1205 added 5 commits January 31, 2024 10:42

CAT: update fix

3cb3ade

CAT: format

f7b7bd0

CAT: update doc

229236d

CAT:fix

208e6bd

CAT: add test

6c9d88d

artem1205 marked this pull request as ready for review January 31, 2024 12:20

artem1205 requested review from lazebnyi and oustynova as code owners January 31, 2024 12:20

artem1205 requested a review from a team January 31, 2024 12:20

artem1205 mentioned this pull request Jan 31, 2024

Add CAT scenario validating that a sync emits at least one stream status for a successful sync #33565

Closed

3 tasks

maxi297 mentioned this pull request Jan 31, 2024

✨ Source Stripe: Enable concurrency on incremental syncs for balance_transactions, files, file_links and shipping_rates #34696

Merged

lazebnyi approved these changes Feb 2, 2024

View reviewed changes

lazebnyi requested review from maxi297 and removed request for a team February 2, 2024 13:04

artem1205 added 2 commits February 2, 2024 17:42

Merge remote-tracking branch 'origin/master' into artem1205/CAT-33565

7605a2c

# Conflicts: # airbyte-integrations/bases/connector-acceptance-test/CHANGELOG.md

CAT: bump version

b7208b7

natikgadzhi approved these changes Feb 2, 2024

View reviewed changes

maxi297 reviewed Feb 5, 2024

View reviewed changes

artem1205 requested a review from maxi297 February 5, 2024 18:35

maxi297 requested changes Feb 7, 2024

View reviewed changes

artem1205 added 4 commits February 7, 2024 17:45

Merge remote-tracking branch 'origin/master' into artem1205/CAT-33565

80304f3

CAT: enable test only for high strictness level

0236b60

CAT: enable test only for high strictness level

9153f5a

CAT: ref tests

085ab2b

artem1205 requested a review from a team as a code owner February 7, 2024 18:31

CAT: format

266b309

maxi297 reviewed Feb 8, 2024

View reviewed changes

artem1205 added 3 commits February 8, 2024 19:50

CAT: refactored

b155b51

Merge remote-tracking branch 'origin/master' into artem1205/CAT-33565

afbb79e

# Conflicts: # airbyte-integrations/bases/connector-acceptance-test/CHANGELOG.md

CAT: resolve conflicts

13331f9

maxi297 approved these changes Feb 8, 2024

View reviewed changes

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py Outdated Show resolved Hide resolved

artem1205 added 3 commits February 8, 2024 21:36

CAT: ref fixtures

8b55494

CAT: update tests

fe49f10

CAT: update tests

9fa583c

artem1205 merged commit a96b7f4 into master Feb 8, 2024
20 checks passed

artem1205 deleted the artem1205/CAT-33565 branch February 8, 2024 21:13

xiaohansong pushed a commit that referenced this pull request Feb 13, 2024

CAT: add validation for stream statuses (#34675)

d16bcce

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 21, 2024

CAT: add validation for stream statuses (airbytehq#34675)

cc6efa2

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

CAT: add validation for stream statuses (airbytehq#34675)

458fca6

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

CAT: add validation for stream statuses (airbytehq#34675)

3fc0555

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

CAT: add validation for stream statuses (airbytehq#34675)

5b0199a

xiaohansong pushed a commit that referenced this pull request Feb 27, 2024

CAT: add validation for stream statuses (#34675)

08d3514

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAT: add validation for stream statuses #34675

CAT: add validation for stream statuses #34675

artem1205 commented Jan 30, 2024

vercel bot commented Jan 30, 2024 •

edited

Loading

artem1205 commented Jan 31, 2024 •

edited

Loading

natikgadzhi left a comment

natikgadzhi Feb 2, 2024

artem1205 Feb 5, 2024 •

edited

Loading

alafanechere Feb 5, 2024

maxi297 left a comment

maxi297 Feb 5, 2024

artem1205 Feb 5, 2024

maxi297 Feb 7, 2024

maxi297 commented Feb 5, 2024 •

edited

Loading

maxi297 left a comment

maxi297 Feb 7, 2024

maxi297 Feb 7, 2024

artem1205 Feb 7, 2024

maxi297 Feb 8, 2024

artem1205 Feb 8, 2024

maxi297 Feb 8, 2024

artem1205 Feb 8, 2024

artem1205 commented Feb 7, 2024

maxi297 left a comment

maxi297 Feb 8, 2024

maxi297 left a comment

CAT: add validation for stream statuses #34675

CAT: add validation for stream statuses #34675

Conversation

artem1205 commented Jan 30, 2024

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Actions

Airbyter

vercel bot commented Jan 30, 2024 • edited Loading

artem1205 commented Jan 31, 2024 • edited Loading

natikgadzhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem1205 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 commented Feb 5, 2024 • edited Loading

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem1205 commented Feb 7, 2024

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

vercel bot commented Jan 30, 2024 •

edited

Loading

artem1205 commented Jan 31, 2024 •

edited

Loading

artem1205 Feb 5, 2024 •

edited

Loading

maxi297 commented Feb 5, 2024 •

edited

Loading