Repository navigation
feat(standard-tests): assert discovered catalog properties in test_discover - #1217
Conversation
…scover Port the discovery assertions from the retired Connector Acceptance Tests into SourceTestSuiteBase.test_discover. Checks with no failing connector in the fleet are enforced; checks that manifest-only connectors still violate emit DiscoveredCatalogWarning until those connectors are fixed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
👋 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/discover-assertions#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/discover-assertionsPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
…tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Daryna Ishchenko (darynaishchenko)
left a comment
There was a problem hiding this comment.
Verdict: fix before merge — 0 P0 · 1 P1 · 5 P2 · 2 P3 (all inline).
Breaking change: BREAKING under criterion A.7 only — a new mandatory standard-test gate that every source connector running airbyte-cdk connector test picks up automatically on the next release (monorepo CI installs the CLI unpinned). No runtime, API, manifest or state break; feat: → minor is correct; the staged-rollout call-out in the body meets the requirement. A cdk-migrations.md entry is still worth adding (inline P2).
Done well
- Staged rollout backed by real data: fleet dry run, the exact list of 11 connectors that will warn, a one-flag promotion path.
- Keyword-aware schema walker removes CAT's false positives on properties named
$ref/not/allOfand on$reftext inexamples— and is tested for exactly those cases. - Every violation reported in one run; a crashing advisory check is reported, not raised, so it cannot fail a connector by throwing on an unexpected shape. The CAT
set("object")bug is fixed and pinned.
Fleet sweep (independent). Static sweep of every language:manifest-only connector on origin/master (239eafd614e) through the PR-head ManifestReferenceResolver + ManifestComponentTransformer, all offline-resolvable schemas run through CATALOG_CHECKS: 516 connectors, 5,256 streams, 0 enforced failures; plus the 4 enterprise manifest-only connectors, 38 streams, 0 enforced failures. Advisory counts for JSON-schema validity (6) and supported data types (6) match the PR body. Not evaluable offline: 9 connectors with HTTP-built catalogs (dynamic_streams in airtable, bing-ads, google-analytics-data-api, google-search-console, google-sheets, hubspot, linkedin-ads, pinterest; CustomSchemaLoader in us-census). Reading the generators, DynamicSchemaLoader hard-codes additionalProperties: True and emits only oneOf, and the custom loaders set additionalProperties: True, so an enforced violation there is not plausible — but reasoned, not run.
Rollout / risk
- CI: all required checks green on 3.10–3.14;
Check: destination-motherduckfails at the same step on #1224/#1221/#1219 (baseline, and destinations do not runtest_discover);Check: source-shopifystill pending;source-google-driveskipped. None of the three passing connector checks exercised the advisory warning path. - Before release:
/prereleaseand a pinnedairbyte-cdk connector testwith CI secrets on source-google-sheets, source-hubspot, source-google-analytics-data-api (HTTP-built catalogs) and source-tvmaze-schedule (warning path). - Overlap with #1219: its
inspect_outputsecret scan is wired intotest_checkonly; the newtest_discovercall here does not pass it, so after both merge discover output is not secret-scanned unless one PR adds it. Note for #1219, not a defect here.
…es in catalog checks - Validate each stream schema against the meta-schema of the draft it declares in `$schema`, falling back to Draft 7. - Read the types of `anyOf`/`oneOf` branches for the `format` and `airbyte_type` checks, and skip those checks on untyped nodes. - Pin which catalog checks are enforced and which only warn, the ALLOW_ANY discover path, the exactly-one-CATALOG assertion, the `status: failed` skip, and every keyword the schema walker follows. - Drop the inert `noqa: BLE001` and use the 2026 copyright year. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add a `cdk-migrations.md` entry for the new `test_discover` checks and document how to turn `DiscoveredCatalogWarning` into a failure in the warning class and the `standard_tests` package docstrings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Daryna Ishchenko (darynaishchenko)
left a comment
There was a problem hiding this comment.
Approving. All eight round-1 threads are resolved, and I checked each fix the same way the problem was found. The mutation harness now catches every wiring mutation. The draft-04 and anyOf/oneOf schema examples that produced false alarms now pass. The docs and the cdk-migrations.md entry are in place and match the code. Re-running the fleet sweep with the round-1 code and this head gives byte-identical results, so the two fix commits change nothing for connectors.
I have only minor nits left, none blocking.
Combines #1217, #1219 and #1220 with this branch: - test_discover keeps the section gate of this branch and runs the catalog checks from main; an empty catalog now fails the enforced 'catalog has streams' check, so its test expects that assertion. - test_check runs the secret leak check before asserting the check outcome. - cdk-migrations.md lists every section under one 7.36.0 heading. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
SourceTestSuiteBase.test_discoverrandiscoverand asserted only that it raised no errors. This PR adds back theTestDiscoverychecks from the retired Connector Acceptance Tests (CAT), so the standard tests check the catalog thatdiscoverreturns.The checks are in a new module,
airbyte_cdk/test/standard_tests/_catalog_checks.py. Each check takes theAirbyteCatalogand returns a list of problems. Each problem names the stream and the field or JSON pointer. One run reports every problem, so fixing one problem does not reveal the next on a later run.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17134 (part of epic airbytehq/airbyte-internal-issues#17129).
Staged rollout: enforced and advisory checks
Monorepo CI installs the CDK CLI unpinned. Any new assertion in
test_discovertherefore runs for every connector on the next CDK release. To roll out safely, the checks are split into two groups:DiscoveredCatalogWarningand do not fail the test. Connectors in the fleet still violate them.To make the advisory checks fail locally or in a connector's own CI, use the standard pytest filter:
test_discovertest_discovertest_streams_has_sync_modestest_defined_refs_exist_in_schemaallOf,not)test_defined_keyword_exist_in_schemaadditionalPropertiesis not falsetest_additional_properties_is_truetest_streams_have_valid_json_schemastest_defined_cursors_exist_in_schematest_primary_keys_exist_in_schematest_primary_keys_data_typetest_catalog_has_supported_data_typesTo promote a check from advisory to enforced, flip its
enforced=FalsetoTrueinCATALOG_CHECKS. Do that per check, after the connectors that violate it are fixed.test_discoveralso asserts thatdiscoveremits exactly oneCATALOGmessage. If a scenario allowsdiscoverto fail (ALLOW_ANYwith errors), the test returns early because there is no catalog to check.Differences from CAT
$ref,notorallOf, and$reftext insideexamplesordefaultdata. The new walker descends only into keywords whose values are schemas.jsonschema's singlebest_matcherror, which picked a different error on each run for the same schema. The new checks report every error, sorted by location.set("object"), which gives{'o', 'b', 'j', ...}. As a result, a key typed as a plain"object"string was never caught. The new check readstypeas a string or a list.pytest.mark.skipped for its whole life. The new check requires:airbyte_typeto be one of the protocol's types and to sit on a compatible JSON type;date,date-time,time) to sit on astringtype.empty_streamsdoes not skip any discovery check. An invalid schema is invalid whether or not the stream has test data.Declined
formatwhitelist (date,date-timeonly). Formats such asemailanduriare harmless to destinations. The whitelist also rejectedtime, which the type system supports. Only temporal formats are checked, and only for their JSON type.typenames, for exampleintin source-bunny-inc.validate_primary_keys_data_typeopt-out. No connector in the monorepo sets it, and P1.5 is removing scenario fields rather than adding them. A connector that needs an exemption should fix its schema instead.test_backward_compatibility. It compares against the previous published image's catalog. That needs a second connector image, so it is out of scope for an in-processdiscoverassertion.Fleet dry run
Method. Every manifest-only connector on airbyte
master(3ab60a0e19f) was built in-process with this branch. Each run used a dummy config generated from the spec: defaults, enums, examples and per-connector date overrides, plusintegration_tests/sample_config.jsonwhere present. All HTTP was blocked. The checks ran against every stream whose schema could be produced without a network call.Coverage.
dynamic_streamsskipped.Results.
test_discoverwith a config in CI today. The rest have no scenario with aconfig_path.Of the 50 failing connectors, these 11 run
test_discoverin CI and will show the warning on the next release:array)airbyte_type: big_number)type)type)nullproperty schemas)airbyte_type: decimal, 82 fields)type)All 50 connectors by check
Python connectors (P0.7)
Of the 106 Python connectors, only source-hardcoded-records runs the standard tests today, and it pins its CDK version, so it picks up these checks on a CDK bump rather than automatically. The others do not run the standard tests yet. A static scan of their schema files suggests what to expect once P0.7 brings them in:
additionalProperties: falseon 6 schemas. That check is enforced, so tplcentral needs a schema fix before, or together with, P0.7. Otherwise P0.7 needs a second staged step.format: date-timeon aninteger. This is an advisory check.discover.Test fixture change
unit_tests/resources/source_pokeapi_w_components_py/manifest.yamldeclared primary keyidwith an emptypropertiesschema. That is the defect the new check reports. The schema now declaresidandname, so the fixture's standard tests pass with the advisory warnings turned into errors.Testing
unit_tests/test/test_catalog_checks.pycovers:test_discoverwith mockeddiscoveroutput: enforced failure, advisory warning, no catalog, andALLOW_ANY;DeclarativeSourceTestSuiterun on an inline manifest.airbyte-cdk connector teston source-tvmaze-schedule with this branch passes and shows the warning. It fails with-W error::...DiscoveredCatalogWarning.mypy airbyte_cdkandruffare clean.Follow-ups
test_discoverbecause they have no scenario with a config. A credential-freediscoverpath would extend these checks to them.🤖 Generated with Claude Code