From b12acbdb6ab52b6fe8ed535cd205cd055ed5fd39 Mon Sep 17 00:00:00 2001 From: Roland Date: Thu, 30 Jul 2026 17:27:31 -0600 Subject: [PATCH] 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) --- app/src/test-fixtures.js | 4 +- wrolpi/schema.py | 26 +++++++++-- wrolpi/test/test_api_fixtures.py | 78 ++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 4 deletions(-) diff --git a/app/src/test-fixtures.js b/app/src/test-fixtures.js index 54d79542..bf5467c5 100644 --- a/app/src/test-fixtures.js +++ b/app/src/test-fixtures.js @@ -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: {}, diff --git a/wrolpi/schema.py b/wrolpi/schema.py index 61afee8b..1d0a32ae 100644 --- a/wrolpi/schema.py +++ b/wrolpi/schema.py @@ -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] diff --git a/wrolpi/test/test_api_fixtures.py b/wrolpi/test/test_api_fixtures.py index 892af397..d565995b 100644 --- a/wrolpi/test/test_api_fixtures.py +++ b/wrolpi/test/test_api_fixtures.py @@ -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.