Skip to content

Commit 3c11ff6

Browse files
rustyconoverclaude
andcommitted
tests: type the split-token tests against the real production types
The mypy gate on tests/ was added in 0.26.0 with tests/ clean, but a run of commits then landed without CI ever executing, so 21 errors accumulated unseen. They are not one mistake repeated; they are three, and each one is the test having drifted from production rather than production being wrong. Structural stand-ins for nominal types. test_split_token.py built a local _Auth class with the three attributes _identity_tail reads, and a local _Ctx with a bare `auth = None`. Both work at runtime because Python does not check, but AuthContext and CallContext are nominal types with real constructors, and every other test in the tree (test_worker, test_auth, test_aggregate_function) already builds them directly. Replaced with AuthContext(domain=..., authenticated=True, principal=...) and the `CallContext(auth=AuthContext.anonymous(), ...)` idiom the rest of the suite uses. Behaviourally identical: _identity_tail maps both `None` and an unauthenticated context to the same anonymous tail. Borrowing an unbound method instead of inheriting it. Three test-local worker doubles did `_stamp_split_tokens = Worker._stamp_split_tokens` on a class that was not a Worker, which mypy correctly flags as an invalid self argument -- the double satisfied the method's needs by accident, and would keep compiling if _stamp_split_tokens grew a dependency on any other Worker member. They are now Worker subclasses overriding _current_split_anchor, which is the seam the method actually documents. Worker is constructible with no arguments, so this costs nothing. Wire types passed as their serialized shape. `arguments=b""` and `function_type="TABLE"` are what those fields become on the wire, not what BindRequest declares (Arguments and the FunctionType enum). Every other BindRequest in the suite already passes the declared types; these three were written before anything checked. Also narrowed PlanResponse.splits at its two use sites. It is genuinely `list[ScanSplit] | list[bytes]` because a worker may hand back either, but stamping always returns the serialized form -- so the new _stamped() helper asserts that rather than casting it away, which pins that half of the contract while it narrows. One unrelated error in test_wire_record_schema_parity.py: a bare pa.Scalar where strict mode wants pa.Scalar[Any], matching how vgi/arguments.py spells it. No production code changed; no type was loosened and no ignore was added. `mypy vgi/`, `mypy tests/`, ruff format/check, and the full suite (2438 passed) are all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent cdb50d0 commit 3c11ff6

2 files changed

Lines changed: 47 additions & 52 deletions

File tree

‎tests/test_split_token.py‎

Lines changed: 46 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,14 @@
99

1010
import json
1111
import pathlib
12+
from typing import Any
1213

1314
import pytest
15+
from vgi_rpc.rpc import AuthContext, CallContext
1416

