fix(e2e): stop silently skipping the transformation suite; authenticate as a test tenant - #1287
Merged
mitasovr merged 5 commits intoJun 11, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRequire X-Insight-Tenant-Id on all analytics-api requests, re-tenant seeded metrics to a fixed test UUID, validate Rust toolchain from src/backend/Cargo.toml, change analytics-api build failures to hard fails, serialize CI E2E runs, and update docs and a placeholder schema. ChangesE2E Tenant Enforcement and Toolchain Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
Comment |
…Rust from Cargo.toml A `cargo build` failure made the analytics_api fixture `pytest.skip`, which silently skipped EVERY bronze→API transformation test — the suite stayed green while testing nothing. That is exactly how the runner's stale rustc 1.92 hid the fact that src/backend now requires 1.95. - conftest.py: the analytics_api fixture now `pytest.fail`s instead of skipping when the binary can't build. Identical behaviour locally and in CI — if the binary can't build, the transformation tests cannot run, so the only honest result is red. - analytics_api.py: drop the hardcoded MIN_CARGO_MINOR (pinned at 92 while the crates moved to 95). The required version is now read from the single source of truth — `[workspace.package].rust-version` in src/backend/Cargo.toml — so the precheck can't drift behind the real requirement again. - Dockerfile.runner: bump RUST_VERSION 1.92.0 -> 1.95.0 so the runner image can actually build the backend and run the transformation fixtures. - Refresh stale "1.92 / edition2024" mentions in the smoke-test docstring and README to point at Cargo.toml's rust-version. Not changed: zero-fixtures still skips (nothing to run), and a fixture without `dbt_selector` still passes without running dbt (view-only metrics, by design). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: insight-ci <ci@cyberfabric.local>
…sion log analytics-api's tenant middleware rejects requests without a resolvable non-nil tenant (auth.rs rejects the nil UUID by design), so after the binary finally built (rustc 1.95), /health polling got 400 and every fixture errored. Tests must work on top of the system as-is, so the fix is harness-only: - config.py: introduce TEST_TENANT_ID (1111…) + TENANT_HEADER constants. - analytics_api.py: send X-Insight-Tenant-Id on every request — both the /health readiness polling and fixture calls. Module docstring updated (it claimed "all requests resolve to nil UUID", the opposite of reality). - metric_seed.py: re-home migration-seeded metrics from the nil tenant onto TEST_TENANT_ID (find_enabled_metric filters the metrics table by exact tenant) and seed overrides under the same tenant. Idempotent UPDATE. - Fix NameError in build(): `version` was only bound when Cargo.toml parsing succeeded, but the log line below used it unconditionally — the graceful fallback path crashed instead of proceeding to the build. - Refresh stale comments (README Rust version, seed/metrics.yaml header). Note: migrations seeding `metrics` under the nil tenant means those rows are invisible to every real tenant in production too — that product gap needs its own backend fix; this commit only unblocks the harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: insight-ci <ci@cyberfabric.local>
…ge placeholder Migration 20260601000000_ai-claude-team-metrics.sql creates a gold view selecting c.cost_cents, c.prs_with_cc_count and c.prs_total_count from silver.class_ai_dev_usage, but the bootstrap placeholder for that table was not extended when those columns were added to the dbt model. On a fresh cluster (and in the e2e rig, which applies placeholders + migrations from scratch) ClickHouse validates the CREATE VIEW SELECT and fails with `Code: 47 UNKNOWN_IDENTIFIER: 'c.cost_cents'`, aborting all migrations — this is why the E2E suite on main has been red since 2026-06-04. Add the three columns as Nullable(UInt32), matching the connector staging models (claude_team__ai_dev_usage et al.). The placeholder is dropped and replaced by dbt with the real schema on the first run, so minimum-viable parity with the migrations is all that is required. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: insight-ci <ci@cyberfabric.local>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/ingestion/tests/e2e/meta/test_ci_workflow.py`:
- Around line 71-73: The current assertion only checks for "-n " and trailing
"-n" and misses attached forms like "-nauto"; update the guard to split the
command string (use shlex.split on test_step["run"]) and assert that no token
startswith("-n") to reject any -n variant. Replace the existing assertion that
references test_step["run"] with a check using the tokenized command and ensure
the failure message remains descriptive (CI must run the e2e suite serially
until the rig is xdist-safe).
🪄 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: 482532d2-4fa8-43e8-ac6d-b9ea9f1e68d7
📥 Commits
Reviewing files that changed from the base of the PR and between 3f2ce3beeea041f5e56a8bd09dbe0ce79e13f4b6 and 283a4eaf40595710c1786e714d5a1231972e3b73.
📒 Files selected for processing (3)
.github/workflows/e2e-bronze-to-api.ymlsrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/meta/test_ci_workflow.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ingestion/tests/e2e/README.md
Comment on lines
+71
to
+73
| assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), ( | ||
| "CI must run the e2e suite serially until the rig is xdist-safe" | ||
| ) |
There was a problem hiding this comment.
Serialization guard is bypassable with attached -n form.
This assertion only blocks "-n " and terminal "-n", so an attached form like -nauto would pass while still enabling parallelism. Tighten the check to reject any -n token variant.
Suggested fix
- assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), (
+ run_cmd = test_step["run"]
+ assert " -n " not in f" {run_cmd} "
+ assert " -nauto" not in f" {run_cmd} "
+ assert "\n-n " not in run_cmd
+ assert not run_cmd.rstrip().endswith("-n"), (
"CI must run the e2e suite serially until the rig is xdist-safe"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), ( | |
| "CI must run the e2e suite serially until the rig is xdist-safe" | |
| ) | |
| run_cmd = test_step["run"] | |
| assert " -n " not in f" {run_cmd} " | |
| assert " -nauto" not in f" {run_cmd} " | |
| assert "\n-n " not in run_cmd | |
| assert not run_cmd.rstrip().endswith("-n"), ( | |
| "CI must run the e2e suite serially until the rig is xdist-safe" | |
| ) |
🤖 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_ci_workflow.py` around lines 71 - 73, The
current assertion only checks for "-n " and trailing "-n" and misses attached
forms like "-nauto"; update the guard to split the command string (use
shlex.split on test_step["run"]) and assert that no token startswith("-n") to
reject any -n variant. Replace the existing assertion that references
test_step["run"] with a check using the tokenized command and ensure the failure
message remains descriptive (CI must run the e2e suite serially until the rig is
xdist-safe).
…-safe
With the suite finally reaching the analytics-api spawn (earlier commits in
this PR), `-n auto` exposed four cross-worker races on the shared data plane:
- each xdist worker spawns its own analytics-api, and concurrent SeaORM
migrations on the shared MariaDB collide (1061 Duplicate key
'uq_metric_catalog_metric_key');
- only the primary worker applies ClickHouse migrations and the others do
not wait, so tests run against a half-migrated database ('identity' not
yet created);
- meta/test_ch_seeder.py's module fixture DROP+CREATEs the same
bronze_e2e_test.events from several workers (TABLE_ALREADY_EXISTS);
- DbtRunner.cleanup() rmtree's the shared target/dbt dir, so whichever
worker finishes first deletes the profiles out from under the rest.
conftest.py has documented "do NOT support xdist yet" since the scaffold
landed; CI invoking `-n auto` contradicted that. Drop xdist from the CI
invocation (wall-time impact is negligible — the image and cargo builds
dominate the job), flip the meta test that previously REQUIRED `-n auto`
into one that forbids it until worker isolation lands, and mark the README
line accordingly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: insight-ci <ci@cyberfabric.local>
mitasovr
force-pushed
the
fix/e2e-fail-fast-and-test-tenant
branch
from
June 11, 2026 10:29
283a4ea to
b64c810
Compare
The E2E check is about to become a required status check on main. A path-filtered required check never reports on PRs outside the filter, so those PRs hang on "Expected" forever. Run the suite on every PR instead: the gate stays uniform and the serial suite costs ~4 minutes with a warm cargo cache. Flip the meta test that previously required specific entries in the paths filter into one that forbids any path filtering on pull_request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: insight-ci <ci@cyberfabric.local>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/ingestion/tests/e2e/meta/test_ci_workflow.py (1)
72-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncomplete check allows
-nautoand other attached forms to bypass the serialization requirement.The current assertion only checks for
-n(with trailing space) and terminal-n, so attached forms like-nautoor-n2would pass. Use tokenization to check if any token starts with-n.🔧 Recommended fix using simpler split approach
- assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), ( - "CI must run the e2e suite serially until the rig is xdist-safe" - ) + tokens = test_step["run"].split() + assert not any(tok.startswith("-n") for tok in tokens), ( + "CI must run the e2e suite serially until the rig is xdist-safe" + )🤖 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_ci_workflow.py` around lines 72 - 74, The assertion in test_ci_workflow.py uses a fragile string check on test_step["run"] that misses attached forms like "-nauto" or "-n2"; replace the check with a whitespace-tokenized inspection (split the run string into tokens and assert that no token starts with "-n") so any "-n" flag in attached or standalone form is detected (reference test_step["run"] in the failing assert and update the assertion message accordingly).
🤖 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.
Duplicate comments:
In `@src/ingestion/tests/e2e/meta/test_ci_workflow.py`:
- Around line 72-74: The assertion in test_ci_workflow.py uses a fragile string
check on test_step["run"] that misses attached forms like "-nauto" or "-n2";
replace the check with a whitespace-tokenized inspection (split the run string
into tokens and assert that no token starts with "-n") so any "-n" flag in
attached or standalone form is detected (reference test_step["run"] in the
failing assert and update the assertion message accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51c20cfe-b1e4-4d1f-8dec-7da9620135d7
📒 Files selected for processing (2)
.github/workflows/e2e-bronze-to-api.ymlsrc/ingestion/tests/e2e/meta/test_ci_workflow.py
Merged
This was referenced Jun 30, 2026
Merged
This was referenced Jul 24, 2026
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
1 participant
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.
Problem
The whole bronze→API transformation suite (
fixtures/test_fixtures.py) can go green while testing nothing: whencargo buildof analytics-api fails, the session-scopedanalytics_apifixture callspytest.skip, which silently skips every fixture test. Two real bugs hid behind this for a while (a stale-toolchain build failure and a MariaDB-incompatible seed migration — the latter already fixed on main).Even with the binary building, the suite still cannot pass today: analytics-api's tenant middleware sits in front of all routes (including
/health) and rejects requests without a resolvable non-nil tenant (auth.rsfilters out the nil UUID by design). The harness sent noX-Insight-Tenant-Idat all →/healthpolling got400→ spawn timeout → (previously) silent skip.Fix — harness only, no backend changes
1. Fail instead of skip (
conftest.py)pytest.fails. If the binary can't build, the transformation tests can't run — the only honest result is red. Identical behaviour locally and in CI.2. Required Rust version from the single source of truth (
e2e_lib/analytics_api.py)1.92whilesrc/backend/Cargo.tomlhad moved torust-version = "1.95.0"— exactly the drift that masked the build failure. The precheck now reads[workspace.package].rust-versionfrom Cargo.toml; on parse failure it degrades gracefully (thecargo builditself stays the hard gate).3. Authenticate as a non-nil test tenant (
e2e_lib/config.py,analytics_api.py,metric_seed.py)TEST_TENANT_ID(1111…1111) andTENANT_HEADER./healthreadiness polling and fixture calls — sendsX-Insight-Tenant-Id: <TEST_TENANT_ID>.metric_seed.seed_test_metricsre-homes migration-seededmetricsrows from the nil tenant ontoTEST_TENANT_ID(idempotent UPDATE) and seeds overrides under the same tenant, becausefind_enabled_metricfilters themetricstable by exact tenant.Plus: stale docs refreshed (README, module docstring,
seed/metrics.yamlheader — all claimed "everything resolves to the nil UUID", the opposite of current backend behaviour).Verified
Full suite green locally in the dockerized runner (
./e2e.sh test): 57 framework tests +test_analytics_api_health/test_analytics_api_lists_metrics+ both transformation fixtures (people_smoke,tasks_closed_smoke) passing — not skipped — for the first time in a while.Note for backend owners
Migrations seed
metricsunder the nil tenant, but the tenant middleware can never resolve nil — so those rows are invisible to every real tenant in any deployment. The harness re-tenant is a test-side workaround; the underlying product gap deserves its own fix (seed under the configured default tenant, or change the lookup).🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation
Chores
CI
Also: fix the placeholder drift that has kept E2E red on main since Jun 4
Migration
20260601000000_ai-claude-team-metrics.sqlselectsc.cost_cents,c.prs_with_cc_count,c.prs_total_countfromsilver.class_ai_dev_usage, but the bootstrap placeholder inscripts/create-bronze-placeholders.shwas never extended with those columns. On a fresh cluster / e2e rig, ClickHouse validates theCREATE VIEWSELECT and aborts all migrations withCode: 47 UNKNOWN_IDENTIFIER. That is why theE2E — Bronze to APIworkflow has been failing onmainfor the last 4 runs — independently of this PR.Added the three columns as
Nullable(UInt32)(matching all four AI connectors' staging projections). The migration references nothing else that is missing (onlysilver.class_ai_dev_usage+insight.people).Without this, the suite this PR un-skips cannot go green — the failure happens at session setup, before any test runs.
For repo admins: making this gate mandatory
The
paths:filter has been removed frome2e-bronze-to-api.yml, so the suite now runs on every PR tomain(~4 min serial with warm cargo cache). To block merging on it, add a ruleset / branch protection onmainwith Require status checks to pass and select the check namedRun E2E suite(plusDCO). With no path filter the check reports on every PR, so nothing will hang on "Expected".