CDK: assert >0 state messages per read (fix tests) #35906

erohmensing · 2024-03-07T23:12:47Z

What

After Introduce FinalStateCursor to emit state messages at the end of full refresh syncs #35905, we should be emitting a state message with every successful sync. However there are a few tests that were too lenient and weren't actually successful syncs. This PR fixes those cases and adds validation that we emit at least one state message per successful sync.

How

Add an assertion that we get at least 1 state message for a successful sync
Fix some tests that previously "output 0 expected records" but actually errored silently - do not run them as read tests
Fix a test that failed silently due to lack of support for multi-format
Add a new test for syncs that output 0 records successfully

🚨 User Impact 🚨

None - test changes

Pre-merge Actions

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Community member? Grant edit access to maintainers (instructions)
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Connector version is set to 0.0.1
- Dockerfile has version 0.0.1
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- docs/integrations/<source or destination>/<name>.md including changelog with an entry for the initial version. See changelog example
- docs/integrations/README.md

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Unit & integration tests added

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:generateScaffolds then checking in your changes
Documentation which references the generator is updated as needed

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-03-07T23:12:49Z

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		Mar 8, 2024 10:09pm

erohmensing · 2024-03-07T23:13:01Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @erohmensing and the rest of your teammates on Graphite

airbyte-cdk/python/unit_tests/sources/file_based/test_scenarios.py

brianjlai

lgtm!

erohmensing · 2024-03-08T04:13:06Z

airbyte-cdk/python/unit_tests/sources/file_based/scenarios/csv_scenarios.py

@@ -467,48 +467,46 @@
    )
 ).build()

-multi_format_analytics_scenario: TestScenario[InMemoryFilesSource] = (
+csv_analytics_scenario: TestScenario[InMemoryFilesSource] = (


This test used to test analytics for multi-format. However, we don't actually support multi format. Although the analytics come out, the read fails silently, which means that we don't get the state message we expect.

Instead, we test the analytics separately (for the cases we do support - one format) and don't allow a read to silently fail.

erohmensing · 2024-03-08T04:15:32Z

airbyte-cdk/python/unit_tests/sources/file_based/scenarios/csv_scenarios.py

@@ -1450,7 +1487,6 @@
        }
    )
    .set_expected_discover_error(AirbyteTracedException, FileBasedSourceError.SCHEMA_INFERENCE_ERROR.value)
-    .set_expected_records([])


These 2 scenarios were scenarios that fail discover. They previously had "0 expected records", but fail silently if sent through the read tests. Instead, they are still discover scenarios, but no longer read scenarios - if discover fails, we don't expect the read to succeed.

These in particular don't assert any specific expected errors, which hints toward silent failure. If we got these cases to yell more aggressively about their errors, we could bring them back as read tests with expected errors.

erohmensing · 2024-03-08T04:16:20Z

airbyte-cdk/python/unit_tests/sources/file_based/scenarios/csv_scenarios.py

    .set_expected_discover_error(AirbyteTracedException, FileBasedSourceError.SCHEMA_INFERENCE_ERROR.value)
 ).build()
