test(cli): #160 RED tests for shared BQ error renderer
3 new test files that drive the upcoming cli/error_render.py module
and the V2ClientError refactor.
tests/test_cli_error_render.py — 5 cases for `render_error(status, body)`:
recognize cross_project_forbidden BqAccessError shape; recognize
remote_scan_too_large guardrail rejection; recognize
bq_path_not_registered RBAC denial; fall back to truncated form for
unrecognized shape; pass through string `detail`.
tests/test_cli_query_render.py — V2ClientError must use the new renderer:
multi-line output instead of `f"HTTP {code}: {body}"`; no
pre-truncation that would hide the hint field; RemoteQueryError
already carries `details` (smoke).
tests/test_remote_query_error_details.py — audit lock-in for
RemoteQueryError raise sites that already populate details
(blocked_keyword) plus the shape contract for local-validation paths.
Run: 5 errors (cli.error_render module missing — clean ImportError),
2 assertion failures (V2ClientError single-line output, blocked_keyword
detail shape pre-existing). 3 regression-green pass for trivial
reasons; will exercise real code paths once GREEN lands.
This commit is contained in:
parent
896c43c7a2
commit
eddb0d2c58
3 changed files with 225 additions and 0 deletions
102
tests/test_cli_error_render.py
Normal file
102
tests/test_cli_error_render.py
Normal file
|
|
@ -0,0 +1,102 @@
|
|||
"""CLI structured error renderer for typed BigQuery error responses.
|
||||
|
||||
The server side maps BQ Forbidden/auth/badrequest errors into typed
|
||||
BqAccessError dicts that the FastAPI handler returns as `detail` JSON.
|
||||
Today the CLI side flattens them via `f"HTTP {code}: {body[:200]}"`,
|
||||
truncating the structured shape and hiding the operator-facing hints.
|
||||
|
||||
`cli.error_render.render_error` recognizes a few canonical shapes and
|
||||
pretty-prints them; falls back to truncated form for anything else.
|
||||
|
||||
Closes part of #160 §4.7.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def render_error():
|
||||
from cli.error_render import render_error
|
||||
return render_error
|
||||
|
||||
|
||||
def test_renders_typed_bq_access_error(render_error):
|
||||
"""`{detail: {kind, hint, billing_project, data_project}}` from
|
||||
BqAccessError surfaces as a multi-line block with the kind line, the
|
||||
key/value pairs, and the hint word-wrapped at 80 cols."""
|
||||
body = {"detail": {
|
||||
"kind": "cross_project_forbidden",
|
||||
"message": "USER_PROJECT_DENIED on bigquery.googleapis.com",
|
||||
"billing_project": "",
|
||||
"data_project": "prj-example-data-001",
|
||||
"hint": (
|
||||
"Set data_source.bigquery.billing_project in /admin/server-config "
|
||||
"to a project where the SA has serviceusage.services.use, or "
|
||||
"grant the SA that role on the data project."
|
||||
),
|
||||
}}
|
||||
out = render_error(502, body)
|
||||
# Single-line `f"HTTP {code}: ..."` style is the OLD form. The new
|
||||
# renderer must produce multi-line output.
|
||||
assert "\n" in out
|
||||
# Kind appears prominently in the output.
|
||||
assert "cross_project_forbidden" in out
|
||||
# Key/value pairs visible.
|
||||
assert "billing_project" in out
|
||||
assert "prj-example-data-001" in out
|
||||
# Hint text included.
|
||||
assert "serviceusage.services.use" in out
|
||||
|
||||
|
||||
def test_renders_remote_scan_too_large(render_error):
|
||||
"""`{detail: {reason: 'remote_scan_too_large', scan_bytes, limit_bytes,
|
||||
tables, suggestion}}` from the new /api/query guardrail formats with
|
||||
the bytes + tables + suggestion clearly visible."""
|
||||
body = {"detail": {
|
||||
"reason": "remote_scan_too_large",
|
||||
"scan_bytes": 10737418240, # 10 GiB
|
||||
"limit_bytes": 5368709120, # 5 GiB
|
||||
"tables": ["finance.unit_economics"],
|
||||
"suggestion": (
|
||||
"Use `da fetch <id> --select <cols> --where <predicate> "
|
||||
"--estimate` to materialize a filtered subset, then query "
|
||||
"the snapshot locally."
|
||||
),
|
||||
}}
|
||||
out = render_error(400, body)
|
||||
assert "remote_scan_too_large" in out
|
||||
assert "10737418240" in out or "10 GiB" in out or "10737418240" in str(out)
|
||||
assert "finance.unit_economics" in out
|
||||
assert "da fetch" in out
|
||||
|
||||
|
||||
def test_renders_bq_path_not_registered(render_error):
|
||||
"""`{detail: {reason: 'bq_path_not_registered', path, hint}}` from the
|
||||
RBAC patch formats path + hint clearly."""
|
||||
body = {"detail": {
|
||||
"reason": "bq_path_not_registered",
|
||||
"path": 'bq."secret_ds"."secret_tbl"',
|
||||
"hint": "Direct bq.* references must point to a registered table.",
|
||||
}}
|
||||
out = render_error(403, body)
|
||||
assert "bq_path_not_registered" in out
|
||||
assert 'secret_ds' in out
|
||||
assert "registered table" in out
|
||||
|
||||
|
||||
def test_falls_back_to_truncated_for_unrecognized_shape(render_error):
|
||||
"""Body without recognizable typed shape falls back to truncated form."""
|
||||
body = "Internal Server Error: Something went wrong" * 20 # 800+ chars
|
||||
out = render_error(500, body)
|
||||
# Old-style truncation kicks in; output is single-line and short.
|
||||
assert len(out) < 600
|
||||
assert "500" in out
|
||||
|
||||
|
||||
def test_falls_back_when_detail_is_string(render_error):
|
||||
"""Many old endpoints return `detail: "<string message>"` — render that
|
||||
as-is without trying to walk it as a structured error dict."""
|
||||
body = {"detail": "Only single SELECT queries are allowed"}
|
||||
out = render_error(400, body)
|
||||
assert "Only single SELECT" in out
|
||||
66
tests/test_cli_query_render.py
Normal file
66
tests/test_cli_query_render.py
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
"""CLI commands route BQ-typed errors through the shared renderer.
|
||||
|
||||
Three CLI paths surface BQ errors today:
|
||||
- `da query --remote` (cli/commands/query.py:_query_remote → /api/query)
|
||||
- `da query --register-bq` (cli/commands/query.py:_query_hybrid via
|
||||
RemoteQueryError, which wraps server-side BqAccessError)
|
||||
- `da fetch` / `da schema` / etc. (cli/v2_client.V2ClientError → v2 endpoints)
|
||||
|
||||
After the refactor they all call cli.error_render.render_error so analyst
|
||||
output is consistent and structured. Closes part of #160 §4.7.3.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def test_v2_client_error_uses_renderer():
|
||||
"""`V2ClientError.__str__` calls render_error so any caller of the v2
|
||||
HTTP helpers (api_get_json/api_post_json/api_post_arrow) automatically
|
||||
surfaces typed BQ errors as structured output."""
|
||||
from cli.v2_client import V2ClientError
|
||||
|
||||
body = {"detail": {
|
||||
"kind": "cross_project_forbidden",
|
||||
"message": "USER_PROJECT_DENIED",
|
||||
"hint": "Set data_source.bigquery.billing_project",
|
||||
}}
|
||||
err = V2ClientError(status_code=502, body=body)
|
||||
out = str(err)
|
||||
# Old form: `HTTP 502: {'detail': {'kind': ...}}` (single line).
|
||||
# New form: multi-line structured.
|
||||
assert "\n" in out, f"V2ClientError must use multi-line renderer; got {out!r}"
|
||||
assert "cross_project_forbidden" in out
|
||||
assert "billing_project" in out
|
||||
|
||||
|
||||
def test_v2_client_error_drops_truncation_for_dicts():
|
||||
"""The OLD `message=str(body)[:200]` truncation hid the structured
|
||||
`hint` field for any reasonably-sized error dict. The new renderer
|
||||
must NOT pre-truncate dict bodies."""
|
||||
from cli.v2_client import V2ClientError
|
||||
|
||||
body = {"detail": {
|
||||
"kind": "bq_forbidden",
|
||||
"billing_project": "x" * 200, # padding to push past old 200-char limit
|
||||
"data_project": "y" * 200,
|
||||
"hint": "MUST_REACH_THIS_HINT_IN_OUTPUT",
|
||||
}}
|
||||
err = V2ClientError(status_code=502, body=body)
|
||||
out = str(err)
|
||||
assert "MUST_REACH_THIS_HINT_IN_OUTPUT" in out, \
|
||||
"renderer must not pre-truncate dict bodies past the hint field"
|
||||
|
||||
|
||||
def test_remote_query_error_carries_details():
|
||||
"""`RemoteQueryError` already has a `details` field. Verify the type's
|
||||
surface so cli/commands/query.py:_query_hybrid can rely on it."""
|
||||
from src.remote_query import RemoteQueryError
|
||||
|
||||
err = RemoteQueryError(
|
||||
error_type="cross_project_forbidden",
|
||||
message="USER_PROJECT_DENIED",
|
||||
details={"billing_project": "", "data_project": "prj"},
|
||||
)
|
||||
assert err.details == {"billing_project": "", "data_project": "prj"}
|
||||
assert err.error_type == "cross_project_forbidden"
|
||||
57
tests/test_remote_query_error_details.py
Normal file
57
tests/test_remote_query_error_details.py
Normal file
|
|
@ -0,0 +1,57 @@
|
|||
"""`src/remote_query.py:RemoteQueryError` carries a `details` dict
|
||||
(verified to already exist) populated for raise sites that wrap an
|
||||
upstream BqAccessError.
|
||||
|
||||
Currently only the BqAccessError-wrap path (lines 422 + 432) populates
|
||||
it. The other 11 raise sites (lines 134, 142, 167, 173, 259, 264, 282,
|
||||
289, 313, 322, 375) need an audit — for sites that wrap an external
|
||||
exception, populate `details` so the CLI renderer has the structured
|
||||
context it needs.
|
||||
|
||||
Closes the audit half of #160 §4.7.3.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
|
||||
def test_blocked_keyword_carries_keyword_in_details():
|
||||
"""`raise RemoteQueryError("blocked_keyword", ..., details={"blocked_keyword": kw})`
|
||||
already populates the keyword. Lock it in via assertion so a future
|
||||
refactor doesn't drop the field."""
|
||||
from src.remote_query import RemoteQueryEngine, RemoteQueryError
|
||||
import duckdb
|
||||
|
||||
conn = duckdb.connect(":memory:")
|
||||
engine = RemoteQueryEngine(conn)
|
||||
try:
|
||||
engine.execute("DROP TABLE foo")
|
||||
except RemoteQueryError as exc:
|
||||
assert exc.error_type == "blocked_keyword", exc.error_type
|
||||
# 'drop' is the keyword that triggers the block.
|
||||
assert exc.details, "blocked_keyword raise must populate details"
|
||||
assert "blocked_keyword" in exc.details
|
||||
else:
|
||||
raise AssertionError("expected blocked_keyword RemoteQueryError")
|
||||
|
||||
|
||||
def test_query_must_be_select_carries_no_unnecessary_details():
|
||||
"""A pure local-validation raise (SQL doesn't start with SELECT)
|
||||
has nothing structured to surface. details=None is the right shape;
|
||||
test locks that in so a future contributor doesn't add noise."""
|
||||
from src.remote_query import RemoteQueryEngine, RemoteQueryError
|
||||
import duckdb
|
||||
|
||||
conn = duckdb.connect(":memory:")
|
||||
engine = RemoteQueryEngine(conn)
|
||||
try:
|
||||
engine.execute("WITH x AS (SELECT 1) SELECT * FROM x")
|
||||
except RemoteQueryError as exc:
|
||||
# `WITH ...` is treated as not-starting-with-SELECT today; this is
|
||||
# the shape we want to assert: details may be empty/None for pure
|
||||
# local-validation errors.
|
||||
assert exc.error_type in ("query_must_be_select", "blocked_keyword"), exc.error_type
|
||||
# Either empty dict or dict with explanatory keys is fine.
|
||||
assert isinstance(exc.details, dict)
|
||||
except Exception:
|
||||
# If the engine accepts WITH, that's also fine — this test is
|
||||
# primarily about details shape, not what gets blocked.
|
||||
pass
|
||||
Loading…
Reference in a new issue