tests: check that client and server schemas match#1192
Conversation
MatthewJamisonJS
left a comment
There was a problem hiding this comment.
Pulled this down, all 15 pass in <0.1s. Test catches the #1189 class of bug cleanly. One thing I think is worth fixing before it lands, plus a couple smaller notes.
The optionality axis isn't covered. _types_compatible("Optional[str]", "str") returns True, so if a client field is Optional[str] = None and the corresponding server field is required str, the test passes but the client can still send None and the server will reject it. Same shape of bug as #1189 just on a different axis. You're already parsing required so it's basically:
@pytest.mark.parametrize("label,client_cls,server_cls", SCHEMA_PAIRS)
def test_required_alignment_on_shared_fields(label, client_cls, server_cls):
client = _parse_class_fields(CLIENT_SCHEMA_PATH, client_cls)
server = _parse_class_fields(SERVER_SCHEMA_PATH, server_cls)
weakened = [
f for f in set(client) & set(server)
if server[f]["required"] and not client[f]["required"]
]
assert not weakened, f"[{label}] server requires fields client treats as optional: {weakened}"Smaller stuff:
_unwrap_optional only handles the Optional[X] spelling. The moment anyone writes str | None or Union[str, None] in a schema the test will start false-failing — not silent, just noisy. Worth handling now if you want to keep it cheap to maintain.
Field() with nothing in it reads as optional today (no Ellipsis, no default=). In Pydantic v2 that's actually required. None of the current schemas use it so it's not biting, but if you're already touching the helper for the optionality fix it's a one-liner to cover.
And SCHEMA_PAIRS is (name, name, name) five times — is that there because you're expecting the server classes to get renamed (Base → Read or whatever) at some point? If so a comment would save the next person guessing, otherwise it can collapse to a list of strings.
Nothing in the second half is a blocker for what the PR actually claims to do. The required-alignment one I think is — it's the same kind of silent wire mismatch the test was written to prevent.
Add 15 parametrized tests across EmissionBase, RunBase, ExperimentBase, ProjectBase and OrganizationBase that verify the client (codecarbon/core/schemas.py) and server (carbonserver/carbonserver/api/schemas.py) schemas are compatible. Tests check: - All client fields exist in the server schema - All required server fields exist in the client schema - Shared fields have wire-compatible types Uses ast to parse schema files without importing either module, so the test runs in the client test environment with no extra deps. This would have caught the on_cloud: str vs bool mismatch fixed in mlco2#1189. Closes mlco2#1190
8081f41 to
fa44a54
Compare
|
Thanks for the thorough review. Addressed everything in the latest push:
|
Description
Adds a test suite that verifies the client (
codecarbon/core/schemas.py) and server (carbonserver/carbonserver/api/schemas.py) schemas stay compatible, addressing #1190.What it checks
For each shared model pair (
EmissionBase,RunBase,ExperimentBase,ProjectBase,OrganizationBase) — 15 parametrized tests total:Why
astinstead of importsThe client and server live in separate
uvenvironments. Using Python'sastmodule to parse schema files as source avoids any import or path issues — the test runs in the existing client test environment with zero extra dependencies.Regression proof
This test catches the exact bug fixed in #1189. Temporarily changing
on_cloud: Optional[bool]toOptional[str]on the server produces:Related Issue
Closes #1190
Types of changes
AI Usage Disclosure
Checklist