+
+csv_no_records_scenario: TestScenario[InMemoryFilesSource] = (


New scenario - a basic test for if we get 0 records in an expected, okay way. Essentially - if there's no data available, we expect 0 records, and I added this case to make sure that even if 0 records are synced, we get a state message.

erohmensing · 2024-03-08T04:17:05Z

airbyte-cdk/python/unit_tests/sources/file_based/test_file_based_scenarios.py

+discover_failure_scenarios = [
+    earlier_csv_scenario,
+    empty_schema_inference_scenario,
+]
+
+discover_success_scenarios = [
+    csv_no_records_scenario,


See other comment - not all discover scenarios can be used as read scenarios, as some that fail discover do not read successfully (in that they don't error successfully either)

airbyte-cdk/python/unit_tests/sources/file_based/test_scenarios.py

brianjlai

✔️

airbyte-cdk/python/unit_tests/sources/file_based/test_scenarios.py

…r read

…s.py

erohmensing · 2024-03-08T22:21:49Z

Merge activity

Mar 8, 5:21 PM EST: @erohmensing merged this pull request with Graphite.

erohmensing mentioned this pull request Mar 7, 2024

Introduce FinalStateCursor to emit state messages at the end of full refresh syncs #35905

Merged

octavia-squidington-iii added the CDK Connector Development Kit label Mar 7, 2024

erohmensing mentioned this pull request Mar 7, 2024

Emit record counts in state messages for concurrent streams #35907

Merged

erohmensing force-pushed the nonzero-num-state-messages-fixes branch from 645c51a to e490543 Compare March 7, 2024 23:26

brianjlai self-requested a review March 8, 2024 00:08

brianjlai reviewed Mar 8, 2024

View reviewed changes

airbyte-cdk/python/unit_tests/sources/file_based/test_scenarios.py Outdated Show resolved Hide resolved

brianjlai approved these changes Mar 8, 2024

View reviewed changes

erohmensing commented Mar 8, 2024

View reviewed changes

erohmensing requested a review from clnoll March 8, 2024 04:18

erohmensing marked this pull request as ready for review March 8, 2024 04:20

erohmensing requested a review from a team as a code owner March 8, 2024 04:20

brianjlai approved these changes Mar 8, 2024

View reviewed changes

airbyte-cdk/python/unit_tests/sources/file_based/test_scenarios.py Outdated Show resolved Hide resolved

octavia-squidington-iv requested review from a team March 8, 2024 17:56

Base automatically changed from final-state-cursor to master March 8, 2024 21:58

erohmensing added 7 commits March 8, 2024 16:04

fix tests in order to be able to assert at least one state message pe…

eb9c03a

…r read

only pass discover success scenarios to read

44f2070

fix formatting errors

c09e1ad

remove expected records from tests which aren't used in read

0180060

add unit test for expected 0 records

a4892ce

Update airbyte-cdk/python/unit_tests/sources/file_based/test_scenario…

3c76dc5

…s.py

format

7ee8a18

erohmensing force-pushed the nonzero-num-state-messages-fixes branch from ae515d5 to 7ee8a18 Compare March 8, 2024 22:04

update assertion failure description

77e7567

erohmensing merged commit a4dca3b into master Mar 8, 2024
27 checks passed

erohmensing deleted the nonzero-num-state-messages-fixes branch March 8, 2024 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDK: assert >0 state messages per read (fix tests) #35906

CDK: assert >0 state messages per read (fix tests) #35906

erohmensing commented Mar 7, 2024 •

edited

vercel bot commented Mar 7, 2024 •

edited

erohmensing commented Mar 7, 2024 •

edited

brianjlai left a comment

erohmensing Mar 8, 2024

erohmensing Mar 8, 2024

erohmensing Mar 8, 2024

erohmensing Mar 8, 2024

brianjlai left a comment

erohmensing commented Mar 8, 2024

CDK: assert >0 state messages per read (fix tests) #35906

CDK: assert >0 state messages per read (fix tests) #35906

Conversation

erohmensing commented Mar 7, 2024 • edited

What

How

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Airbyter

vercel bot commented Mar 7, 2024 • edited

erohmensing commented Mar 7, 2024 • edited

brianjlai left a comment

Choose a reason for hiding this comment

erohmensing Mar 8, 2024

Choose a reason for hiding this comment

erohmensing Mar 8, 2024

Choose a reason for hiding this comment

erohmensing Mar 8, 2024

Choose a reason for hiding this comment

erohmensing Mar 8, 2024

Choose a reason for hiding this comment

brianjlai left a comment

Choose a reason for hiding this comment

erohmensing commented Mar 8, 2024

Merge activity

erohmensing commented Mar 7, 2024 •

edited

vercel bot commented Mar 7, 2024 •

edited

erohmensing commented Mar 7, 2024 •

edited