feat(standard-tests): remove the ignored scenario fields and reject acceptance-test-config keys that are not read

airbyte_cdk/test/models/scenario.py

@@ -17,12 +17,12 @@
 from typing import TYPE_CHECKING, Any, Literal, cast
 
 import yaml
-from pydantic import BaseModel, ConfigDict, field_validator
+from pydantic import BaseModel, ConfigDict, field_validator, model_validator
 
 from airbyte_cdk.test.models.outcome import ExpectedOutcome
 
 if TYPE_CHECKING:
-    from collections.abc import Generator, Mapping
+    from collections.abc import Generator, Iterable, Mapping
 
 
 ScenarioCommand = Literal["spec", "check", "discover", "read"]
@@ -63,28 +63,147 @@
 )
 """Every command; what a scenario built by hand (no `sections`) runs."""
 
+_NO_RECORD_COMPARISON = (
+    "it configured the Connector Acceptance Tests' comparison of the records read against "
+    "recorded ones, which the standard tests do not run"
+)
+
+REMOVED_ENTRY_KEYS: Mapping[str, str] = MappingProxyType(
+    {
+        "expect_records": _NO_RECORD_COMPARISON,
+        "exact_order": _NO_RECORD_COMPARISON,
+        "ignored_fields": (
+            "it listed the fields the Connector Acceptance Tests skipped when they compared "
+            "records, against `expect_records` or between two consecutive full-refresh reads; "
+            "the standard tests run neither comparison"
+        ),
+        "fail_on_extra_columns": (
+            "the Connector Acceptance Tests accepted it but never enforced it, and the standard "
+            "tests have no such check"
+        ),
+        "timeout_seconds": "the standard tests have no per-config timeout",
+        "file_types": "the standard tests have no check that every supported file type is read",
+    }
+)
+"""Keys an `acceptance-test-config.yml` entry can no longer set, with why they are not read.
+
+They configured checks of the retired Connector Acceptance Tests (CAT) that the standard tests do
+not run. `ConnectorTestScenario` declared some of them and ignored them; an entry that still sets
+one is now rejected, so a config cannot look like it asks for a check that never runs.
+"""
+
+LEGACY_ENTRY_KEYS: Mapping[str, frozenset[str]] = MappingProxyType(
+    {
+        "spec": frozenset({"spec_path", "backward_compatibility_tests_config", "deployment_mode"}),
+        "connection": frozenset({"deployment_mode"}),
+        "discovery": frozenset({"backward_compatibility_tests_config"}),
+        "basic_read": frozenset(
+            {
+                "client_container_config",
+                "expect_trace_message_on_failure",
+                "validate_state_messages",
+            }
+        ),
+        "full_refresh": frozenset({"client_container_config"}),
+    }
+)
+"""CAT keys that an entry of each section may still set, although the scenario does not read them.
+
+`backward_compatibility_tests_config` of a `spec` entry is read from the file itself by the spec
+backward-compatibility test (`disable_for_version`). The others configure CAT checks that have no
+standard-test counterpart yet, and connectors still set them. They are accepted, and dropped, only
+in the section CAT read them from. Every other key that is not a field of `ConnectorTestScenario`
+is rejected (see `entry_key_problems`).
+"""
+
+
+POPULATED_FIELDS: Mapping[str, str] = MappingProxyType(
+    {
+        "sections": "the standard tests set it from the section that lists the entry",
+        "test_strictness_level": (
+            "it is read only at the top level of `acceptance-test-config.yml`"
+        ),
+    }
+)
+"""Fields of `ConnectorTestScenario` that `get_scenarios()` fills in, so an entry cannot set them.
+
+An entry that set one would be overwritten without a word, so it is rejected, with the reason.
+"""
+
+
+def entry_key_problems(
+    entry: Mapping[str, Any],
+    sections: Iterable[str],
+    *,
+    from_file: bool = True,
+) -> list[str]:
+    """Return one message per key of an `acceptance-test-config.yml` entry that is not accepted.
+
+    An entry may set the fields of `ConnectorTestScenario`, except the `POPULATED_FIELDS`, and the
+    `LEGACY_ENTRY_KEYS` of the section that lists it. A key of `REMOVED_ENTRY_KEYS` is reported as
+    no longer read, and a key of `POPULATED_FIELDS` as not an entry key, with the reason; any
+    other key as unknown. Each message names the key, the section and the entry's `config_path`,
+    never a value.
+
+    `from_file=False` checks the data a scenario is built from (by `get_scenarios()` or by hand),
+    where the `POPULATED_FIELDS` are accepted.
+    """
+    sections = tuple(sections)
+    accepted = set(ConnectorTestScenario.model_fields)
+    if from_file:
+        accepted -= set(POPULATED_FIELDS)
+    for section in sections:
+        accepted |= LEGACY_ENTRY_KEYS.get(section, frozenset())
+    where = (f"the `{'`/`'.join(sections)}` entry" if sections else "the scenario") + (
+        f" for config `{entry['config_path']}`"
+        if entry.get("config_path")
+        else " without a `config_path`"
+    )
+    listed = sorted(accepted - set(POPULATED_FIELDS))
+
+    problems: list[str] = []
+    for key in entry:
+        if key in accepted:
+            continue
+        if key in REMOVED_ENTRY_KEYS:
+            problems.append(
+                f"`{key}` in {where} is no longer read: {REMOVED_ENTRY_KEYS[key]}. Remove the key."
+            )
+        elif key in POPULATED_FIELDS:
+            problems.append(
+                f"`{key}` in {where} is not an entry key: {POPULATED_FIELDS[key]}. "
+                "Remove it from the entry."
+            )
+        else:
+            problems.append(
+                f"`{key}` in {where} is not a key the standard tests read. Remove it, or fix "
+                f"its spelling (accepted: {', '.join(f'`{k}`' for k in listed)})."
+            )
+    return problems
+
 
 class ConnectorTestScenario(BaseModel):
     """Acceptance test scenario, as a Pydantic model.
 
     This class represents an acceptance test scenario, which is a single test case
     that can be run against a connector. It is used to deserialize and validate the
     acceptance test configuration file.
+
+    Every field is read by the standard tests (a unit test enforces it). An entry that sets a key
+    that is not a field is rejected, except for the `LEGACY_ENTRY_KEYS` of its section; a key the
+    standard tests used to accept and ignore is reported as no longer read (`REMOVED_ENTRY_KEYS`).
+    `get_scenarios()` also rejects an entry that sets one of the `POPULATED_FIELDS`, which it fills
+    in itself.
     """
 
     # Allows the class to be hashable, which PyTest will require
     # when we use to parameterize tests.
-    model_config = ConfigDict(frozen=True)
-
-    class AcceptanceTestExpectRecords(BaseModel):
-        path: Path
-        exact_order: bool = False
-
-    class AcceptanceTestFileTypes(BaseModel):
-        skip_test: bool
-        bypass_reason: str
+    # `hide_input_in_errors`: a validation error must not print config values.
+    model_config = ConfigDict(frozen=True, extra="forbid", hide_input_in_errors=True)
 
     class AcceptanceTestEmptyStream(BaseModel):
+        model_config = ConfigDict(extra="forbid")
+
         name: str
         bypass_reason: str | None = None
 
@@ -99,10 +218,10 @@ def __hash__(self) -> int:
 
     configured_catalog_path: Path | None = None
     empty_streams: list[AcceptanceTestEmptyStream] | None = None
-    timeout_seconds: int | None = None
-    expect_records: AcceptanceTestExpectRecords | None = None
-    file_types: AcceptanceTestFileTypes | None = None
     status: Literal["succeed", "failed", "exception"] | None = None
+    # Why the entry tests nothing. Read by the zero-scenario rule (a connector without test
+    # scenarios must declare why, airbyte-internal-issues#17131), which is not implemented yet.
+    bypass_reason: str | None = None
     # CAT's `basic_read` opt-out from validating records against the stream schemas.
     validate_schema: bool = True
     # The connector's top-level `test_strictness_level` in `acceptance-test-config.yml`. Populated
@@ -112,6 +231,21 @@ def __hash__(self) -> int:
     # `get_scenarios()`; empty for scenarios built by hand, which run every command.
     sections: tuple[str, ...] = ()
 
+    @model_validator(mode="before")
+    @classmethod
+    def _reject_keys_not_read(cls, data: Any) -> Any:
+        """Reject keys that are not read, and drop the `LEGACY_ENTRY_KEYS` of the entry's section."""
+        if not isinstance(data, dict):
+            return data
+        sections = data.get("sections") or ()
+        if isinstance(sections, str):
+            sections = (sections,)
+        problems = entry_key_problems(data, sections, from_file=False)
+        if problems:
+            raise ValueError("\n".join(problems))
+        legacy = {key for section in sections for key in LEGACY_ENTRY_KEYS.get(section, ())}
+        return {key: value for key, value in data.items() if key not in legacy}
+
     @field_validator("sections")
     @classmethod
     def _sections_must_be_known(cls, sections: tuple[str, ...]) -> tuple[str, ...]:

airbyte_cdk/test/standard_tests/docker_base.py

@@ -17,6 +17,7 @@
 import requests
 import yaml
 from boltons.typeutils import classproperty
+from pydantic import ValidationError
 
 from airbyte_cdk.models import (
     AirbyteStream,
@@ -29,7 +30,11 @@
 from airbyte_cdk.models.connector_metadata import MetadataFile
 from airbyte_cdk.test.entrypoint_wrapper import EntrypointOutput
 from airbyte_cdk.test.models import ConnectorTestScenario
-from airbyte_cdk.test.models.scenario import SCENARIO_SECTIONS, ScenarioCommand
+from airbyte_cdk.test.models.scenario import (
+    SCENARIO_SECTIONS,
+    ScenarioCommand,
+    entry_key_problems,
+)
 from airbyte_cdk.test.standard_tests._assertions import (
     assert_check_outcome,
     assert_config_matches_spec,
@@ -391,6 +396,16 @@ def _dedup_scenarios(scenarios: list[ConnectorTestScenario]) -> list[ConnectorTe
                 )
         return deduped_scenarios
 
+    @classmethod
+    def _raise_if_invalid(cls, problems: list[str]) -> None:
+        """Fail collection with every problem found in `acceptance-test-config.yml`, if any."""
+        if problems:
+            config_file = cls.get_connector_root_dir().absolute() / ACCEPTANCE_TEST_CONFIG
+            raise ValueError(
+                f"`{config_file}` is not valid for the standard tests:\n"
+                + "\n".join(f"- {problem}" for problem in problems)
+            )
+
     @classmethod
     def get_scenarios(
         cls,
@@ -404,6 +419,11 @@ def get_scenarios(
         streams of the config's `configured_catalog_path`, or of every discovered stream when it
         declares none (`SECTION_COMMANDS`). The `incremental` section is not collected.
 
+        An entry of a collected section that sets a key the standard tests do not read, or one
+        they fill in themselves (`sections`, `test_strictness_level`), fails collection, with one
+        line per key naming the key, the section and the config, including entries that are
+        skipped (no `config_path`, or an `iam_role` config). See `entry_key_problems`.
+
         This has to be a separate function because pytest does not allow
         parametrization of fixtures with arguments from the test class itself.
         """
@@ -419,7 +439,7 @@ def get_scenarios(
             return []
 
         test_strictness_level = all_tests_config.get("test_strictness_level")
-        test_scenarios: list[ConnectorTestScenario] = []
+        entries: list[tuple[str, dict[str, Any]]] = []
         for category in SCENARIO_SECTIONS:
             if (
                 category not in all_tests_config["acceptance_tests"]
@@ -428,23 +448,49 @@ def get_scenarios(
                 continue
 
             for test in all_tests_config["acceptance_tests"][category]["tests"]:
-                if "config_path" not in test:
-                    # Skip tests without a config_path
-                    continue
+                if isinstance(test, dict):
+                    entries.append((category, test))
+
+        # Every entry is checked, including those skipped below, so that a key that is not read
+        # fails collection wherever it is set.
+        cls._raise_if_invalid(
+            [
+                problem
+                for category, test in entries
+                for problem in entry_key_problems(test, (category,))
+            ]
+        )
+
+        problems: list[str] = []
+        test_scenarios: list[ConnectorTestScenario] = []
+        for category, test in entries:
+            if "config_path" not in test:
+                # Skip tests without a config_path
+                continue
 
-                if "iam_role" in test["config_path"]:
-                    # We skip iam_role tests for now, as they are not supported in the test suite.
-                    continue
+            if "iam_role" in test["config_path"]:
+                # We skip iam_role tests for now, as they are not supported in the test suite.
+                continue
 
+            try:
                 scenario = ConnectorTestScenario.model_validate(
                     {
                         **test,
                         "sections": (category,),
                         "test_strictness_level": test_strictness_level,
                     }
                 )
+            except ValidationError as error:
+                # Reported without the input, which can hold config values.
+                problems.extend(
+                    f"`{category}` entry for config `{test['config_path']}`: "
+                    f"`{'.'.join(str(part) for part in detail['loc'])}`: {detail['msg']}"
+                    for detail in error.errors()
+                )
+                continue
 
-                test_scenarios.append(scenario)
+            test_scenarios.append(scenario)
+        cls._raise_if_invalid(problems)
 
         deduped_test_scenarios = cls._dedup_scenarios(test_scenarios)
 

cdk-migrations.md

@@ -1,5 +1,41 @@
 # CDK Migration Guide
 
+## Upgrading to the next minor release (unreleased)
+
+### `acceptance-test-config.yml` keys the Standard Tests do not read fail collection
+
+`ConnectorTestScenario` declared `expect_records` (with `exact_order`), `timeout_seconds` and `file_types`, and no test read them: they parsed, and the check they configured in the retired Connector Acceptance Tests (CAT) never ran. They are removed. Any other key that the model did not declare was dropped without a word, so a misspelled key (`empty_stream` for `empty_streams`) was ignored as well.
+
+An entry of a collected section (`spec`, `connection`, `discovery`, `basic_read`, `full_refresh`) that sets a key the Standard Tests do not read now fails test collection, with one line per key naming the key, the section and the config:
+
+```
+`.../source-example/acceptance-test-config.yml` is not valid for the standard tests:
+- `expect_records` in the `basic_read` entry for config `secrets/config.json` is no longer read: it configured the Connector Acceptance Tests' comparison of the records read against recorded ones, which the standard tests do not run. Remove the key.
+- `timeout_seconds` in the `full_refresh` entry for config `secrets/config.json` is no longer read: the standard tests have no per-config timeout. Remove the key.
+```
+
+| Key | Status |
+|---|---|
+| `expect_records`, `exact_order` | Removed: no comparison against recorded records. |
+| `ignored_fields` | Removed: it listed fields CAT skipped when comparing records, against `expect_records` or between two consecutive full-refresh reads, and the Standard Tests run neither comparison. It was never declared on the model. |
+| `fail_on_extra_columns` | Removed: CAT accepted it but never enforced it, and the Standard Tests have no such check. It was never declared on the model. |
+| `timeout_seconds` | Removed: no per-config timeout. |
+| `file_types` | Removed: no file-type coverage check. |
+| `config_path`, `config_dict`, `configured_catalog_path`, `empty_streams` (`name`, `bypass_reason`), `status`, `validate_schema`, `bypass_reason` | Read. `bypass_reason` is reserved for the upcoming rule that a connector without test configs must say why. |
+| `spec_path`, `deployment_mode` and `backward_compatibility_tests_config` in `spec`; `deployment_mode` in `connection`; `backward_compatibility_tests_config` in `discovery`; `client_container_config`, `expect_trace_message_on_failure` and `validate_state_messages` in `basic_read`; `client_container_config` in `full_refresh` | Accepted in that section only. `backward_compatibility_tests_config.disable_for_version` of a `spec` entry is read by the spec backward-compatibility test; the others are CAT keys without a Standard Test counterpart yet. |
+| `sections`, `test_strictness_level` | Rejected on an entry: the Standard Tests fill them in. `sections` comes from the section that lists the entry; `test_strictness_level` is read only at the top level of the file. |
+| Anything else | Rejected as unknown. |
+
+Entries are checked even when they are not run (no `config_path`, or an `iam_role` config). The `incremental` section is not collected and is not checked. A config value that fails validation (an unknown `status`, for example) is reported the same way, without printing the value.
+
+Migration steps:
+
+- Remove the keys the error names from `acceptance-test-config.yml`. Keep the recorded-records files that `expect_records.path` pointed at: they stay useful as a reference for the records a connector produces. These are test-only changes and need no version bump.
+- Fix the spelling of a misspelled key; the error lists the keys the section accepts.
+- Python code that builds `ConnectorTestScenario` with `expect_records`, `timeout_seconds` or `file_types`, or uses `ConnectorTestScenario.AcceptanceTestExpectRecords` or `AcceptanceTestFileTypes`, must drop them.
+
+Rationale: a key that parses and is ignored reads like a check that runs. A connector with `expect_records` looks like its records are compared against a recorded file, and they are not. Removal is loud rather than silent, and a hard error rather than a deprecation window: the cleanup that removes these keys from every Airbyte-maintained connector (the monorepo and the enterprise connectors) merges before this release, so none of them fails on it.
+
 ## Upgrading to 7.36.0
 
 This release of the CDK adds checks to the Standard Tests (`airbyte-cdk connector test` and `airbyte-cdk image test`) and changes which commands they run for the configs listed in `acceptance-test-config.yml`. There is no change to how connectors run. The release is a minor one, since only the test harness changes, but the connector test suites of connectors that do not satisfy the new expectations start failing. The connector monorepo installs the `airbyte-cdk` CLI unpinned (`uv tool install --upgrade 'airbyte-cdk[dev]'`), so connector CI picks the new behaviour up with this release, without a per-connector CDK bump.