Repository navigation
feat(standard-tests): check primary keys, state messages and error traces in read tests - #1230
Draft
Anatolii Yatsuk (tolik0) wants to merge 2 commits into
Draft
Anatolii Yatsuk (tolik0) wants to merge 2 commits into
Anatolii Yatsuk (tolik0) wants to merge 2 commits into
Conversation
…aces in read tests The Standard Tests replaced the Connector Acceptance Tests as the only read coverage for most connectors, but three checks of CAT's basic read test were not carried over. This restores them: - Primary keys: for every `basic_read` config, every record of a stream with a `source_defined_primary_key` must have a usable key. No key field may hold an object or an array, and not every key field may be missing or null, as in CAT's `validate_primary_keys_data_type`. Failures name the stream, record number and key path, never the value. Opt out per config with `validate_primary_keys_data_type: false`. - State: every configured stream must be named by a STREAM state message or by a GLOBAL state's `stream_states`; no state may use the LEGACY type; every state must carry `sourceStats.recordCount`. CAT only compared the number of state messages with the number of streams, which one stream's many states could satisfy for another stream that emitted none. Opt out per config with `validate_state_messages: false`. - Error traces: the new `test_fail_read_with_invalid_config` reads with a config the connector's own spec rejects (it holds none of the required top-level properties and no real value, so no request can be sent with real credentials) and requires a well-formed ERROR trace, as CAT's `test_airbyte_trace_message_on_failure` did. `test_fail_read_with_bad_catalog` now checks its error traces the same way. Opt out with `expect_trace_message_on_failure: false`. The new `assert_read_records` arguments default to off, so the Docker read test is unchanged; it can pass them once it collects record messages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ss_reason A `basic_read` entry has no `bypass_reason` field: pydantic ignores the key silently, and only `empty_streams` items carry one. Tell readers to give the reason in a YAML comment, as the 7.36.0 `validate_schema` step and the connector opt-outs do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 task done
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@tolik0/cdk/read-assertions-slice-2#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch tolik0/cdk/read-assertions-slice-2PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Anatolii Yatsuk (tolik0)
marked this pull request as draft
October 14, 2026 14:14
PyTest Results (Fast)5 813 tests +65 5 782 ✅ +64 11m 32s ⏱️ -38s Results for commit cc674f5. ± Comparison against base commit 2082c2d. This pull request skips 1 test. |
PyTest Results (Full)5 816 tests +65 5 785 ✅ +64 16m 31s ⏱️ -29s Results for commit cc674f5. ± Comparison against base commit 2082c2d. This pull request skips 1 test. |
4 of 5 tasks
This branch has not been deployed
This file contains hidden or 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
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.
Part of airbytehq/airbyte-internal-issues#17135 (P1.2 "Restore read assertions", epic airbytehq/airbyte-internal-issues#17129). Follows #1218.
What
#1218 restored per-stream record presence and schema validation in
test_basic_read. Three more checks of CAT's basic read test were still missing from the Standard Tests. This PR adds them, with CAT's config flags as the opt-outs:_validate_primary_keys_data_type). For every config underbasic_read, every record of a stream with asource_defined_primary_keymust have a usable key: no key field holds an object or an array, and not every key field is missing or null. Opt out withvalidate_primary_keys_data_type: false._validate_state_messages). Every stream the read covers must be named by aSTREAMstate message (or aGLOBALstate'sstream_states); no state uses the deprecatedLEGACYtype; every state carriessourceStats.recordCount. Opt out withvalidate_state_messages: false.test_airbyte_trace_message_on_failure). New per-connector testtest_fail_read_with_invalid_config, andtest_fail_read_with_bad_catalognow checks its error traces the same way. Opt out withexpect_trace_message_on_failure: false.Failure messages name the stream, the record number and the key path, never a value:
How
_read_assertions.py:_StreamPrimaryKeyChecker(runs in the same single pass over the records as the schema check),_find_state_message_failures,assert_error_trace_messages,make_invalid_config.assert_read_recordsgainsvalidate_primary_keysandstate_messages, both default off, so the Docker read test, which P1.4 (airbytehq/airbyte-internal-issues#17137) is changing in parallel, is not touched. It takes record messages plus the configured catalog, so P1.4 can pass the new arguments as is.ConnectorTestScenario:validate_primary_keys_data_type,validate_state_messages,expect_trace_message_on_failure, with CAT's names and defaults (true)._dedup_scenarioskeeps an opt-out declared by any entry for the config, as it already did forvalidate_schema.test_basic_readpasses the new arguments forbasic_readconfigs. A config read only because offull_refreshkeeps the "some records" floor, as in feat(standard-tests): assert per-stream records and record schemas in the basic read test #1218.cdk-migrations.md: a section under "Upgrading to the next minor release (unreleased)".Decisions
Primary keys follow CAT exactly. A composite key may have null parts as long as one part is present, and booleans count as scalars (the message calls them "boolean", not "integer"). The stricter "every key part present" rule was measured only (see below), not enforced. As in CAT,
validate_primary_keys_data_type: falseis allowed attest_strictness_level: high; onlyvalidate_schema: falseis refused there.State: the per-stream version, not CAT's count. CAT asserted
len(state_messages) >= len(streams). That is not vacuous, but one stream that checkpoints many times hides another stream that emitted no state at all. This PR checks coverage per stream (matched on namespace and name). The LEGACY check is kept; CAT's version crashed onGLOBALstates, which this one handles. ThesourceStatscheck is kept, but it is vacuous for CDK connectors, because the CDK entrypoint setssourceStats.recordCounton every state it emits; it guards non-CDK sources and the Docker path. The CDK emits a final{"__ab_no_cursor_state_message": true}state for every full-refresh stream it completes, so a CDK connector fails only when a stream ends abnormally or custom code drops the state.The failure input for the trace test is a config the connector's spec rejects. CAT's trace test used the valid config plus a broken catalog, and FAST's
test_fail_read_with_bad_catalogalready is that test. It now uses the same well-formedness check instead of only requiring some trace message, so there is no duplicate. The new test needs a different failure, one that reaches the connector:__airbyte_standard_tests_invalid_config) and none of the spec's required top-level properties. The CDK entrypoint rejects it against the spec with aconfig_errortrace, before any request is sent.basic_readconfigs are all secret-backed are covered too.AbstractSourcereports it asINCOMPLETEwithout an error.basic_readentry setsexpect_trace_message_on_failure: false, as CAT did.entrypoint_wrapperturns any uncaught exception into a well-formed trace message, so this check mostly guards the CDK and the harness. The same helper is meant for the Docker path (P1.4), where a crashing image can emit nothing.source-s3's hand-ported CAT harness (integration_tests/test_acceptance.py), named as prior art in the issue, has none of these three checks, so they are ported from CAT's semantics directly.Fleet impact
I ran this branch's checks in-process (as
airbyte-cdk connector testdoes) against airbytehq/airbytemasterat1e22e3c689c. The population was every manifest-only source with abasic_readconfig: 185 connectors. 16 were skipped, because their test secret holds a single-use rotating refresh token, and a local run would invalidate the copy CI uses. Eachbasic_readscenario was read once (20-minute timeout, and 45 minutes for a second try of the 6 that timed out), and the verdict onmain(presence and schema, from #1218) and the new checks were taken from the same read.test_fail_read_with_invalid_configran once per connector, andtest_fail_read_with_bad_catalogon the first read scenario.Already red before this change: 41 have expired, revoked or under-scoped test credentials, or hit plan limits; 8 have no test secret in GSM; 3 fail to load with the current CDK or have an
acceptance-test-config.ymlthe harness cannot parse; 25 fail on other API or connector errors, or on #1218's checks. None of the reads that reached the checks failed the new ones, so they hide nothing that is known.colorscalendarandeventhold objects.subscribers_histogram_by_stateid, the declared key.Both are real key problems, not false positives, and CAT would have failed them too. Changing a primary key is a breaking change, so airbytehq/airbyte#88937 opts the two configs out with
validate_primary_keys_data_type: falseinstead, and the key fixes are left for separate PRs.Per check:
validate_state_messages: false). No connector fails.test_fail_read_with_invalid_config: 161 passed and 6 skipped because the spec requires no top-level property. 2 failed, both on connectors whose every test already fails (one cannot load its custom components with the current CDK, one has anacceptance-test-config.ymlthe harness cannot parse).test_fail_read_with_bad_catalog: 159 passed. The 9 failures are connectors that are red before: 8 have no test secret, and 1 cannot load its custom components.The CDK's Python test connectors, run in-process with this branch: source-hardcoded-records passes (the new trace test skips, as its spec requires nothing). source-shopify's
test_fail_read_with_invalid_configpasses; its twotest_basic_readscenarios fail before any new check, because the test store'sEurope/Kyivtime zone is missing from thependulum2 time zone data, which this PR does not touch. source-google-drive cannot be created by the harness (its constructor needscatalog,configandstate), and the new trace test skips on its existingexpect_trace_message_on_failure: false.Rollout
The connector monorepo installs the
airbyte-cdkCLI unpinned, so connector CI picks the new checks up with the release, without a per-connector CDK bump. That reaches the manifest-only connectors, which run the in-process suite (airbyte-cdk connector test). Python connectors run only the Docker suite (airbyte-cdk image test) or pytest against their own pinned CDK, so they get these checks through P1.4's Docker read test (airbytehq/airbyte-internal-issues#17137) or a CDK bump. source-hardcoded-records is the only Python connector that subclasses the suite. This ships as a minor (feat, no!), as standard-test changes do.Merge order:
The monorepo picks the release up through the unpinned CLI, so the connector opt-outs must be on
masterbefore this PR is released.Landing order for airbytehq/airbyte-internal-issues#17135 (updated)
fail_on_extra_columns), stream status completeness (validate_stream_statuses), and data points (validate_data_points). CAT never appliedfail_on_extra_columnsand defaultedvalidate_data_pointsto false, so those two ship behind flags. CAT required stream statuses by default for certified connectors only, so that check needs its own measurement before it can be enforced. Not in this PR.The Docker read test (
test_docker_image_build_and_read) is not changed here; P1.4 (airbytehq/airbyte-internal-issues#17137) owns it.Test plan
poetry run pytest unit_tests/test/ -p no:cacheprovider: 848 passed, 9 skipped. New tests inunit_tests/test/test_read_keys_state_and_traces.pycover each check directly and throughSourceTestSuiteBase(fake sources that emit a null key, no state, a malformed trace, or a read that succeeds on the invalid config), the opt-outs, high strictness, and opt-outs surviving_dedup_scenariosin both orders.ruff format,ruff check,mypy airbyte_cdk/test: clean.source_pokeapi_w_components_pystandard tests (the CDK fixture): 12 passed, 10 skipped;test_fail_read_with_invalid_configpasses.🤖 Generated with Claude Code