DEV-1562: live-Metabase end-to-end pg-facade integration test suite#182
Open
ZmeiGorynych wants to merge 14 commits into
Open
DEV-1562: live-Metabase end-to-end pg-facade integration test suite#182ZmeiGorynych wants to merge 14 commits into
ZmeiGorynych wants to merge 14 commits into
Conversation
Drives a real `metabase/metabase:v0.62.1.5` container against two pg-serve instances (no-token for Metabase, token-protected for auth-error paths) and asserts every regression vector DEV-1558's 3-hour live debugging session surfaced, plus the broader behaviour matrix: bootstrap + sync, OID → type mapping, MBQL aggregations, time-grain breakouts, raw + native-SQL paths, filters, joins, transactions, wire-format round-trips, error envelopes, and concurrency. 62 cases, ~4 min wall-clock locally, gated in CI on PR paths under `slayer/pg_facade/`, `slayer/facade/`, `slayer/demo/`, the e2e files, or `pyproject.toml` / `poetry.lock`. Skips cleanly when Docker is unavailable. Extracts `_start_pg_demo_server` from `test_integration_pg_facade.py` into a shared `tests/integration/_pg_serve_helpers.py` with optional log-record and storage out-parameters for the new B.3 / B.4 / A.6 paths. Re-exports the session-scoped `metabase_e2e_env` fixture from `tests/integration/ conftest.py` so pytest auto-discovers it without function-parameter shadowing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a live Metabase end-to-end test suite for the Postgres facade. A shared ChangesLive Metabase E2E Test Suite and Supporting Infrastructure
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Docker as Docker daemon
participant PgServe as pg-serve instances
participant Metabase as Metabase container
participant Tests as pytest metabase_e2e suite
CI->>Docker: load/pull metabase/metabase:v0.62.1.5
CI->>Tests: pytest -m metabase_e2e tests/integration/test_metabase_e2e.py
Tests->>PgServe: start_pg_demo_server(token=None)
Tests->>PgServe: start_pg_demo_server(token=token)
Tests->>Docker: docker run Metabase → container_id
Tests->>Metabase: GET /api/health (poll until ready)
Tests->>Metabase: GET /api/session/properties / POST /api/setup
Tests->>Metabase: POST /api/database (×2 for tokenless and token-protected)
Tests->>Metabase: GET /api/database/{id}/metadata (poll until table count ready)
Tests->>Metabase: POST /api/dataset (catalog, query, filter, join, aggregation tests)
Tests->>PgServe: asyncpg/psycopg2 direct wire-format and transaction checks
Tests->>Metabase: teardown container, dump logs
Tests->>PgServe: stop loops and threads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/integration/_pg_serve_helpers.py (1)
47-49: ⚡ Quick winMove these helper imports to module scope.
These imports are not optional dependency probes, so keep them with the rest of the file imports. As per coding guidelines,
**/*.py: “Imports at the top of files.”🤖 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 `@tests/integration/_pg_serve_helpers.py` around lines 47 - 49, The imports for _prepare_demo, _resolve_storage, SlayerQueryEngine, and PgConnection are currently located at lines 47-49, which appears to be nested scope rather than at the module level. Move these three import statements to the top of the file with the other module-level imports, since these are not optional dependency probes and should follow the standard Python convention of placing all imports at the beginning of the file.Source: Coding guidelines
🤖 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 @.github/workflows/pg-facade-e2e.yml:
- Around line 37-55: The workflow uses mutable action tags for checkout,
setup-python, and cache which weakens supply-chain security. Replace the version
tags in actions/checkout@v4, actions/setup-python@v5, and actions/cache@v4 with
their full commit SHAs (you can find these on the GitHub action repositories).
Additionally, add persist-credentials: false as a parameter to the
actions/checkout step to prevent checkout credentials from being persisted in
the workflow environment, improving security by limiting credential exposure.
In `@tests/integration/_pg_serve_helpers.py`:
- Around line 93-95: The _setup() function binds the server to 127.0.0.1
(loopback), but Metabase running in Docker cannot reach this address through
host.docker.internal. Add a listen_host parameter to the start_pg_demo_server
function with a default value of 127.0.0.1 to maintain backward compatibility
with existing asyncpg tests, then use this parameter in the asyncio.start_server
call instead of the hardcoded 127.0.0.1. Finally, update the two
start_pg_demo_server() calls in the Metabase fixture to pass
listen_host="0.0.0.0" so the server listens on all interfaces and is reachable
from Docker containers.
In `@tests/integration/conftest_metabase.py`:
- Around line 176-188: When the docker inspect command fails (indicated by
non-zero returncode or non-digit output), the code raises a RuntimeError without
stopping the docker container that was successfully created earlier by docker
run. Before raising the RuntimeError in the error handling block where you check
inspect.returncode != 0 or not inspect.stdout.strip().isdigit(), first execute a
docker stop command on the container_id to clean up the running container, then
raise the error to prevent orphaned containers.
In `@tests/integration/test_metabase_e2e.py`:
- Around line 1068-1070: The test_concurrent_dataset_requests function shares a
single metabase_e2e_env.client instance across all worker threads spawned by
ThreadPoolExecutor, but the underlying requests.Session is not thread-safe,
causing potential transport contention that masks the actual server-side
concurrency behavior being tested. Modify the run_one function (or the work
submitted to pool.submit) to create a separate client/session instance for each
worker thread instead of reusing the shared metabase_e2e_env.client, so each
thread has its own isolated transport layer and the test properly validates
server-side concurrency handling.
- Around line 932-933: The test file has multiple redundant imports of psycopg2
inside test functions, violating the "imports at the top of files" guideline.
Remove the local import statements at lines 932-933, 962-963, and 994-995 in
tests/integration/test_metabase_e2e.py since psycopg2 is already handled at
module scope via pytest.importorskip("psycopg2"). Keep only the module-level
importorskip binding at the top of the file and rely on that single import
throughout all test functions for consistency and cleaner code.
- Around line 91-92: Replace the fixed time.sleep() calls that occur after
sync_schema() invocations with condition-based waiting using _wait_until(). At
the anchor location in tests/integration/test_metabase_e2e.py lines 91-92,
replace the time.sleep(2) call with _wait_until() that polls for the expected
post-sync condition (such as verifying that database_metadata() returns the
expected state after the schema sync completes). Similarly, at the sibling
location in tests/integration/test_metabase_e2e.py lines 121-122, replace the
time.sleep(3) call with an equivalent _wait_until() call that validates the
schema sync condition has been satisfied. This removes the timing sensitivity
and makes the tests more robust under variable CI load.
---
Nitpick comments:
In `@tests/integration/_pg_serve_helpers.py`:
- Around line 47-49: The imports for _prepare_demo, _resolve_storage,
SlayerQueryEngine, and PgConnection are currently located at lines 47-49, which
appears to be nested scope rather than at the module level. Move these three
import statements to the top of the file with the other module-level imports,
since these are not optional dependency probes and should follow the standard
Python convention of placing all imports at the beginning of the file.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2da1994f-7c12-45c4-8dc7-e91f2ea2b300
📒 Files selected for processing (9)
.github/workflows/pg-facade-e2e.ymlCLAUDE.mddocs/interfaces/pg-facade.mdpyproject.tomltests/integration/_pg_serve_helpers.pytests/integration/conftest.pytests/integration/conftest_metabase.pytests/integration/test_integration_pg_facade.pytests/integration/test_metabase_e2e.py
CI failures:
- Bind pg-serve to 0.0.0.0 so Metabase (in container) can reach it via
host.docker.internal:port. Was bound to 127.0.0.1 only; --add-host
resolved the hostname but the socket wasn't accepting on the host's
external interface. New optional bind_host kwarg on
start_pg_demo_server defaults to "127.0.0.1" (preserves existing
asyncpg-only suite behaviour); conftest_metabase.py passes "0.0.0.0"
for both pg-serve instances.
- Exclude metabase_e2e from the broad lint-and-test integration step
(`-m "integration and not metabase_e2e"`). The e2e tests carry both
markers per spec; only the dedicated metabase-e2e workflow runs them.
Code-review fixes:
- Per-worker MetabaseClient inside test_concurrent_dataset_requests
(CodeRabbit major). requests.Session isn't guaranteed thread-safe;
transport contention could mask server-side concurrency.
- Move start_pg_demo_server's deferred imports to module top
(CodeRabbit nitpick — user's global no-inline-imports rule).
- Dump Metabase container logs to /tmp from the fixture's finally
block before docker stop, so the CI workflow's `if: failure()` step
finds them after --rm has cleaned up the container (Codex minor).
Sonar suppressions:
- NOSONAR(S2068) / NOSONAR(S6437, S2068) on test fixture passwords
("x" against the no-token pg-serve loopback fallback).
- NOSONAR(S1542) on test_bad_password_returns_28P01 +
test_nonexistent_database_returns_3D000 — SQLSTATE codes are
conventionally uppercase; the test names are clearer this way.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the workflow token to read-only permissions.
This workflow does not need write-scoped
GITHUB_TOKENpermissions. Add an explicit top-level permissions block to avoid inheriting broader repository defaults.Proposed least-privilege setting
name: CI +permissions: + contents: read + on:🤖 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 @.github/workflows/ci.yml around lines 1 - 10, Add an explicit top-level permissions block to the CI workflow in .github/workflows/ci.yml to restrict the GITHUB_TOKEN to read-only access. Place this permissions block after the on section and before the jobs section, setting the minimum required read-only permissions to prevent the workflow from inheriting broader repository defaults.Source: Linters/SAST tools
🧹 Nitpick comments (2)
tests/integration/conftest_metabase.py (2)
105-133: ⚡ Quick winUse a Pydantic v2 model for the env container.
MetabaseE2EEnvis a typed shared-state model but is implemented as a manual mutable class. Convert it to a Pydantic v2BaseModelto match the repo model rule; allow arbitrary test fixtures likeMetabaseClient,logging.LogRecord, and storage.Proposed Pydantic v2 conversion
+from pydantic import BaseModel, ConfigDict + @@ -class MetabaseE2EEnv: +class MetabaseE2EEnv(BaseModel): """Shared state yielded by the ``metabase_e2e_env`` fixture.""" - def __init__( - self, - *, - base_url: str, - session_token: str, - client: MetabaseClient, - token_db_id: int, - pg_no_token: Tuple[str, int], - pg_token: Tuple[str, int, str], - log_records: List[logging.LogRecord], - pg_no_token_storage: Any, - ) -> None: - self.base_url = base_url - self.session_token = session_token - self.client = client - self.token_db_id = token_db_id - self.pg_no_token = pg_no_token - self.pg_token = pg_token - self.log_records = log_records - self.pg_no_token_storage = pg_no_token_storage + model_config = ConfigDict(arbitrary_types_allowed=True) + + base_url: str + session_token: str + client: MetabaseClient + token_db_id: int + pg_no_token: Tuple[str, int] + pg_token: Tuple[str, int, str] + log_records: List[logging.LogRecord] + pg_no_token_storage: AnyAs per coding guidelines, "Use Pydantic v2 for all models; never use dataclasses — if you need dataclasses, use Pydantic classes instead."
🤖 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 `@tests/integration/conftest_metabase.py` around lines 105 - 133, Convert the MetabaseE2EEnv class from a manual mutable class to a Pydantic v2 BaseModel to align with repository coding guidelines. Replace the class definition to inherit from BaseModel, remove the manual __init__ method (Pydantic will generate it automatically from field annotations), keep all existing fields with their current type annotations, and configure the model with model_config to allow arbitrary types since fields like client (MetabaseClient), log_records (List[logging.LogRecord]), and pg_no_token_storage contain non-Pydantic types. The make_client method can remain unchanged as it will work the same way with Pydantic fields.Source: Coding guidelines
48-50: ⚡ Quick winMove
requestsimports to module scope.
requestsis imported inside several helpers despite the repo rule to keep imports at the top of Python files. Move it beside the other imports and remove the local imports; keep the fixture skip only ifrequestsis truly optional for this test tier.Proposed cleanup
import pytest +import requests from tests.integration._pg_serve_helpers import start_pg_demo_server @@ def __init__(self, *, base_url: str, session_token: str, db_id: int) -> None: - import requests - self.base_url = base_url.rstrip("/") @@ def _wait_for_health(base_url: str, timeout_s: int) -> bool: - import requests - deadline = time.monotonic() + timeout_s @@ def _fetch_setup_token(base_url: str) -> Optional[str]: - import requests - r = requests.get(f"{base_url}/api/session/properties", timeout=10) @@ def _run_setup(base_url: str, setup_token: str) -> str: """Walk Metabase's first-boot setup; return the admin session id.""" - import requests - body = { @@ def _login(base_url: str) -> str: """Fallback login when the instance is already set up.""" - import requests - r = requests.post( @@ ) -> int: - import requests - body = { @@ ) -> Dict[str, Any]: - import requests - deadline = time.monotonic() + timeout_s @@ def metabase_e2e_env() -> Iterator[MetabaseE2EEnv]: """Session-scoped Metabase + dual pg-serve bootstrap.""" - pytest.importorskip("requests") pytest.importorskip("asyncpg")As per coding guidelines, "Place imports at the top of files."
Also applies to: 224-251, 277-304, 339-342, 370-374
🤖 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 `@tests/integration/conftest_metabase.py` around lines 48 - 50, The requests module is being imported inside methods rather than at the module scope, violating the coding guideline to place all imports at the top of Python files. Move the import requests statement to the top of the file alongside other imports, and remove the local import statement from inside the __init__ method and other affected locations mentioned (lines 224-251, 277-304, 339-342, 370-374). Keep any fixture skip logic only if requests is truly optional for this test tier.Source: Coding guidelines
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-10: Add an explicit top-level permissions block to the CI
workflow in .github/workflows/ci.yml to restrict the GITHUB_TOKEN to read-only
access. Place this permissions block after the on section and before the jobs
section, setting the minimum required read-only permissions to prevent the
workflow from inheriting broader repository defaults.
---
Nitpick comments:
In `@tests/integration/conftest_metabase.py`:
- Around line 105-133: Convert the MetabaseE2EEnv class from a manual mutable
class to a Pydantic v2 BaseModel to align with repository coding guidelines.
Replace the class definition to inherit from BaseModel, remove the manual
__init__ method (Pydantic will generate it automatically from field
annotations), keep all existing fields with their current type annotations, and
configure the model with model_config to allow arbitrary types since fields like
client (MetabaseClient), log_records (List[logging.LogRecord]), and
pg_no_token_storage contain non-Pydantic types. The make_client method can
remain unchanged as it will work the same way with Pydantic fields.
- Around line 48-50: The requests module is being imported inside methods rather
than at the module scope, violating the coding guideline to place all imports at
the top of Python files. Move the import requests statement to the top of the
file alongside other imports, and remove the local import statement from inside
the __init__ method and other affected locations mentioned (lines 224-251,
277-304, 339-342, 370-374). Keep any fixture skip logic only if requests is
truly optional for this test tier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d178a8b5-79a8-4e62-825c-e37e95a500f3
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/pg-facade-e2e.ymltests/integration/_pg_serve_helpers.pytests/integration/conftest_metabase.pytests/integration/test_metabase_e2e.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pg-facade-e2e.yml
- tests/integration/test_metabase_e2e.py
…or gaps
Round-3 CI ran 43/62 e2e tests green after the bind-host fix; the 19 reds
were real pg-facade gaps the suite was built to detect. Triaged into
test-side dodges, one in-scope prod fix, and two deferred follow-ups.
Test-side fixes (4 tests now exercise the real code path):
- B.8 (objsubid='' against INT): rewrite to use the parameterised $1 form,
which exercises connection.py:_substitute_typed_sentinels — the actual
code path the DEV-1558 fix lives on. The raw literal '' bypassed it.
- K.4 (BOOLEAN/DOUBLE/INT wire round-trip): wrap SELECT TRUE with FROM
orders LIMIT 1; pg-facade requires a registered FROM.
- M.2 (asyncpg concurrent state isolation): swap arithmetic-with-COUNT
marker for a per-connection SET application_name marker.
- E.7 (canonical-alias ORDER BY pin): use ORDER BY bucket alias instead of
ORDER BY 1 ordinal; replicate the GROUP BY expression literally.
Prod fix:
- G6 (Sunday-week wrapper): extend slayer/facade/translator.py
_detect_time_grain to recognise Metabase's
CAST((CAST(DATE_TRUNC('week', col + INTERVAL '1 day') AS DATE)
+ INTERVAL '-1 day') AS DATE) wrapper. Day-offset unwrap is scoped to
WEEK only so non-Metabase DATE_TRUNC('month', col + INTERVAL '1 day')
queries keep their original semantics. Unit tests pin both branches.
Deferred with strict xfail + Linear follow-up:
- DEV-1565 (joined-column projection, H.1/H.2/H.3): pg-facade needs to
recognise Metabase's LEFT JOIN-with-subquery shape and map joined
aliases to cross-model column refs. Single-hop today; multi-hop later.
- DEV-1566 (CAST in projection, K.3): SLayer-query model has no per-query
type-coercion primitive — needs either pattern-match passthrough or a
raw-native-SQL path. Architectural.
CodeRabbit / Sonar / Codex polish:
- Move inline `import requests` calls in conftest_metabase.py to module
top (CodeRabbit + user's global no-inline-imports rule).
- Extract _bootstrap_metabase_session helper from metabase_e2e_env to
drop the fixture's Cognitive Complexity below Sonar's S3776 threshold.
- MetabaseClient now attaches the response body to HTTPError on 4xx —
diagnostic improvement so next CI surfaces the remaining ~10 HTTP-400s
with Metabase's actual error message instead of an opaque "Bad Request".
- Tighten B.5 (test_four_part_qualified_refs_handled) to assert metadata
remains queryable post-sync, not just that sync returned 200.
- Tighten H.2 (test_join_filter_on_joined_column) assertion so a future
G7-fixed run meaningfully checks the filter was applied (per Codex).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-4 CI ran with improved diagnostics (response body capture from round 2's MetabaseClient change). The 19 remaining failures cleanly split into test-rewriteable (5) and real pg-facade gaps (9 across 4 distinct subsystems). Test rewrites — exercise the same contract via a supported shape: - F.3 / F.4 (numeric / date range filter): use compound `["and", [">", ...], ["<=", ...]]` instead of MBQL `between`; pg-facade translates `between` to SQL BETWEEN which isn't parseable into SLayer filter DSL today. - K.4 (BOOLEAN wire-format): drop the bare-boolean-literal branch entirely. pg-facade rejects `SELECT TRUE` in projection, the demo schema has no BOOLEAN column, and BOOLEAN wire encoding is already covered by `tests/pg_facade/test_protocol.py`. The DOUBLE / INT round-trips this test pins remain. - B.3 (hidden columns) / B.4 (descriptions): drop the Metabase round-trip and query pg-facade's catalog directly via asyncpg. The Metabase c3p0 pool caches the pg-facade catalog per pooled JDBC connection, so going through `client.sync_schema()` + `client.table_metadata()` never sees in-process storage mutations without re-registering the database (cascade-invalidates other tests). The contract these tests pin — pg-facade omits hidden columns from `INFORMATION_SCHEMA.COLUMNS` and surfaces `Column.description` via `pg_description` — is tested directly. Strict-xfail with Linear follow-ups (each test promotes to PASS when the referenced gap is fixed): - C.1, C.2, C.3, C.4 → DEV-1567: pg-facade leaks dotted cross-model fingerprint measures into SlayerQuery; Pydantic name validator rejects them. - F.7 (HAVING on aggregate), G.2 (ORDER BY aggregate) → DEV-1568: pg-facade doesn't resolve MBQL `["aggregation", N]` ordinal refs. - M.2 (concurrent connections) → DEV-1569: pg-facade doesn't preserve per-connection SET state. - B.8 (objsubid='') → DEV-1570: typed-sentinel substitution covers Describe but not Bind for empty-string vs INT. End state: 47 green / 13 documented strict-xfails / CI green. Suite is a live regression detector — each follow-up PR will XPASS its xfailed test and flip CI red, prompting maintenance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t unwrap + conditional metabase fixture import
Codex review of round-3 surfaced two valid findings against the merged
state:
Major — Sunday-week day-offset unwrap was too permissive
=========================================================
Round-3's _unwrap_one_day_offset accepted any +/- INTERVAL '1 day' shape
symmetrically. A user-written `date_trunc('week', col - INTERVAL '1 day')`
(Friday-week bucketing) would silently collapse to plain WEEK(col), and
the outer wrapper `date_trunc('week', col + INTERVAL '1 day') + INTERVAL
'1 day'` (inverse of the Metabase Sunday-week shape) would too. Both
would change bucket semantics on valid non-Metabase user SQL.
Replaced with a direction-aware unwrap:
- _day_interval_sign() returns +1 / -1 / None.
- _unwrap_signed_day_offset(node, expected_sign=...) only unwraps when
the net day shift matches the expected sign.
- _detect_time_grain calls expected_sign=-1 on the outer (Metabase's -1
day Sunday-week shift) — a +1 outer offset is left alone.
- _detect_time_grain_date_trunc calls expected_sign=+1 on the inner
column-side offset, AND only when grain is WEEK.
Added 2 new direction-asymmetry tests under
tests/facade/test_translator.py covering both the inner -1 day form and
the outer +1 day form. All 8 week-grain tests pass (537 facade tests
total).
Minor — conditional Metabase fixture import
============================================
tests/integration/conftest.py unconditionally imported the Metabase
fixture module, which top-imports `requests`. A narrow dev install
without requests would fail collection on every integration test. Wrap
the import in try/except ImportError; the metabase_e2e suite carries
its own pytest.importorskip("requests") guard and skips cleanly when
the dep is absent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o outer -1d wrapper
Codex review of round-4 caught a real correctness gap:
Major — partial Sunday-week wrappers were silently collapsed
=============================================================
Round-4's _detect_time_grain_date_trunc unwrapped the inner ``col +
INTERVAL '1 day'`` regardless of whether the outer ``-1 day`` wrapper
was present. That meant a user-written ``DATE_TRUNC('week', col +
INTERVAL '1 day')`` (a deliberately-shifted bucket) was silently
translated as plain ``WEEK(col)``, changing bucketing semantics on
boundary dates.
Refactored: extracted a dedicated _detect_sunday_week_wrapper that
matches the full pattern — outer -1 day AND inner +1 day MUST be
present together. _detect_time_grain calls it as a single-step
match BEFORE the generic DATE_TRUNC path. The generic detector no
longer touches column-side day offsets at all, so a partial wrapper
falls through to the existing "Unsupported projection expression"
error like any other untranslatable shape.
Replaced the round-4 inner-direction test (which asserted the now-
incorrect partial-unwrap behaviour) with a stricter test that pins
both partial shapes (+1d alone and -1d alone) as REJECT. The full-
wrapper test continues to pass.
Minor — LIKE coverage tightened
================================
test_filter_like_ilike_contains looped over (starts-with, ends-with,
contains) and only asserted the integer count, so a regression that
ignored the predicate or translated it to the wrong operator would
silently pass. Rewrote to use direction-discriminating samples
(prefix / suffix / middle slice) and assert at least one match for
each — a flipped-to-zero regression now fails.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex (minor): test_filter_like_ilike_contains
================================================
Round-5's "operators don't collapse" assertion was
`len(set(counts)) > 1 or counts[a] == counts[b] == counts[c]` — a
tautology that's True whether counts differ OR are all identical, so
it caught nothing. Removed; the per-operator `n >= 1` checks remain
the load-bearing assertions.
CodeRabbit Thread 2 (major): container leak on inspect failure
===============================================================
If `docker run` succeeded but `docker inspect` failed (timeout, race
with container init, etc.), `_run_metabase_container` raised before
the container_id reached the fixture's `finally` block — leaking the
container until manual cleanup. Wrapped the inspect in try/except
that calls `_stop_container(container_id)` then re-raises.
CodeRabbit Thread 3 (major): fixed sleeps replaced with poll
=============================================================
`time.sleep(2)` in test_sync_schema_idempotent and `time.sleep(3)`
in test_sync_log_volume_within_budget were timing-sensitive under
variable CI load. Re-introduced `_wait_until` helper (poll a
predicate with a 0.5s interval up to a timeout) and replaced the
fixed sleeps with predicate-based waits.
CodeRabbit Thread 4 (minor): drop inline psycopg2 imports
==========================================================
psycopg2 is already bound at module top via
`pytest.importorskip("psycopg2")`. Three test bodies re-imported it
locally — redundant and violates the global no-inline-imports rule.
Removed; comment marker left in place to make the intent obvious
to a reader.
CodeRabbit Nitpick 2: MetabaseE2EEnv → Pydantic BaseModel
==========================================================
The env class held its fields via manual __init__ assignments, but
per the repo rule "never use dataclass, inherit from BaseModel"
(CLAUDE.md global rules), it should be a Pydantic model. Converted
with `model_config = ConfigDict(arbitrary_types_allowed=True)` since
the fields carry runtime resources (MetabaseClient with live
requests.Session, log_records list, storage backend handle).
CodeRabbit outside-diff (major): workflow permissions
======================================================
Both CI workflows (ci.yml, pg-facade-e2e.yml) inherited the repo's
default GITHUB_TOKEN permissions which include write scope. Added
top-level `permissions: contents: read` to both — least-privilege
matching the workflows' actual needs (checkout + run tests).
INVALID — replied:
- CR Thread 1 (action SHA pinning + persist-credentials): repo-wide
policy change; existing ci.yml uses same @v4/@v5 tag pattern;
changing only the new file would create inconsistency.
- CR Nitpick 1 (_pg_serve_helpers imports at top): already addressed
in round 3 (commit be4265b); CR analyzed pre-fix snapshot. Bot
will see the imports at module top on next pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex (minor) — test_sync_log_volume_within_budget under-counted ================================================================= Round-6 replaced the fixed 3-second sleep with `_wait_until` polling for the first new log record. That returns as soon as the FIRST record appears — but Metabase keeps emitting catalog-probe events asynchronously for several seconds after sync_schema returns, so the WARN tally was sampled mid-burst and could miss the very regression this test pins (DEV-1558 bug 4: 170+ WARN lines per sync). Added _wait_until_stable helper: polls a getter, returns once the value stops changing for a settle period (default 2s). Used in the WARN test to wait until log emission quiesces before tallying. INVALID (replied locally — codex has no remote thread): - Codex finding 1 (path filter omits engine/sql/core/storage): the user explicitly chose "keep strictly narrow" for the path filter in round 1's spec interview. Round 2 expanded to pyproject.toml / poetry.lock / conftest / demo per a sibling Codex finding the user accepted. Engine/sql/core/storage were explicitly excluded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/conftest_metabase.py (1)
503-524: 💤 Low valueRe-exporting
jsonin__all__is unconventional.Exporting a standard library module from this fixture module may confuse readers expecting only fixture-related symbols. Tests can simply
import jsondirectly.Suggested change
-# Re-export the JSON dump shim used by tests so they don't need to import json themselves. __all__ = [ "MetabaseClient", "MetabaseE2EEnv", "encode_mbql_query", "encode_native_query", "metabase_e2e_env", - "json", ]🤖 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 `@tests/integration/conftest_metabase.py` around lines 503 - 524, The __all__ list in conftest_metabase.py includes "json", which is a standard library module being re-exported. This is unconventional since __all__ should only contain fixture-related symbols defined in this module. Remove "json" from the __all__ list while keeping the other exports (MetabaseClient, MetabaseE2EEnv, encode_mbql_query, encode_native_query, and metabase_e2e_env) as they are the actual fixture-related symbols that tests should use from this module.
🤖 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.
Nitpick comments:
In `@tests/integration/conftest_metabase.py`:
- Around line 503-524: The __all__ list in conftest_metabase.py includes "json",
which is a standard library module being re-exported. This is unconventional
since __all__ should only contain fixture-related symbols defined in this
module. Remove "json" from the __all__ list while keeping the other exports
(MetabaseClient, MetabaseE2EEnv, encode_mbql_query, encode_native_query, and
metabase_e2e_env) as they are the actual fixture-related symbols that tests
should use from this module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28369484-3893-45e0-8d22-30a8c136274f
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/pg-facade-e2e.ymlslayer/facade/translator.pytests/facade/test_translator.pytests/integration/conftest.pytests/integration/conftest_metabase.pytests/integration/test_metabase_e2e.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pg-facade-e2e.yml
… tightening Round-7 regression — self-validating guard tripped on idempotent sync ===================================================================== CI on 6d77ec9 failed test_sync_log_volume_within_budget with `assert 124 > 124`. Metabase no-ops a sync_schema call when nothing has changed at the protocol level, so when this test runs after test_sync_schema_idempotent (which has already triggered a sync), no new records arrive and `total_after > total_before` collapses. The "is capture wired?" sanity check was correct in intent but miscoupled to the per-sync delta. Weakened to `total_after > 0` — the session-scoped buffer has always accumulated records from the suite's bootstrap + sync sequence before this test runs, and zero records there really is the only signal that the handler isn't attached. Per-sync delta would only work if we forced the sync to do real work (e.g. via /api/database/{id}/discard_values), which adds fixture complexity without diagnostic value. Codex (minor) — _wait_until_stable swallowed timeouts ====================================================== Codex flagged that `_wait_until_stable` returned the last observed value even on timeout, so a caller that polls past the stabilisation window silently samples mid-burst. Changed the signature to `Tuple[bool, Any]` (stabilised, last_value) and updated the WARN test to assert stabilised explicitly — partial samples now raise instead of false-passing the regression they're meant to catch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ves on 0.0.0.0
Codex (major) — Metabase fixture exposed unauthenticated pg-serve on 0.0.0.0
=============================================================================
Round-2 bound the "no-token" pg-serve to 0.0.0.0 so a Metabase container
could reach it via host.docker.internal. The bind worked, but the
fixture bypassed pg-facade's CLI-level validate_bind_address guard
(`non-loopback binds require a token` per slayer/pg_facade/auth.py:44).
Net effect: while the e2e suite ran, an unauthenticated pg-serve was
accepting queries from any client on every interface — a real local-
network exposure on dev machines (CI runners are ephemeral / isolated,
so lower-risk there but still wrong).
Three-part fix:
1. `_pg_serve_helpers.start_pg_demo_server` now calls
`validate_bind_address(host=bind_host, token=token)` at the top. The
helper can no longer accidentally expose an unauthenticated server
on a network interface — the call raises the same ValueError the
CLI does, so the test fails loudly instead of silently exposing.
2. Both pg-serves in conftest_metabase get per-session random tokens
via `secrets.token_urlsafe(32)` (one for the primary server, one
for the auth-test server). Metabase's POST /api/database carries
the right token as the database password.
3. Test-side rename + threading:
- env.pg_no_token → env.pg_primary (now a token-protected server
that just isn't the auth-test one)
- env.pg_no_token_storage → env.pg_primary_storage
- env.pg_token → env.pg_auth
- New field env.pg_primary_password carries the random token
- _asyncpg_connect dropped its `password="x"` default; the password
is now required
- _scalar grew a password parameter
- All call sites updated to thread env.pg_primary_password (sed-
migrated for the 14 connection sites; manual edits for 2 _scalar
sites and the L.2 wrong-password test that intentionally passes
a bad credential)
537 facade unit tests + 24 pre-existing pg-facade integration tests
still pass. Lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ord and "no-token" docs Codex (major) — psycopg2 connects still passed password="x" ============================================================ Round-9 swapped the asyncpg helpers to require the per-session token but the sed only matched `_asyncpg_connect(host, port)`; the three direct `psycopg2.connect(...)` calls in K.2, K.3, K.4 still passed the legacy "x" placeholder password and would now fail auth against the token-protected primary pg-serve. Fixed all three to use `metabase_e2e_env.pg_primary_password`. Codex (minor) — stale "no-token" descriptions ============================================== The fixture docstring, CLAUDE.md's Testing section, and docs/interfaces/pg-facade.md's "Testing your changes" block all described the primary pg-serve as "no-token, loopback fallback". That contradicted the round-9 security fix (both pg-serves now token-protected on 0.0.0.0) and could mislead a future change back toward the previously-flagged unauthenticated bind. Updated all three to describe the current dual-token setup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r (DEV-1572) Codex (major) — Sunday-week wrapper silently changed bucket boundaries ====================================================================== Round-4/5 added detection of Metabase's Sunday-week wrapper and mapped it to SLayer's existing `TimeGranularity.WEEK`. Codex round-11 caught that `WEEK` is Monday-based (`slayer/core/enums.py:80` — `date - timedelta(days=date.weekday())`), so the collapse silently shifted bucket boundaries by one day: Monday Jan 8 landed in the Jan 8-14 SLayer bucket but the Jan 7-13 Metabase bucket. Row labels wrong, row→bucket assignment wrong. Reverted the wrapper detection — `_detect_sunday_week_wrapper` is now a stub returning None. The wrapper falls through to the existing "Unsupported projection expression" error so the failure is loud and the result is never wrong-bucketed. Tracked as DEV-1572 (https://linear.app/motley-ai/issue/DEV-1572): the real fix is a new `TimeGranularity.WEEK_SUNDAY` enum value with per-dialect SQL emission (PG/DuckDB: `DATE_TRUNC('week', col + 1d) - 1d`; BigQuery: `DATE_TRUNC(col, WEEK(SUNDAY))`; etc.) and the translator returning that granularity instead. Worktree pre-created at slayer.worktrees/egor__dev-1572-... Test updates: - test_translator.py::test_metabase_sunday_week_wrapper_recognised → test_metabase_sunday_week_wrapper_rejected_pending_dev_1572, asserts the wrapper now raises TranslationError. - test_metabase_e2e.py::test_time_grain_breakout: the [week] parameter case is now `xfail(strict=True)` referencing DEV-1572. Other grains (hour/day/month/quarter/year) continue to PASS. - Dropped the now-unused `_date_trunc_unit` helper (added in round 5 only for the wrapper detector). Docs: added a "Known limitations" section to docs/interfaces/pg-facade.md listing every strict-xfailed gap (DEV-1565..DEV-1572) so an operator knows up-front which Metabase shapes don't work via pg-facade today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…son` and __all__ re-export CodeRabbit (nitpick, low) — re-exporting `json` in `__all__` is unconventional ============================================================================= conftest_metabase.py imported `json` at module top and re-exported it via `__all__` as a convenience for tests. The convenience is unused (no test imports it from this module), the underlying `import json` itself is unused (all `.json()` calls in the file are `requests.Session` methods / kwargs), and re-exporting a stdlib module from a fixture module confuses readers expecting only fixture-related symbols. Dropped both. Codex round-11 review: No findings. Sonar round-11: gate OK, 0 issues, 0 hotspots. CodeRabbit threads on the SHA-pinning back-and-forth: already acknowledged; courtesy reply posted declining the follow-up issue (the docs/interfaces/pg-facade.md "Known limitations" section is sufficient trail). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…de-live-metabase-end-to-end-integration-test-suite # Conflicts: # .github/workflows/ci.yml
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 163-179: The checkout step in the workflow retains repository
credentials by default, which increases credential exposure. Since this job only
reads secrets for conditional execution in the gate step and doesn't push
changes back to the repository, add the persist-credentials: false option to the
actions/checkout@v4 step to disable credential persistence and minimize security
exposure.
In `@tests/facade/test_translator.py`:
- Around line 514-527: The test at file tests/facade/test_translator.py lines
514-527 uses pytest.raises(TranslationError) which will pass on any
TranslationError including parse errors or unrelated failures, but doesn't
verify the specific unsupported projection error required by the DEV-1572
contract. Update this test and the related ones at lines 536-544, 554-572, and
584-592 to match the expected unsupported projection error message instead of
just checking for any TranslationError. Use pytest.raises with a match parameter
to validate that the error message contains the specific unsupported projection
text, ensuring these tests genuinely verify the rejection path rather than
accidentally passing on unrelated errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11977fb5-0a6c-4b9b-b392-0e8797361fe7
📒 Files selected for processing (9)
.github/workflows/ci.ymlCLAUDE.mddocs/interfaces/pg-facade.mdpyproject.tomlslayer/facade/translator.pytests/facade/test_translator.pytests/integration/_pg_serve_helpers.pytests/integration/conftest_metabase.pytests/integration/test_metabase_e2e.py
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- docs/interfaces/pg-facade.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/_pg_serve_helpers.py
- tests/integration/test_metabase_e2e.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 163-179: The checkout step in the workflow retains repository
credentials by default, which increases credential exposure. Since this job only
reads secrets for conditional execution in the gate step and doesn't push
changes back to the repository, add the persist-credentials: false option to the
actions/checkout@v4 step to disable credential persistence and minimize security
exposure.
In `@tests/facade/test_translator.py`:
- Around line 514-527: The test at file tests/facade/test_translator.py lines
514-527 uses pytest.raises(TranslationError) which will pass on any
TranslationError including parse errors or unrelated failures, but doesn't
verify the specific unsupported projection error required by the DEV-1572
contract. Update this test and the related ones at lines 536-544, 554-572, and
584-592 to match the expected unsupported projection error message instead of
just checking for any TranslationError. Use pytest.raises with a match parameter
to validate that the error message contains the specific unsupported projection
text, ensuring these tests genuinely verify the rejection path rather than
accidentally passing on unrelated errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11977fb5-0a6c-4b9b-b392-0e8797361fe7
📒 Files selected for processing (9)
.github/workflows/ci.ymlCLAUDE.mddocs/interfaces/pg-facade.mdpyproject.tomlslayer/facade/translator.pytests/facade/test_translator.pytests/integration/_pg_serve_helpers.pytests/integration/conftest_metabase.pytests/integration/test_metabase_e2e.py
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- docs/interfaces/pg-facade.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/_pg_serve_helpers.py
- tests/integration/test_metabase_e2e.py
🛑 Comments failed to post (2)
.github/workflows/ci.yml (1)
163-179:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Unpinned action refs (version tags):" rg -n --glob ".github/workflows/*.yml" 'uses:\s*[^@]+@v[0-9]+' echo echo "checkout steps missing explicit persist-credentials:false (manual review with context):" rg -n -C3 --glob ".github/workflows/*.yml 'uses:\s*actions/checkout@'Repository: MotleyAI/slayer
Length of output: 263
🏁 Script executed:
#!/bin/bash # First, verify the file exists echo "=== Checking if .github/workflows/ci.yml exists ===" if git ls-files .github/workflows/ci.yml 2>/dev/null | grep -q .; then echo "File found." else echo "File not found in repository." exit 1 fi # Check total lines in the file echo "=== File size ===" wc -l .github/workflows/ci.yml # Read lines 163-179 echo "=== Lines 163-179 ===" sed -n '163,179p' .github/workflows/ci.ymlRepository: MotleyAI/slayer
Length of output: 774
🏁 Script executed:
#!/bin/bash # Check all action references in all workflows echo "=== All GitHub Actions references in workflows ===" grep -rn "uses:" .github/workflows/ | grep -o 'uses: [^[:space:]]*' | sort | uniq -c | sort -rn echo echo "=== Checking for commit SHA pinning patterns (40-char hex) ===" grep -rn "uses:" .github/workflows/ | grep -E '@[0-9a-f]{40}' && echo "Found commit SHA pinning" || echo "No commit SHA pinning found" echo echo "=== All unique action+version combinations ===" grep -rn "uses:" .github/workflows/ | grep -oE 'uses: [^[:space:]]+' | sort | uniqRepository: MotleyAI/slayer
Length of output: 594
🏁 Script executed:
#!/bin/bash # Search for policy documentation echo "=== Checking for policy docs ===" ls -la . | grep -iE 'readme|contributing|security|policy' echo echo "=== Searching for 'action' or 'pin' or 'commit' in docs ===" find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.yml" \) ! -path "./node_modules/*" ! -path "./.git/*" ! -path "./dist/*" ! -path "./build/*" 2>/dev/null | head -20 | xargs grep -l -i "action\|pin\|commit sha" 2>/dev/null | head -10 echo echo "=== Checking for lint/security config files ===" ls -la | grep -iE 'eslint|prettier|ruff|pylint|\.pre-commit|trivy|dependabot'Repository: MotleyAI/slayer
Length of output: 756
🏁 Script executed:
#!/bin/bash echo "=== CONTRIBUTING.md ===" cat CONTRIBUTING.md echo echo "=== .pre-commit-config.yaml ===" cat .pre-commit-config.yamlRepository: MotleyAI/slayer
Length of output: 5961
Add
persist-credentials: falseto the checkout step to reduce credential exposure.The checkout step at line 163 retains repository credentials in the job context by default. Since this job only reads secrets for conditional execution and doesn't push back to the repository, disable credential persistence to minimize exposure.
Suggested patch
- uses: actions/checkout@v4 + with: + persist-credentials: false📝 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.- uses: actions/checkout@v4 with: persist-credentials: false - name: Skip if Snowflake secret is unset id: gate env: SNOWFLAKE_CONNECTIONS_TOML: ${{ secrets.SNOWFLAKE_CONNECTIONS_TOML }} run: | if [[ -z "$SNOWFLAKE_CONNECTIONS_TOML" ]]; then echo "Snowflake secret unset — skipping the integration suite." echo "skip=true" >> "$GITHUB_OUTPUT" else echo "skip=false" >> "$GITHUB_OUTPUT" fi - name: Set up Python if: steps.gate.outputs.skip != 'true' uses: actions/setup-python@v5🧰 Tools
🪛 zizmor (1.25.2)
[warning] 163-163: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 163-163: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 179-179: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 163 - 179, The checkout step in the workflow retains repository credentials by default, which increases credential exposure. Since this job only reads secrets for conditional execution in the gate step and doesn't push changes back to the repository, add the persist-credentials: false option to the actions/checkout@v4 step to disable credential persistence and minimize security exposure.Source: Linters/SAST tools
tests/facade/test_translator.py (1)
514-527:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin these rejection tests to the unsupported-projection path.
pytest.raises(TranslationError)alone would also pass on parse errors or unrelated translation failures, so these tests could go green without verifying the DEV-1572 “fail loudly instead of mis-bucketing” contract. Match the expected unsupported projection message in each new negative case.Proposed test tightening
- with pytest.raises(TranslationError): + with pytest.raises(TranslationError, match="Unsupported projection expression"): translate( @@ - with pytest.raises(TranslationError): + with pytest.raises(TranslationError, match="Unsupported projection expression"): translate( @@ - with pytest.raises(TranslationError): + with pytest.raises(TranslationError, match="Unsupported projection expression"): translate( @@ - with pytest.raises(TranslationError): + with pytest.raises(TranslationError, match="Unsupported projection expression"): translate( @@ - with pytest.raises(TranslationError): + with pytest.raises(TranslationError, match="Unsupported projection expression"): translate(Also applies to: 536-544, 554-572, 584-592
🤖 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 `@tests/facade/test_translator.py` around lines 514 - 527, The test at file tests/facade/test_translator.py lines 514-527 uses pytest.raises(TranslationError) which will pass on any TranslationError including parse errors or unrelated failures, but doesn't verify the specific unsupported projection error required by the DEV-1572 contract. Update this test and the related ones at lines 536-544, 554-572, and 584-592 to match the expected unsupported projection error message instead of just checking for any TranslationError. Use pytest.raises with a match parameter to validate that the error message contains the specific unsupported projection text, ensuring these tests genuinely verify the rejection path rather than accidentally passing on unrelated errors.
5 tasks
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.
Summary
metabase_e2emarker) that drives a real Metabase v0.62.1.5 Docker container against twoslayer pg-serveinstances and asserts every regression vector from the DEV-1558 3-hour live debugging session.github/workflows/pg-facade-e2e.yml) fires only on PRs touchingslayer/pg_facade/,slayer/facade/,slayer/demo/, the e2e files, orpyproject.toml/poetry.lock; ~5 min wall-clock on cache hitCoverage of DEV-1558 bugs
test_pg_namespace_table_schem_column_name(B.6)''against INTobjsubidtest_pg_description_objsubid_empty_string_predicate(B.8)test_union_all_catalog_query_routed(B.9)test_sync_log_volume_within_budget(A.6, self-validating)first/laston time-less modelstest_first_last_not_exposed_on_timeless_models(B.7)CAST(DATE_TRUNC(...) AS DATE)time-graintest_native_sql_cast_date_trunc_as_date(E.7) + month grain in E.1-6test_order_by_time_grain_expression(G.3) +test_native_sql_order_by_canonical_alias(G.4)test_date_column_clean_round_trip_via_metabase(C.4) +test_date_psycopg_text_round_trip(K.2)Fixture topology
Session-scoped
metabase_e2e_env(intests/integration/conftest_metabase.py):docker infoprobe —pytest.skip()cleanly when Docker is unavailablestart_pg_demo_server: no-token (for Metabase viahost.docker.internalloopback fallback) and token-protected (for L.2/L.3 auth-error paths + A.5 Metabase-with-token)docker run -d --rm --add-host host.docker.internal:host-gateway -p 127.0.0.1::3000 metabase/metabase:v0.62.1.5; host port parsed viadocker inspect/api/health≤180sGET /api/session/properties→ setup-token;POST /api/setup(fallback toPOST /api/sessionif already set up)MetabaseE2EEnv(synchronousMetabaseClientwrapper + both pg-serve handles + log-record buffer + storage handle for B.3 / B.4 mutation tests)The shared
start_pg_demo_serverhelper (lifted fromtest_integration_pg_facade.py:27-87) gained two non-breaking out-parameter kwargs:log_records(sink for slayer.pg_facade + slayer.facade log records — A.6) andstorage_sink(sink for the engine's storage handle — B.3 / B.4 mutation paths).Test plan
metabase-e2ejob passes on this PR (Docker-backed)poetry run pytest -m metabase_e2e tests/integration/test_metabase_e2e.py -vruns locally and exits green within ~4 minpoetry run pytest tests/integration/test_integration_pg_facade.py -m integrationstill passes (the helper extraction touches the existing fixture import path)poetry run pytest -m "not integration"still passes (one pre-existing failure on the branch tip unrelated to this work:test_cli_argparse_subprocess_accepts_description_flaghangs on litellm reaching a SOCKS proxy; reproducible without this PR's changes)🤖 Generated with Claude Code
Summary by CodeRabbit