mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 19:01:39 -04:00
test: check declared types, not just field names, and fix the three that were wrong
Greptile's point on #592 was right and generalized further than the case it raised. The guard compared property names only, so it passed while the published contract disagreed with the endpoint about what those properties contain. Three wrong types, all pre-existing, all now caught by a test: StatusResponse.downloads was `int` while the handler has always sent the download manager's summary object -- the spec told a generated client to expect a number where it gets a dict. It is now DownloadsSummaryResponse, with the six fields get_summary() actually returns. StatusResponse.wrol_mode was `str`; wrol_mode_enabled() returns a bool. SettingsResponse.log_level was `str` while the handler sends a Python logging integer -- SettingsRequest.log_level already said int, and the frontend converts it numerically through fromApiLogLevel(). settingsFixture was carrying 'info', which that function maps to undefined, so every Settings test has rendered a blank log-level label where production shows "Info". The fixture now carries 20. This came from the same hand-written cypress body as cpu_percent did. statusFixture's nested downloads was also short a field: daily_limit_reached, one the API has always sent. /api/status returns an empty downloads object while the database is down, which it is for much of the suite, so the endpoint cannot vouch for its own nested shape -- get_summary() is the authority and is now compared directly. The type check runs against both endpoints rather than only the one the review named; that is how log_level surfaced. Comparison is limited to properties the spec gives a type for and the handler actually sent, since a null from an optional field says nothing about its declared type. Fixture values are still compared by name only: they are read from JS source rather than executed, so their types are not reliably available. Said plainly in the test rather than left to look like coverage. 1709 pytest, 986 jest, 46 e2e. One unrelated flake in one backend run (the known filename-formatting tests under -nauto), clean across two further full runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
f91eed0251
commit
b12acbdb6a
3 changed files with 104 additions and 4 deletions
|
|
@ -62,7 +62,8 @@ export const settingsFixture = (overrides = {}) => ({
|
|||
ignore_outdated_zims: false,
|
||||
// Relative to the media directory, as the API returns them.
|
||||
ignored_directories: [],
|
||||
log_level: 'info',
|
||||
// Python logging integers: 40 error, 30 warning, 20 info, 10 debug, 5 trace.
|
||||
log_level: 20,
|
||||
map_default_location: null,
|
||||
map_destination: 'map',
|
||||
// `/media/wrolpi` on a Pi, but the API is the authority on this and a test that
|
||||
|
|
@ -108,6 +109,7 @@ export const statusFixture = (overrides = {}) => ({
|
|||
disabled: false,
|
||||
stopped: false,
|
||||
outside_download_window: false,
|
||||
daily_limit_reached: false,
|
||||
},
|
||||
drives_stats: [],
|
||||
flags: {},
|
||||
|
|
|
|||
|
|
@ -35,7 +35,10 @@ class SettingsResponse:
|
|||
check_for_upgrades: bool
|
||||
ignore_outdated_zims: bool
|
||||
ignored_directories: List[str]
|
||||
log_level: str
|
||||
# A Python logging integer (INFO is 20), not a name. Declared `str` here while
|
||||
# SettingsRequest.log_level already said int, and the frontend converts it numerically --
|
||||
# so the fixture was carrying 'info', which fromApiLogLevel() maps to undefined.
|
||||
log_level: int
|
||||
map_default_location: Optional[dict]
|
||||
nav_color: str
|
||||
media_directory: str
|
||||
|
|
@ -193,6 +196,20 @@ class DiskBandwidthStatusStat:
|
|||
speed: int
|
||||
|
||||
|
||||
@dataclass
|
||||
class DownloadsSummaryResponse:
|
||||
"""`DownloadManager.get_summary()`, which /api/status returns as `downloads`.
|
||||
|
||||
It is an empty object while the database is down.
|
||||
"""
|
||||
pending: int
|
||||
recurring: int
|
||||
disabled: bool
|
||||
stopped: bool
|
||||
outside_download_window: bool
|
||||
daily_limit_reached: bool
|
||||
|
||||
|
||||
@dataclass
|
||||
class StatusResponse:
|
||||
"""The response of GET /api/status.
|
||||
|
|
@ -209,7 +226,9 @@ class StatusResponse:
|
|||
"""
|
||||
cpu_stats: CPUStatusResponse
|
||||
dockerized: bool
|
||||
downloads: int
|
||||
# Declared `int` for a long time while the handler sent the summary object, so the published
|
||||
# contract promised a generated client a number where it gets a dict.
|
||||
downloads: DownloadsSummaryResponse
|
||||
drives_stats: List[DriveStatusStat]
|
||||
flags: FlagsStatusResponse
|
||||
is_rpi4: bool
|
||||
|
|
@ -221,7 +240,8 @@ class StatusResponse:
|
|||
processes_stats: List[ProcessStatusStat]
|
||||
sanic_workers: dict
|
||||
version: str
|
||||
wrol_mode: str
|
||||
# `str` here was wrong too; wrol_mode_enabled() returns a bool.
|
||||
wrol_mode: bool
|
||||
# Upgrade info, seeded in contexts.py and refreshed by the upgrade check.
|
||||
commits_behind: int
|
||||
current_commit: Optional[str]
|
||||
|
|
|
|||
|
|
@ -140,6 +140,84 @@ async def test_status_fixture_matches_the_spec(async_client):
|
|||
)
|
||||
|
||||
|
||||
def json_kind(value) -> str:
|
||||
"""The OpenAPI `type` a Python value corresponds to."""
|
||||
# bool before int: bool is a subclass of int, so the order matters.
|
||||
if isinstance(value, bool):
|
||||
return 'boolean'
|
||||
if isinstance(value, int):
|
||||
return 'integer'
|
||||
if isinstance(value, float):
|
||||
return 'number'
|
||||
if isinstance(value, str):
|
||||
return 'string'
|
||||
if isinstance(value, dict):
|
||||
return 'object'
|
||||
if isinstance(value, list):
|
||||
return 'array'
|
||||
return 'null'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize('path', ['/api/settings', '/api/status'])
|
||||
async def test_the_spec_declares_the_right_types(async_client, test_directory, path):
|
||||
"""Matching names is not matching shapes.
|
||||
|
||||
`StatusResponse` declared `downloads: int` while the handler has long sent the download
|
||||
manager's summary object, and `wrol_mode: str` where it sends a boolean. The published
|
||||
contract was telling a generated client to expect a number where it will receive a dict. A
|
||||
name-only comparison passes straight over both, which is what the first version of this file
|
||||
did.
|
||||
|
||||
Only the pairs the spec gives a `type` for and the handler actually sent are compared; a
|
||||
`null` from an optional field says nothing about its declared type.
|
||||
"""
|
||||
spec = await get_spec(async_client)
|
||||
properties = spec['paths'][path]['get']['responses']['default'] \
|
||||
['content']['*/*']['schema']['properties']
|
||||
|
||||
request, response = await async_client.get(path)
|
||||
assert response.status == 200
|
||||
|
||||
mismatches = []
|
||||
for name, value in response.json.items():
|
||||
declared = properties.get(name, {}).get('type')
|
||||
if not declared or value is None:
|
||||
continue
|
||||
actual = json_kind(value)
|
||||
if declared != actual:
|
||||
mismatches.append(f'{name}: spec says {declared}, handler sent {actual}')
|
||||
|
||||
assert not mismatches, f'the OpenAPI spec declares the wrong type for {path}:\n ' \
|
||||
+ '\n '.join(mismatches)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_downloads_summary_fixture_matches_the_download_manager(async_client, test_session):
|
||||
"""The one nested object worth checking by hand.
|
||||
|
||||
`/api/status` returns an empty `downloads` while the database is down, which it is for much
|
||||
of the test suite, so the endpoint's own response cannot vouch for the shape. The download
|
||||
manager's summary is the authority instead -- and statusFixture was missing
|
||||
`daily_limit_reached` from it, a field the API has always sent.
|
||||
"""
|
||||
from wrolpi.downloader import download_manager
|
||||
|
||||
declared = set(download_manager.get_summary())
|
||||
|
||||
source = TEST_FIXTURES_JS.read_text()
|
||||
block = re.search(r'^ downloads: \{(.*?)\n },', source, re.S | re.M)
|
||||
assert block, 'statusFixture no longer has a `downloads` object in the expected form'
|
||||
fixture = set(re.findall(r'^ ([a-z0-9_]+):', block.group(1), re.M))
|
||||
assert fixture, 'the downloads block appears to have no keys'
|
||||
|
||||
assert declared == fixture, (
|
||||
"statusFixture's downloads has drifted from DownloadManager.get_summary().\n"
|
||||
f' manager reports, fixture omits: {sorted(declared - fixture)}\n'
|
||||
f' fixture invents: {sorted(fixture - declared)}'
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_which_fixtures_the_spec_can_vouch_for(async_client):
|
||||
"""Name the API-shaped fixtures this file does not cover, and why.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue