test(e2e): declarative YAML metric-test rig (replaces the CSV rig) - #1380
Conversation
…ormat Declarative YAML fixture format (WIP proposal): - fixtures/schemas/*.yaml — per-table JSON schemas (all real Airbyte fields). - fixtures/templates/*.yaml — record templates with extend/$ref composition; base records carry every schema field (incl. _airbyte_* used by transforms). - fixtures/collab_ emails_sent.yaml — example: IC Bullet Collaboration (…0012) team median over m365_emails_sent, with a real Airbyte re-sync duplicate. Uses the batch endpoint POST /v1/metrics/queries and an `expect` array with mongo-style `find` + `equal`/CEL `assert`. Rig changes for the bronze->API path: - migration_applier.reapply_migrations: recreate gold views after dbt so the date-filtered query matches the real silver schema (Code 80 was a rig artifact; verified clean on dev). - conftest: session-start truncation of multi-reader collab silver/staging. - test_fixtures: two-pass dbt build (staging then silver). - dbt_runner: profile host from session config (not hardcoded 127.0.0.1). - analytics_api: non-nil tenant_default so metrics stay visible post constructorfabric#522. - create-bronze-placeholders: m365 bronze placeholders aligned to the real Airbyte schema (counts as Decimal(38,9)). - meta/test_dbt_runner: assert host from config. Remove legacy spec.yaml fixtures (people_smoke, tasks_closed_smoke). NOTE (WIP): the loader for the new YAML format is not implemented yet, so e2e discovery currently finds no runnable fixtures. Opened as draft. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 35 minutes and 21 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the CSV folder-based e2e test rig with a declarative YAML format ( ChangesDeclarative YAML E2E Test Rig
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over conftest,fixture_loader: Collection time
conftest->>fixture_loader: discover_tests(specs_root)
fixture_loader-->>conftest: [test_paths]
conftest->>fixture_loader: load(test_path)
fixture_loader->>ref_resolver: resolve(bronze_record, ctx_file)
ref_resolver-->>fixture_loader: resolved_record
fixture_loader->>schema_validator: pad_and_validate(record, schema)
schema_validator-->>fixture_loader: padded_record
fixture_loader-->>conftest: TestYaml(bronze, cases)
end
rect rgba(144, 238, 144, 0.5)
note over test_fixtures,expect_engine: Per-test execution
test_fixtures->>CHSeeder: seed_bronze(bronze)
CHSeeder->>ClickHouse: INSERT rows (incl. duplicates)
test_fixtures->>DbtRunner: derive_selectors(touched_tables)
DbtRunner->>DbtRunner: parse manifest.json
DbtRunner-->>test_fixtures: staging_selectors, silver_selectors
test_fixtures->>DbtRunner: dbt build (staging pass)
test_fixtures->>DbtRunner: dbt build (silver pass)
test_fixtures->>migration_applier: reapply_migrations(cfg)
test_fixtures->>AnalyticsApiProcess: call_request(case.request)
AnalyticsApiProcess->>analytics_api: POST /v1/metrics/queries
analytics_api-->>AnalyticsApiProcess: (status_code, payload)
test_fixtures->>expect_engine: evaluate_case(case, batch, status)
expect_engine->>expect_engine: _select_result, _find, _eval_cel
expect_engine-->>test_fixtures: pass or ExpectError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Makes the YAML format runnable (the prior commit was format-only/WIP) and fully replaces the spec.yaml/CSV rig. Runner (src/ingestion/tests/e2e/e2e_lib/): - ref_resolver: $ref + sibling-override composition (cross-file, cycle-guarded, base resolves in its own file's context). - schema_validator: resolve schema by table name, pad missing cols to null, jsonschema-validate (additionalProperties:false catches typos). - expect_engine: `in` (pick batch result by id) + mongo-style `find` + `equal` subset + CEL `assert` (via cel-python; response numbers floatified so int/double compares work, `status` kept int). - fixture_loader rewritten (discover *.test.yaml, resolve+validate bronze rows). - ch_seeder seeds resolved records (typed coercion; dups inserted physically). - analytics_api.call_request → batch POST /v1/metrics/queries. - dbt_runner.derive_selectors (bronze table → staging models + silver tags from the manifest); test_fixtures: truncate → seed → 2-pass dbt → reapply migrations (recreate gold views vs real silver) → refresh MV → batch call → evaluate expect. - Removed csv_asserter, spec_schema and the CSV-era meta tests; added meta/test_ref_resolver.py (12 invariants) + meta/test_expect_engine.py. - pyproject: add cel-python. - placeholders: bronze_bamboohr.employees (29 cols) + bronze_m365.email_activity (+isDeleted/assignedProducts) aligned to the real Airbyte schema. Reference test: fixtures/collab_emails_sent.test.yaml (IC bullet …0012 team median over m365_emails_sent, with a deduping re-sync duplicate). Fresh `./e2e.sh test` = 37 passed. Specs: docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md (+ DESIGN.md v1.1, artifacts.toml). Skill: .claude/skills/metric-e2e-test (guide + scaffold). Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l-487399 # Conflicts: # src/ingestion/scripts/create-bronze-placeholders.sh # src/ingestion/tests/e2e/e2e_lib/analytics_api.py
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/meta/test_expect_engine.py (1)
82-90: ⚡ Quick winAdd selector regression tests for
$existsand invalid operators.Please add cases asserting
{field: {"$exists": true}}withnullvalues and that unknown operators (e.g.$foo) fail fast; this protects the selector contract from silent drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/meta/test_expect_engine.py` around lines 82 - 90, Add two new regression test functions following the same pattern as the existing tests like test_mongo_operator_in_find and test_in_optional_with_single_result. Create a test that uses _case with a selector checking that fields with null values correctly match or don't match a {"$exists": true} condition, and another test that verifies an unknown operator like "$foo" fails fast rather than silently passing, using evaluate_case with _batch() and appropriate assertions or expected error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/metric-e2e-test/SKILL.md:
- Around line 126-157: Replace the hardcoded personal KUBECONFIG path in the
kubectl exec command with an environment variable placeholder (such as
$KUBECONFIG or similar). In the Python validation script that follows, update
the file path references from `schemas/<db>.<table>.yaml` and
`templates/<group>.yaml` to use the documented `fixtures/` directory paths
instead, so that the example is portable across different environments and user
setups.
In `@cypilot/config/artifacts.toml`:
- Around line 872-876: Remove the duplicated array header declaration that
creates an empty artifact item. In the systems.artifacts section containing the
artifact with name "Bronze-to-API E2E Tests — Declarative YAML Rig Feature" and
path "docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md", delete
the first [[systems.artifacts]] line that appears before the kind, path, name,
and traceability fields. This empty array item could cause validation failures
or result in blank entries when the configuration is consumed.
In `@docs/domain/bronze-to-api-e2e/specs/DESIGN.md`:
- Line 413: In the API table row for the POST `/v1/metrics/queries` endpoint,
the pipe character in the cell content `items|error` is being interpreted as a
table column separator rather than literal text. Escape the pipe character by
replacing `items|error` with `items\|error` so the markdown table renders with
the correct cell count.
In `@docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md`:
- Line 278: The Touches section in this DoD references an outdated file path
`src/ingestion/tests/e2e/e2e_lib/api_client.py` that no longer matches the
current implementation. Update the file path reference to `analytics_api.py` to
point readers to the correct location where the rewritten runner implementation
actually lives, keeping the component reference
`cpt-bronze-to-api-e2e-component-api-client` and API endpoint intact.
- Line 32: The TOC link for the reference test has an incorrect anchor fragment
that doesn't match the rendered markdown slug. Update the fragment in the TOC
link from `#reference-test-collabemailssent` to `#reference-test-collab_emails_sent`
to match the actual anchor slug that the markdown renderer generates when
processing the heading "Reference Test (collab_emails_sent)". The underscore in
collab_emails_sent needs to be preserved in the anchor fragment.
- Around line 158-166: The algorithm reference
`cpt-bronze-to-api-e2e-algo-csv-rig-truncate-touched` in the first step still
contains "csv-rig" in its name, which references the retired CSV rig system.
Rename this algorithm identifier to reflect the new YAML rig system (replacing
"csv-rig" with "yaml-rig" in the name) to maintain proper traceability chain and
clarify that this algorithm is part of the current YAML flow specification.
- Around line 151-157: Update the validation example paths in the FEATURE.md
file to reference the correct fixture directories. The example currently uses
relative paths like schemas/ and templates/ which do not exist when running from
the documented location. Change these path references to point to
fixtures/schemas/ and fixtures/templates/ respectively to match the documented
directory layout and ensure the example will execute correctly.
In `@src/ingestion/scripts/create-bronze-placeholders.sh`:
- Around line 224-226: The columns cost_cents, prs_with_cc_count, and
prs_total_count are currently defined as Nullable(Float64), but since these
represent exact values (cents and counts), they should use exact numeric types
instead to avoid floating-point rounding errors. Change these three column
definitions from Nullable(Float64) to an appropriate exact integer type such as
Nullable(Int64) or Nullable(UInt64) depending on whether negative values are
expected.
In `@src/ingestion/tests/e2e/e2e_lib/analytics_api.py`:
- Around line 259-260: The conditional check at lines 259-260 restricts body
serialization to only POST and PUT methods, causing request bodies to be dropped
for PATCH and DELETE methods that may also carry payloads. Update the method
check in the condition `if method in ("POST", "PUT")` to also include PATCH and
DELETE, or refactor to check if body is not None regardless of the HTTP method,
ensuring the body is properly serialized for all relevant HTTP methods that can
accept a request payload.
In `@src/ingestion/tests/e2e/e2e_lib/ch_seeder.py`:
- Around line 124-127: The boolean coercion logic in the Boolean/Bool type
handling is too permissive and silently converts non-canonical input values
(like "2", " yes ", or "TRUE ") to False, corrupting test data. Instead of
relying solely on the `str(value).lower() in ("true", "1")` check, implement
stricter validation that explicitly handles valid boolean representations and
raises an error or returns a clear exception for invalid or ambiguous input
values. This ensures that non-canonical inputs are rejected rather than silently
producing incorrect boolean values that compromise test seeding integrity.
In `@src/ingestion/tests/e2e/e2e_lib/expect_engine.py`:
- Around line 30-56: The _match_value function has two correctness issues:
unknown operators starting with $ are silently accepted instead of raising an
error, and the $exists operator implementation is broken because _find uses
.get() which cannot distinguish between a missing key and a key with a None
value. Fix this by: (1) adding an else clause that raises an error for any
unrecognized $ operator, and (2) modifying _match_value to accept both the item
dictionary and field name as parameters so it can check key presence using the
`in` operator for the $exists check rather than relying on whether the actual
value is None, then update the _find function's call to _match_value to pass the
item and field name.
In `@src/ingestion/tests/e2e/e2e_lib/fixture_loader.py`:
- Around line 81-83: The current code assumes doc.get("bronze") returns a
mapping when it is not None, but if it's a malformed value like a list or
string, calling .items() will raise AttributeError instead of a clean
FixtureError. Add a validation check to ensure the bronze value is a
dictionary/mapping before attempting to iterate over its items using .items().
If bronze is not a mapping, raise a FixtureError with the path and a message
indicating that bronze must be a mapping.
In `@src/ingestion/tests/e2e/e2e_lib/migration_applier.py`:
- Around line 70-75: The reapply_migrations function should fail fast when no
migration files are discovered instead of silently returning success. After the
files list is created from cfg.migrations_dir.glob("*.sql") on line 70, add a
check to verify that files is not empty and raise an appropriate error if it is
empty. Since apply_all already implements this guard, this will make the
behavior consistent across both functions and prevent misconfigured test setups
from going undetected.
In `@src/ingestion/tests/e2e/e2e_lib/ref_resolver.py`:
- Around line 70-75: The resolve() function in ref_resolver.py does not handle
list elements, causing any $ref entries nested inside lists to remain
unresolved. Add a case to check if the node is a list and recursively call
resolve() on each element in the list, returning the list with all resolved
elements. This check should be placed after the isinstance(node, dict) check so
that lists are properly traversed and any nested $ref references within list
items are resolved.
In `@src/ingestion/tests/e2e/e2e_lib/schema_validator.py`:
- Around line 34-37: The yaml.safe_load call can raise exceptions for invalid
YAML, and the subsequent doc.get call assumes doc is a mapping which may not be
guaranteed. Wrap the yaml.safe_load call in a try-except block to catch YAML
parsing exceptions, and validate that both the parsed doc and the schemas value
are dictionaries before calling get(). Convert any exceptions encountered
(invalid YAML or invalid schema structure) into SchemaError to ensure consistent
error handling expected by the FixtureError wrapper in the fixture loader.
In `@src/ingestion/tests/e2e/fixtures/test_fixtures.py`:
- Around line 65-67: The code accesses case["request"] without first validating
that each case object contains the required keys, which can result in a raw
KeyError if a case is malformed. Add explicit schema validation in the
fixture_loader.load function to check that each case in the cases list contains
the required "request" key before the test execution loop reaches it. When
validation fails, raise an error with a clear, descriptive message that
identifies what schema requirement was violated, rather than allowing a raw
KeyError to propagate.
---
Nitpick comments:
In `@src/ingestion/tests/e2e/meta/test_expect_engine.py`:
- Around line 82-90: Add two new regression test functions following the same
pattern as the existing tests like test_mongo_operator_in_find and
test_in_optional_with_single_result. Create a test that uses _case with a
selector checking that fields with null values correctly match or don't match a
{"$exists": true} condition, and another test that verifies an unknown operator
like "$foo" fails fast rather than silently passing, using evaluate_case with
_batch() and appropriate assertions or expected error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53858350-0937-431c-80b0-cf877cb6c89e
⛔ Files ignored due to path filters (7)
src/ingestion/tests/e2e/fixtures/people_smoke/bronze/bronze_bamboohr.employees.csvis excluded by!**/*.csvsrc/ingestion/tests/e2e/fixtures/people_smoke/expected/response.csvis excluded by!**/*.csvsrc/ingestion/tests/e2e/fixtures/tasks_closed_smoke/bronze/bronze_bamboohr.employees.csvis excluded by!**/*.csvsrc/ingestion/tests/e2e/fixtures/tasks_closed_smoke/expected/response.csvis excluded by!**/*.csvsrc/ingestion/tests/e2e/fixtures/tasks_closed_smoke/silver/class_focus_metrics.csvis excluded by!**/*.csvsrc/ingestion/tests/e2e/fixtures/tasks_closed_smoke/silver/class_task_field_history.csvis excluded by!**/*.csvsrc/ingestion/tests/e2e/fixtures/tasks_closed_smoke/silver/class_task_users.csvis excluded by!**/*.csv
📒 Files selected for processing (34)
.claude/skills/metric-e2e-test/SKILL.mdcypilot/config/artifacts.tomldocs/domain/bronze-to-api-e2e/specs/DESIGN.mddocs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.mdsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e_lib/__init__.pysrc/ingestion/tests/e2e/e2e_lib/analytics_api.pysrc/ingestion/tests/e2e/e2e_lib/ch_seeder.pysrc/ingestion/tests/e2e/e2e_lib/csv_asserter.pysrc/ingestion/tests/e2e/e2e_lib/dbt_runner.pysrc/ingestion/tests/e2e/e2e_lib/expect_engine.pysrc/ingestion/tests/e2e/e2e_lib/fixture_loader.pysrc/ingestion/tests/e2e/e2e_lib/migration_applier.pysrc/ingestion/tests/e2e/e2e_lib/ref_resolver.pysrc/ingestion/tests/e2e/e2e_lib/schema_validator.pysrc/ingestion/tests/e2e/e2e_lib/spec_schema.pysrc/ingestion/tests/e2e/fixtures/collab_emails_sent.test.yamlsrc/ingestion/tests/e2e/fixtures/people_smoke/spec.yamlsrc/ingestion/tests/e2e/fixtures/schemas/bronze_bamboohr.employees.yamlsrc/ingestion/tests/e2e/fixtures/schemas/bronze_m365.email_activity.yamlsrc/ingestion/tests/e2e/fixtures/tasks_closed_smoke/spec.yamlsrc/ingestion/tests/e2e/fixtures/templates/m365_email.yamlsrc/ingestion/tests/e2e/fixtures/templates/people.yamlsrc/ingestion/tests/e2e/fixtures/test_fixtures.pysrc/ingestion/tests/e2e/meta/test_api_response.pysrc/ingestion/tests/e2e/meta/test_ch_seeder.pysrc/ingestion/tests/e2e/meta/test_csv_asserter.pysrc/ingestion/tests/e2e/meta/test_dbt_runner.pysrc/ingestion/tests/e2e/meta/test_expect_engine.pysrc/ingestion/tests/e2e/meta/test_fixture_loader.pysrc/ingestion/tests/e2e/meta/test_ref_resolver.pysrc/ingestion/tests/e2e/meta/test_snapshot_ci_guard.pysrc/ingestion/tests/e2e/pyproject.toml
💤 Files with no reviewable changes (9)
- src/ingestion/tests/e2e/fixtures/tasks_closed_smoke/spec.yaml
- src/ingestion/tests/e2e/fixtures/people_smoke/spec.yaml
- src/ingestion/tests/e2e/e2e_lib/spec_schema.py
- src/ingestion/tests/e2e/meta/test_api_response.py
- src/ingestion/tests/e2e/meta/test_ch_seeder.py
- src/ingestion/tests/e2e/meta/test_snapshot_ci_guard.py
- src/ingestion/tests/e2e/e2e_lib/csv_asserter.py
- src/ingestion/tests/e2e/meta/test_fixture_loader.py
- src/ingestion/tests/e2e/meta/test_csv_asserter.py
| - [Typed Bronze Seed from Records](#typed-bronze-seed-from-records) | ||
| - [Batch API Roundtrip](#batch-api-roundtrip) | ||
| - [Expect Engine](#expect-engine) | ||
| - [Reference Test (collab_emails_sent)](#reference-test-collabemailssent) |
There was a problem hiding this comment.
Fix the TOC fragment for the reference test.
This target appears to miss the rendered anchor for Reference Test (collab_emails_sent), so the TOC link will stay broken. Update it to the actual slug used by the markdown renderer.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 32-32: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md` at line 32,
The TOC link for the reference test has an incorrect anchor fragment that
doesn't match the rendered markdown slug. Update the fragment in the TOC link
from `#reference-test-collabemailssent` to `#reference-test-collab_emails_sent` to
match the actual anchor slug that the markdown renderer generates when
processing the heading "Reference Test (collab_emails_sent)". The underscore in
collab_emails_sent needs to be preserved in the anchor fragment.
Source: Linters/SAST tools
|
|
||
| **Input**: a resolved `TestYaml` (`bronze`, `cases`), `WorkerContext` | ||
|
|
||
| **Output**: `Pass` | `Fail(failing rule report)` | ||
|
|
||
| **Steps**: | ||
|
|
There was a problem hiding this comment.
Point the validation example at fixtures/.
The snippet is meant to run from src/ingestion/tests/e2e, but it opens schemas/... and templates/... as if those directories were local. That path does not match the documented layout under fixtures/, so the example will fail as written.
Fix
- s=set(yaml.safe_load(open("schemas/<db>.<table>.yaml"))["schemas"]["<db>.<table>"]["properties"])
- t=set(yaml.safe_load(open("templates/<group>.yaml"))["templates"]["<base>"]); t.discard("$ref")
+ s=set(yaml.safe_load(open("fixtures/schemas/<db>.<table>.yaml"))["schemas"]["<db>.<table>"]["properties"])
+ t=set(yaml.safe_load(open("fixtures/templates/<group>.yaml"))["templates"]["<base>"]); t.discard("$ref")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md` around lines
151 - 157, Update the validation example paths in the FEATURE.md file to
reference the correct fixture directories. The example currently uses relative
paths like schemas/ and templates/ which do not exist when running from the
documented location. Change these path references to point to fixtures/schemas/
and fixtures/templates/ respectively to match the documented directory layout
and ensure the example will execute correctly.
| for case in test_yaml.cases: | ||
| status, payload = analytics_api.call_request(case["request"]) | ||
| if status != 200: |
There was a problem hiding this comment.
Validate case shape before dereferencing case["request"].
load() only enforces that cases is a non-empty list, so a malformed case can trigger a raw KeyError here. Add an explicit schema check (preferably in fixture_loader.load) so invalid fixtures fail with a precise error message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/fixtures/test_fixtures.py` around lines 65 - 67, The
code accesses case["request"] without first validating that each case object
contains the required keys, which can result in a raw KeyError if a case is
malformed. Add explicit schema validation in the fixture_loader.load function to
check that each case in the cases list contains the required "request" key
before the test execution loop reaches it. When validation fails, raise an error
with a clear, descriptive message that identifies what schema requirement was
violated, rather than allowing a raw KeyError to propagate.
Add the table of variables available in an `expect[].assert` CEL expression (it / items / result / results / status) and point to where they are set — `e2e_lib/expect_engine.py::evaluate_case` (bindings dict) / `_eval_cel` — across: - spec: feature-yaml-rig FEATURE (eval-expect algo) + DESIGN expect-engine component - doc: src/ingestion/tests/e2e/README.md (new cases/expect section) - skill: .claude/skills/metric-e2e-test - a CANONICAL-source comment in expect_engine.py Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- git mv src/ingestion/tests/e2e/fixtures → specs (tests are declarative specs now). Updated all references: conftest (_SPECS_ROOT), fixture_loader (discover_tests param + _find_schemas_dir parent name), pytest.ini testpaths, pyproject exclude, e2e.sh scaffold path, docstrings; and the path refs in the yaml-rig FEATURE, DESIGN (v1.1 components), README, and the /metric-e2e-test skill. Removed a stray empty fixtures/ai. - README: add a "What is CEL" section (what the language is, that asserts are evaluated via cel-python in expect_engine._eval_cel, operators/macros, examples). Fresh `./e2e.sh test` = 37 passed (runs from specs/test_fixtures.py). Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| if isinstance(obj, bool): | ||
| return obj | ||
| if isinstance(obj, int): | ||
| return float(obj) |
There was a problem hiding this comment.
Is it ok about precision lost on big numbers?
1000000000000000.0
>>> a = int(1_123_123_123_123_123_123)
>>> b = float(a)
>>> b
1.1231231231231231e+18
>>> c = int(b)
>>> c
1123123123123123072
|
|
||
|
|
||
| def _find(items: list[dict], selector: dict) -> list[dict]: | ||
| return [it for it in items if all(_match_value(it.get(f), c) for f, c in selector.items())] |
There was a problem hiding this comment.
Is there any test using this notation? I saw plain eq not $eq
| This skill writes and validates `*.test.yaml` fixtures that drive the full | ||
| `bronze → dbt silver → gold view → analytics-api` path and assert the result. | ||
|
|
||
| ## Source of truth (read THIS turn before authoring) |
There was a problem hiding this comment.
Is that required for this skill? We could reference those as sources, but does it required to load into context every time?
|
|
||
| ## Scaffolding a new test | ||
|
|
||
| 1. **Resolve the metric_id and its shape.** Find it in the seed catalog |
There was a problem hiding this comment.
Let's create script that will query all existing metrics and their final schemas? Probably not in this pr
| | `status` | the batch HTTP status code (int) | always | | ||
|
|
||
| Numbers under `it`/`items`/`result`/`results` are float-coerced (CEL won't compare | ||
| `int` to `double`) → compare metric values with float literals (`it.value > 39.5`); |
There was a problem hiding this comment.
Which file type comparisons supported? Worth addition additional docs.
| 2. **Ensure a schema file per table.** If `schemas/<db>.<table>.yaml` is missing, | ||
| generate it from the REAL table (do not invent columns): | ||
| ```bash | ||
| export KUBECONFIG=/Users/roman/alemira/insight/access/dev-vhc/insight-k8s.kubeconfig |
There was a problem hiding this comment.
Hardcode. Let's use local run related.
| # --------------------------------------------------------------------------- | ||
|
|
||
| def _match_value(actual: Any, cond: Any) -> bool: | ||
| if isinstance(cond, dict) and any(k.startswith("$") for k in cond): |
There was a problem hiding this comment.
You bring ceply, why you need to invent other language?
| - id: collaboration | ||
| metric_id: 00000000-0000-0000-0001-000000000012 | ||
| $top: 50 | ||
| $filter: "person_id eq 'alice@example.com' and metric_date ge '2026-01-01' and metric_date le '2026-01-31'" |
There was a problem hiding this comment.
not $eq, not !=.... same in skill.
…ening - expect_engine: `find` now raises on an unknown `$`-operator (was silently matching); added regression tests for unknown-op and `$exists` vs null. - ref_resolver: resolve list elements recursively so a `$ref` nested in a list value (or a top-level list) is no longer skipped; added tests. - ch_seeder: strict Bool coercion — raise on non-canonical values instead of silently yielding False. - analytics_api.call_request: send the JSON body for any method (was dropped for non-POST/PUT). - fail-fast guards: fixture_loader validates `bronze` is a mapping and each `case` has request+expect; schema_validator wraps YAML/shape errors as SchemaError; migration_applier.reapply_migrations errors if no migrations. - docs: DESIGN — escape `|` in the batch-endpoint table cell; SKILL — replace the hardcoded personal KUBECONFIG path with a placeholder. Unit tests 24 passed; full `./e2e.sh test` = 41 passed. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @coderabbitai — addressed the actionable findings in Applied:
Unit tests: 24 passed; full Not applied (stale or false positive):
|
|
Tip For best results, initiate chat on the files or code changes.
The fail-fast guard additions ( If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Per review (@cyberantonz): the rig already depends on CEL, so the Mongo-style `find` operator mini-language was redundant — and only plain equality was ever used. - expect_engine: `find` is now exact field-equality only (removed $eq/$ne/$gt/$gte/$lt/$lte/$in/$regex/$exists and the operator-hardening). - expect_engine: removed the blanket float-coercion of bindings (it silently lost precision on large ints). CEL is strictly typed, so authors cast explicitly: `double(it.value) > 39.5`. Updated tests + docs accordingly. - specs/docs/skill (FEATURE, DESIGN, README, SKILL): `find` documented as exact equality; "richer matching → CEL assert"; double()-cast guidance replaces the float-coercion note. Unit tests 22 passed; full `./e2e.sh test` = 39 passed. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- "Source of truth" is now reference-only (no "read every time" mandate). - "Running" lists existing tests + run-all / run-one / down commands. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @cyberantonz / @ktursunov — addressed in @cyberantonz — expect engine
@ktursunov — skill
|
…l-487399 # Conflicts: # src/ingestion/tests/e2e/fixtures/tasks_closed_smoke/expected/response.csv
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ingestion/tests/e2e/e2e.sh (1)
74-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
newstill scaffolds legacy CSV fixture format underspecs/, so generated tests are not runnable.Line 74 moves output to
specs/, but the scaffold still writesspec.yaml,bronze/*.csv.todo, and snapshot instructions. The new runner only discovers*.test.yamlunderspecs/, so./e2e.sh new ...creates fixtures that pytest won’t collect.Proposed direction
- mkdir -p "$dir/bronze" "$dir/expected" + mkdir -p "$dir" - cat > "$dir/spec.yaml" <<EOF -spec_version: 1 -description: > - TODO describe what this fixture exercises. -... -EOF + cat > "$dir.test.yaml" <<EOF +name: $name +bronze: + # bronze_<db>.<table>: + # - \$ref: templates/<template>.yaml#/... +cases: + - name: "happy_path" + request: + method: POST + path: /v1/metrics/queries + json: + # TODO: batch payload + queries: [] + expect: + # TODO: find/equal/assert rules + - equal: + [] +EOF - cat > "$dir/bronze/$bronze_tbl.csv" <<EOF -... -EOF - mv "$dir/bronze/$bronze_tbl.csv" "$dir/bronze/$bronze_tbl.csv.todo"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/e2e.sh` around lines 74 - 123, The scaffold code in the `new` subcommand (around the cat commands that write spec.yaml and bronze CSV files) is still generating fixtures in the old CSV format, but the new test runner only discovers *.test.yaml files. Update the scaffold to write a *.test.yaml file instead of spec.yaml, and remove the bronze CSV scaffolding and .todo file handling since the new format doesn't use that structure. Also update the echo statements that provide next steps to reflect the new fixture format and workflow for *.test.yaml files.src/ingestion/tests/e2e/e2e_lib/expect_engine.py (1)
88-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce exactly one terminal assertion key (
equalxorassert)The current branch logic accepts both keys and silently skips
assertwhenequalis present, which violates the documented contract and can hide failing expectations.Suggested minimal fix
it = None if "find" in rule: matches = _find(items, rule["find"]) if len(matches) != 1: raise ExpectError( f"{where}: find {rule['find']} matched {len(matches)} rows (expected exactly 1)" ) it = matches[0] - if "equal" in rule: + has_equal = "equal" in rule + has_assert = "assert" in rule + if has_equal == has_assert: + raise ExpectError(f"{where}: rule must have exactly one of `equal` or `assert`") + + if has_equal: if it is None: raise ExpectError(f"{where}: `equal` requires a `find` that selects one row") for field, exp in rule["equal"].items(): got = it.get(field) if got != exp: raise ExpectError(f"{where}: {field}: expected {exp!r}, got {got!r}") - elif "assert" in rule: + else: # CANONICAL source of the CEL `assert` bindings (documented in the # yaml-rig FEATURE, DESIGN expect-engine component, README, and the # /metric-e2e-test skill). `it` is None unless this rule had a `find`. bindings = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/e2e_lib/expect_engine.py` around lines 88 - 113, The current if/elif branch structure silently skips the assert check when both equal and assert keys are present in a rule, violating the documented contract that rules must have exactly one terminal assertion key. Before the existing if/elif branches, add validation logic to explicitly check that the rule dictionary contains exactly one of the two keys (equal xor assert), and raise an ExpectError with a clear message if both keys are present, neither is present, or if the rule has unexpected keys.
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/meta/test_expect_engine.py (1)
73-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for invalid mixed rule shape (
equal+assert)This suite covers happy-path
equaland CEL assertions, but not the contract violation where both are present in one rule. A negative test here will prevent silent behavior drift.Test to add
+def test_rule_rejects_equal_and_assert_together(): + case = _case([{ + "in": "collab", + "find": {"metric_key": "m365_emails_sent"}, + "equal": {"value": 40}, + "assert": "double(it.value) > 39.5", + }]) + with pytest.raises(ExpectError, match="exactly one of `equal` or `assert`"): + evaluate_case(case, _batch(), 200)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/meta/test_expect_engine.py` around lines 73 - 87, The test suite covers happy-path usage of `equal` and `assert` fields separately in the test_cel_inequality_and_null and test_find_is_exact_equality_on_any_field functions, but lacks a regression test for the invalid contract violation where both fields are present in the same rule. Add a new test function after the existing test functions that creates a case rule with both `equal` and `assert` fields present simultaneously and verifies that evaluate_case properly rejects or raises an error for this invalid mixed rule shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/domain/bronze-to-api-e2e/specs/DESIGN.md`:
- Around line 141-145: The documentation in the "The table schema is the source
of truth for a row's shape" principle section references an outdated schema file
path convention. Update all instances of the path reference from
`specs/schemas/<table>.yaml` to `schemas/<db>.<table>.yaml` to match the actual
fixture layout used in this PR. Additionally, review and update any other
references to the Schema Validator in this same principle section that may
reference the old naming convention to ensure consistency throughout.
---
Outside diff comments:
In `@src/ingestion/tests/e2e/e2e_lib/expect_engine.py`:
- Around line 88-113: The current if/elif branch structure silently skips the
assert check when both equal and assert keys are present in a rule, violating
the documented contract that rules must have exactly one terminal assertion key.
Before the existing if/elif branches, add validation logic to explicitly check
that the rule dictionary contains exactly one of the two keys (equal xor
assert), and raise an ExpectError with a clear message if both keys are present,
neither is present, or if the rule has unexpected keys.
In `@src/ingestion/tests/e2e/e2e.sh`:
- Around line 74-123: The scaffold code in the `new` subcommand (around the cat
commands that write spec.yaml and bronze CSV files) is still generating fixtures
in the old CSV format, but the new test runner only discovers *.test.yaml files.
Update the scaffold to write a *.test.yaml file instead of spec.yaml, and remove
the bronze CSV scaffolding and .todo file handling since the new format doesn't
use that structure. Also update the echo statements that provide next steps to
reflect the new fixture format and workflow for *.test.yaml files.
---
Nitpick comments:
In `@src/ingestion/tests/e2e/meta/test_expect_engine.py`:
- Around line 73-87: The test suite covers happy-path usage of `equal` and
`assert` fields separately in the test_cel_inequality_and_null and
test_find_is_exact_equality_on_any_field functions, but lacks a regression test
for the invalid contract violation where both fields are present in the same
rule. Add a new test function after the existing test functions that creates a
case rule with both `equal` and `assert` fields present simultaneously and
verifies that evaluate_case properly rejects or raises an error for this invalid
mixed rule shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7477e86-a678-412d-9c17-012b1af9fb6c
📒 Files selected for processing (24)
.claude/skills/metric-e2e-test/SKILL.mddocs/domain/bronze-to-api-e2e/specs/DESIGN.mddocs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.mdsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e.shsrc/ingestion/tests/e2e/e2e_lib/analytics_api.pysrc/ingestion/tests/e2e/e2e_lib/ch_seeder.pysrc/ingestion/tests/e2e/e2e_lib/expect_engine.pysrc/ingestion/tests/e2e/e2e_lib/fixture_loader.pysrc/ingestion/tests/e2e/e2e_lib/migration_applier.pysrc/ingestion/tests/e2e/e2e_lib/ref_resolver.pysrc/ingestion/tests/e2e/e2e_lib/schema_validator.pysrc/ingestion/tests/e2e/meta/test_expect_engine.pysrc/ingestion/tests/e2e/meta/test_ref_resolver.pysrc/ingestion/tests/e2e/pyproject.tomlsrc/ingestion/tests/e2e/pytest.inisrc/ingestion/tests/e2e/specs/collab_emails_sent.test.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_bamboohr.employees.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_m365.email_activity.yamlsrc/ingestion/tests/e2e/specs/templates/m365_email.yamlsrc/ingestion/tests/e2e/specs/templates/people.yamlsrc/ingestion/tests/e2e/specs/test_fixtures.py
💤 Files with no reviewable changes (6)
- src/ingestion/tests/e2e/specs/templates/m365_email.yaml
- src/ingestion/tests/e2e/specs/schemas/bronze_bamboohr.employees.yaml
- src/ingestion/tests/e2e/specs/collab_emails_sent.test.yaml
- src/ingestion/tests/e2e/specs/schemas/bronze_m365.email_activity.yaml
- src/ingestion/tests/e2e/specs/templates/people.yaml
- src/ingestion/tests/e2e/specs/test_fixtures.py
✅ Files skipped from review due to trivial changes (2)
- src/ingestion/tests/e2e/pytest.ini
- .claude/skills/metric-e2e-test/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (10)
- src/ingestion/tests/e2e/e2e_lib/migration_applier.py
- src/ingestion/tests/e2e/e2e_lib/schema_validator.py
- src/ingestion/scripts/create-bronze-placeholders.sh
- src/ingestion/tests/e2e/pyproject.toml
- src/ingestion/tests/e2e/e2e_lib/analytics_api.py
- src/ingestion/tests/e2e/e2e_lib/ch_seeder.py
- src/ingestion/tests/e2e/e2e_lib/fixture_loader.py
- src/ingestion/tests/e2e/e2e_lib/ref_resolver.py
- src/ingestion/tests/e2e/conftest.py
- docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md
| ./e2e.sh test # run all tests (specs/ + meta/) | ||
| ./e2e.sh test -k <name> # run one test by name | ||
| ./e2e.sh test -k <name> -v # verbose (per-step log) | ||
| ./e2e.sh down # reset the stack; do this before a warm re-run |
There was a problem hiding this comment.
Why this script reset the stack? Deploy is for the ./dev-compose.sh
| return ApiResponse.from_httpx(response) | ||
| try: | ||
| payload = response.json() | ||
| except Exception: # noqa: BLE001 |
There was a problem hiding this comment.
Too broad exception catch. Suggest using exact json decode error exception catch
| LOG = logging.getLogger("e2e.runner") | ||
|
|
||
|
|
||
| def test_fixture( |
There was a problem hiding this comment.
Please rename to test_e2e_metric_smoke - in report test_fixture looks weird
| @@ -0,0 +1,92 @@ | |||
| """Unit tests for expect_engine (DoD cpt-bronze-to-api-e2e-dod-yaml-expect-engine). | |||
There was a problem hiding this comment.
What's that test is about? test on test harness?
There was a problem hiding this comment.
Yes — these (and test_ref_resolver.py) are unit tests of the test harness's own logic, not of any product metric. meta/ is where the rig tests itself.
This one covers expect_engine: in result selection, exact-equality find, equal subset matching, and CEL assert evaluation (incl. the result.status/size(items)/double() cases). They're pure and fast — no ClickHouse/dbt — so they catch regressions in the expect engine in milliseconds, independently of the full e2e run. Cheap insurance for the matcher that decides whether every metric test passes; I'd keep them.
| f"stderr tail:\n{result.stderr[-1000:]}" | ||
| ) | ||
|
|
||
| def derive_selectors(self, tables: set[tuple[str, str]]) -> tuple[list[str], list[str]]: |
There was a problem hiding this comment.
Silver layer is not needed for e2e tests now, suggest skipping it completely now
There was a problem hiding this comment.
This rig tests the whole path from raw data to what the API returns — bronze (seeded) → dbt staging → dbt silver class_* → insight.collab_bullet_rows (gold view) → /v1/metrics/queries. The gold view reads silver.class_collab_*, so without the silver dbt transformations the view reads an empty placeholder and the metric returns zero rows — there's nothing left to assert.
Note we don't seed silver at all anymore; it's built by dbt from the seeded bronze (derive_selectors returns the staging models to build first, then their silver:<class> targets). The only way to "skip silver dbt" would be to go back to seeding silver tables directly — the approach we intentionally dropped, since it stops exercising the bronze→silver dedup/transform. So the silver build stays. Did you have a specific simplification in mind?
| @@ -0,0 +1,150 @@ | |||
| """Unit tests for ref_resolver — the 12 invariants of | |||
There was a problem hiding this comment.
Is this test required for test harness?
There was a problem hiding this comment.
Same as the test_expect_engine.py thread: this is a harness self-test under meta/, not a product test. It pins the $ref/sibling-override resolver — the 12 invariants from the FEATURE DoD (local & cross-file refs, override precedence, nested-ref-in-its-own-file, deep merge, cycle detection, missing file/pointer errors, list elements). Pure and fast (no ClickHouse/dbt). Since every fixture's bronze rows are built by this resolver, a regression here would silently corrupt seed data — so the unit coverage is worth keeping.
- analytics_api.call_request: narrow the JSON-parse except to json.JSONDecodeError (was a bare Exception catch). - rename the parametrized runner test_fixture → test_e2e_metric_smoke so the pytest report id is meaningful; updated the conftest generate hook. - SKILL: clarify that `./e2e.sh down` is the e2e compose teardown (not a deploy) and that warm re-runs are fine (session-start reset) — no need to down first. - docs (DESIGN, FEATURE): schema filename convention is `specs/schemas/<db>.<table>.yaml` (e.g. bronze_m365.email_activity.yaml), not `<table>.yaml`. Unit tests 22 passed; full ./e2e.sh test = 39 passed (now test_e2e_metric_smoke[...]). Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o claude/festive-nobel-487399
Closes #1422
What
Replaces the per-folder CSV e2e rig with a single declarative
<name>.test.yamlformat, plus the specs and an authoring skill. The full path (bronze → dbt silver → gold view → analytics-api) is unchanged; only the authoring format and the assertion engine change.Format (
src/ingestion/tests/e2e/fixtures/)schemas/<db>.<table>.yaml— one JSON schema per bronze table (all real Airbyte columns; resolved by table name).templates/*.yaml— reusable records; a record is a field map with optional$ref: "<file>#/<json-pointer>"to inherit + sibling keys override (closest wins). Base records carry every column incl._airbyte_*(transforms depend on_airbyte_extracted_at).bronze:keyed by table → records ($ref+ the fields under test); padded to the full schema and validated at load; duplicate rows allowed (dedup must hold).cases:→requestto the batch endpointPOST /v1/metrics/queries;expectis a list of rules:in(pick result by id) / mongo-stylefind/equal(subset) or CELassert.Runner (
e2e_lib/)ref_resolver(compose),schema_validator(pad + validate),expect_engine(find + equal + CEL viacel-python).fixture_loaderrewritten;ch_seederseeds resolved records;analytics_api.call_requestdoes the batch call;dbt_runner.derive_selectorsderives the dbt models from the manifest;test_fixturesruns truncate → seed → 2-pass dbt → recreate gold views → refresh MV → batch → evaluate. Retirescsv_asserter/spec_schemaand the CSV-era meta tests; addsmeta/test_ref_resolver.py(12 invariants) +meta/test_expect_engine.py. Bronze placeholders forbamboohr.employees/m365.email_activityaligned to the real schema.Specs & skill
docs/domain/bronze-to-api-e2e/specs/feature-yaml-rig/FEATURE.md(+ DESIGN v1.1, artifacts.toml). Skill/metric-e2e-test(guide + scaffold).Status — green
Fresh
./e2e.sh test= 37 passed, incl. the referencecollab_emails_sent.test.yaml(IC bullet…0012team median overm365_emails_sent, with a re-sync duplicate that must dedup → value 40, median 20, range 10–40). Resolver + expect-engine covered by 20 pure unit tests.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
/v1/metrics/queriesbatch testing.Refactor
*.test.yamlfiles with$ref-driven bronze record composition and dedup scenarios.Documentation
Bug Fixes / Chores