17+
from vgi.arguments import Arguments
18+
from vgi.invocation import FunctionType
19+
from vgi.protocol import BindRequest, PlanResponse, ScanSplit, TableFunctionPlanRequest
1520
from vgi.split_token import (
1621
_HEADER_STRUCT,
1722
FLAG_PAYLOAD_SEALED,
@@ -33,11 +38,27 @@
3338
PAYLOAD = MANIFEST["payload"].encode()
3439

3540

36-
class _Auth:
37-
def __init__(self, principal: str) -> None:
38-
self.authenticated = True
39-
self.domain = "test"
40-
self.principal = principal
41+
def _auth(principal: str) -> AuthContext:
42+
"""A caller identity, as the transports actually build one."""
43+
return AuthContext(domain="test", authenticated=True, principal=principal)
44+
45+
46+
def _anon_ctx() -> CallContext:
47+
"""The unauthenticated call context a subprocess worker runs under."""
48+
return CallContext(auth=AuthContext.anonymous(), emit_client_log=lambda *a, **kw: None)
49+
50+
51+
def _stamped(response: PlanResponse, index: int) -> bytes:
52+
"""The serialized bytes of one stamped split.
53+
54+
``PlanResponse.splits`` is ``list[ScanSplit] | list[bytes]`` because a worker
55+
may hand back either, but stamping always returns the serialized form — so
56+
asserting it here both narrows the union for the reader and pins that half of
57+
the contract.
58+
"""
59+
entry = response.splits[index]
60+
assert isinstance(entry, bytes), "stamping must serialize every split"
61+
return entry
4162

4263

4364
# --------------------------------------------------------------------------- #
@@ -105,11 +126,11 @@ def test_seal_binds_the_caller_principal() -> None:
105126
a split token names data (files, offsets, tenant partitions).
106127
"""
107128
token = build_split_token(
108-
payload=PAYLOAD, fingerprint=FINGERPRINT, anchor=ANCHOR, signing_key=KEY, auth=_Auth("alice")
129+
payload=PAYLOAD, fingerprint=FINGERPRINT, anchor=ANCHOR, signing_key=KEY, auth=_auth("alice")
109130
)
110-
assert open_split_token(token, signing_key=KEY, auth=_Auth("alice")) == PAYLOAD
131+
assert open_split_token(token, signing_key=KEY, auth=_auth("alice")) == PAYLOAD
111132
with pytest.raises(SplitTokenInvalid):
112-
open_split_token(token, signing_key=KEY, auth=_Auth("bob"))
133+
open_split_token(token, signing_key=KEY, auth=_auth("bob"))
113134

114135

115136
def test_header_is_covered_by_the_aad() -> None:
@@ -156,7 +177,7 @@ def test_error_kinds_are_stable_strings() -> None:
156177

157178

158179
@pytest.mark.parametrize("case", MANIFEST["cases"], ids=lambda c: c["name"])
159-
def test_fixture_vectors_reach_their_recorded_verdict(case: dict) -> None:
180+
def test_fixture_vectors_reach_their_recorded_verdict(case: dict[str, Any]) -> None:
160181
"""Every checked-in vector must reach the verdict its manifest records."""
161182
raw = (FIXTURES / f"{case['name']}.bin").read_bytes()
162183
assert len(raw) == case["size"]
@@ -224,31 +245,22 @@ def test_anchor_defaults_to_the_live_catalog_version_not_zero() -> None:
224245
Invisible wherever the catalog's version is 0, which is most fixtures — so this
225246
pins the non-zero case specifically.
226247
"""
227-
from vgi.protocol import BindRequest as PBindRequest
228-
from vgi.protocol import PlanResponse, ScanSplit, TableFunctionPlanRequest
229248

230-
class _VersionedWorker:
249+
class _VersionedWorker(Worker):
231250
"""Stands in for a worker whose catalog reports a non-zero version."""
232251

233-
_signing_key = None
234-
235-
def _current_split_anchor(self, request: object, ctx: object) -> bytes:
252+
def _current_split_anchor(self, request: Any, ctx: CallContext | None) -> bytes:
236253
return (47).to_bytes(8, "little", signed=True)
237254

238-
_stamp_split_tokens = Worker._stamp_split_tokens
239-
240-
bind = PBindRequest(function_name="f", arguments=b"", function_type="TABLE")
255+
bind = BindRequest(function_name="f", arguments=Arguments(positional=()), function_type=FunctionType.TABLE)
241256
request = TableFunctionPlanRequest(bind_call=bind)
242257

243-
class _Ctx:
244-
auth = None
245-
246258
# on_plan left catalog_version unset — the common case for a worker that has
247259
# not thought about snapshots.
248260
planned = PlanResponse(splits=[ScanSplit(payload=b"file=1")])
249-
stamped = _VersionedWorker()._stamp_split_tokens(planned, request, _Ctx())
261+
stamped = _VersionedWorker()._stamp_split_tokens(planned, request, _anon_ctx())
250262

251-
split = ScanSplit.deserialize_from_bytes(stamped.splits[0])
263+
split = ScanSplit.deserialize_from_bytes(_stamped(stamped, 0))
252264
# Redemption compares against the live version; the token must name it.
253265
opened = open_split_token(
254266
split.token,
@@ -260,26 +272,17 @@ class _Ctx:
260272

261273
def test_a_worker_that_names_its_version_is_taken_at_its_word() -> None:
262274
"""An explicit ``catalog_version`` wins: the worker knows which snapshot it planned."""
263-
from vgi.protocol import BindRequest as PBindRequest
264-
from vgi.protocol import PlanResponse, ScanSplit, TableFunctionPlanRequest
265-
266-
class _VersionedWorker:
267-
_signing_key = None
268275

269-
def _current_split_anchor(self, request: object, ctx: object) -> bytes:
276+
class _VersionedWorker(Worker):
277+
def _current_split_anchor(self, request: Any, ctx: CallContext | None) -> bytes:
270278
return (47).to_bytes(8, "little", signed=True)
271279

272-
_stamp_split_tokens = Worker._stamp_split_tokens
273-
274-
bind = PBindRequest(function_name="f", arguments=b"", function_type="TABLE")
280+
bind = BindRequest(function_name="f", arguments=Arguments(positional=()), function_type=FunctionType.TABLE)
275281
request = TableFunctionPlanRequest(bind_call=bind)
276282

277-
class _Ctx:
278-
auth = None
279-
280283
planned = PlanResponse(splits=[ScanSplit(payload=b"file=1")], catalog_version=11)
281-
stamped = _VersionedWorker()._stamp_split_tokens(planned, request, _Ctx())
282-
split = ScanSplit.deserialize_from_bytes(stamped.splits[0])
284+
stamped = _VersionedWorker()._stamp_split_tokens(planned, request, _anon_ctx())
285+
split = ScanSplit.deserialize_from_bytes(_stamped(stamped, 0))
283286

284287
with pytest.raises(SplitSnapshotExpired):
285288
open_split_token(
@@ -310,32 +313,24 @@ def test_stamping_clears_the_plaintext_payload() -> None:
310313
this is a leak with no consumer, which is the easiest kind to leave in place
311314
for years.
312315
"""
313-
from vgi.protocol import BindRequest as PBindRequest
314-
from vgi.protocol import PlanResponse, ScanSplit, TableFunctionPlanRequest
315-
316316
secret = b"file=/tenant-a/private.parquet;v=47"
317317

318-
class _KeyedWorker:
318+
class _KeyedWorker(Worker):
319319
_signing_key = b"k" * 32
320320

321-
def _current_split_anchor(self, request: object, ctx: object) -> bytes:
321+
def _current_split_anchor(self, request: Any, ctx: CallContext | None) -> bytes:
322322
return (0).to_bytes(8, "little", signed=True)
323323

324-
_stamp_split_tokens = Worker._stamp_split_tokens
325-
326-
class _Ctx:
327-
auth = None
328-
329-
bind = PBindRequest(function_name="f", arguments=b"", function_type="TABLE")
324+
bind = BindRequest(function_name="f", arguments=Arguments(positional=()), function_type=FunctionType.TABLE)
330325
request = TableFunctionPlanRequest(bind_call=bind)
331-
stamped = _KeyedWorker()._stamp_split_tokens(PlanResponse(splits=[ScanSplit(payload=secret)]), request, _Ctx())
326+
stamped = _KeyedWorker()._stamp_split_tokens(PlanResponse(splits=[ScanSplit(payload=secret)]), request, _anon_ctx())
332327

333328
# Not in the field...
334-
split = ScanSplit.deserialize_from_bytes(stamped.splits[0])
329+
split = ScanSplit.deserialize_from_bytes(_stamped(stamped, 0))
335330
assert split.payload == b""
336331
# ...and not anywhere else in the serialized record either, which is the
337332
# assertion that survives someone moving the leak to a different column.
338-
assert secret not in stamped.splits[0]
333+
assert secret not in _stamped(stamped, 0)
339334

340335
# Still fully recoverable from the token, so clearing the field cost nothing.
341336
opened = open_split_token(

‎tests/test_wire_record_schema_parity.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class WireRecordCase:
6767
build: Callable[[], Any]
6868

6969

70-
def _scalar_args() -> tuple[list[pa.Scalar], dict[str, pa.Scalar]]:
70+
def _scalar_args() -> tuple[list[pa.Scalar[Any]], dict[str, pa.Scalar[Any]]]:
7171
return (
7272
[pa.scalar("s3://bucket/x.parquet", pa.string())],
7373
{"hive_partitioning": pa.scalar(True, pa.bool_())},

0 commit comments

Comments
 (0)