test: catch models that silently drop MLB API values, and fix the 50 remaining fields - #249
Closed
Mattsface wants to merge 7 commits into
Closed
test: catch models that silently drop MLB API values, and fix the 50 remaining fields#249Mattsface wants to merge 7 commits into
Mattsface wants to merge 7 commits into
Conversation
MLBBaseModel ignores unknown keys, so a field whose alias does not match the API exactly parses as None instead of raising. Issue #246 shipped that way in v0.7.1 and survived a fully green test run, because the stats tests assert on the response envelope and never on a stat value. Adds two layers of checking: - tests/test_model_alias_coverage.py runs offline against payloads recorded from the real API, asserting that every recorded key naming a declared field actually reaches it, and that no field in the package expects a casing the API does not use. - tests/external_tests/models/test_alias_drift_live.py repeats those checks against the live API so renames on MLB's side surface without a re-record. Fixtures are regenerated with tests/tools/record_api_fixtures.py. These tests fail on main; the following commit fixes the fields they report. Co-authored-by: Matthew Spah <[email protected]>
Completes the fix started in #244 and #247. The camelCase alias generator added in v0.7.1 only capitalises across underscores, so every field whose name is a single lowercase token kept expecting its own name: strikeouts, putouts, walkoffs, nickname, timezone. MLB sends strikeOuts, putOuts, walkOffs, nickName and timeZone, and because MLBBaseModel ignores unknown keys those values were discarded rather than raising. Corrects the 50 remaining fields, covering pitching and fielding splits, sabermetrics, Person and its subclasses, Season, Venue, standings and team records, schedule and game data, and pitch coordinates. Two spellings could not be derived from any rule, so they are pinned with a comment: MLB returns offseasonStartDate alongside offSeasonEndDate, and AdvancedPitchingSplit's strikeouts-to-walks ratio arrives as 'strikesoutsToWalks'. The latter accepts both spellings via AliasChoices in case MLB corrects the typo. Co-authored-by: Matthew Spah <[email protected]>
get_venue and get_venues set params['hydrate'] to a list, which requests encodes as repeated hydrate query params. MLB answers those without hydrating, so Venue.location and Venue.timezone were always None no matter what the aliases said. The existing venue test only checked hasattr, which passes on a pydantic model whether or not a value arrived, so this was invisible. Adds assertions on the hydrated values and on the encoded parameter. Co-authored-by: Matthew Spah <[email protected]>
CI ran only tests/external_tests/, so tests/test_base_model.py and tests/test_gamepk_validation_regressions.py have never executed on a push, and the new alias coverage tests would not have either. Co-authored-by: Matthew Spah <[email protected]>
The first crawl took fielding stats from an outfielder, a person record from a living player and a game feed from before ABS challenges existed, so whole groups of keys were never observed and the guard stayed silent on them. Adds a catcher's fielding split, the outsAboveAverage stat type, a deceased player and a recent game feed, taking confirmed coverage of model fields from 96% to 98% (918 distinct keys, up from 835). Also asserts the recorded vocabulary still covers most of the model surface, so a failed or truncated re-record cannot quietly disable these checks. Co-authored-by: Matthew Spah <[email protected]>
SimpleFieldingSplit.catcher_era read 'catcherEra'. The field only appears on a catcher's fielding split, which the earlier crawl never requested. Co-authored-by: Matthew Spah <[email protected]>
get_player_stats(stats=['outsAboveAverage']) raised ValidationError with 17 missing fields for every player. OutsAboveAverage declared the statistics directly on the split, but MLB nests them under 'stat' like every other split type, so none of them were ever found and all were required. Moves them to an OutsAboveAverageSplit that the split holds as stat, matching PitchArsenal and the other Split subclasses, and makes them optional since this stat type returns zeroes or nothing at all for most players. Adds fieldingRunsPrevented, which the response includes but the model omitted. The float widening is deliberate: the API returns values such as 8.0. Co-authored-by: Matthew Spah <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
MLBBaseModelis configuredextra="ignore", so a field whose alias does not match the key MLB sends is not a validation error — the value is discarded and the field keeps itsNonedefault forever. Nothing raises, nothing logs.That is how issue #246 shipped in v0.7.1. When the data adapter stopped lowercasing every response key and moved to a camelCase alias generator, the generator only capitalised across underscores, so every field whose name is a single lowercase token kept expecting its own name.
strikeoutsexpectsstrikeouts, MLB sendsstrikeOuts. Same forputouts,walkoffs,nickname,timezone,roundrobin, and theax/ay/azpitch coordinates.The important part is that the full suite was green the whole time. The stats tests assert on the response envelope (
group,type,total_splits == len(splits)) and never on a stat value, so 62 permanently-Nonefields produced 191 passing tests. #244 and #247 fixed 12 of those fields; this PR adds the checks that would have caught all of them, and fixes what was left.What
Tests (the point of the PR). Two layers, both driven by
tests/alias_audit.py:tests/test_model_alias_coverage.pyruns offline against payloads recorded from the real API. It asserts that every recorded key naming a declared field actually reaches that field, that no field anywhere in the package expects a casing MLB does not use, and that each field named in the bug reports is populated.tests/external_tests/models/test_alias_drift_live.pyrepeats those checks against the live API, so a rename on MLB's side surfaces even if nobody re-records, and flags fixtures that have gone stale.Fixtures live in
tests/fixtures/and are regenerated withpython tests/tools/record_api_fixtures.py, the only part that touches the network. It crawls 28 endpoints and captures 918 distinct keys.Choosing the endpoints turned out to matter more than the checking logic. The first crawl took fielding stats from an outfielder, a person record from a living player and a game feed from before ABS challenges existed, so entire groups of keys were never observed and the guard stayed silent on them. The crawl now includes a catcher's fielding split, the
outsAboveAveragestat type, a deceased player and a recent game feed, which took confirmed coverage of model fields from 96% to 98%. A floor test keeps that from silently regressing if a re-record fails.Fixes needed to make those tests pass. 50 field aliases across pitching and fielding splits, sabermetrics,
Personand its subclasses,Season,Venue, standings and team records, schedule and game data, and pitch coordinates. Two spellings cannot be derived from any rule and are pinned with a comment: MLB returnsoffseasonStartDatealongsideoffSeasonEndDate, andAdvancedPitchingSplit's strikeouts-to-walks ratio arrives asstrikesoutsToWalks. That last one accepts both spellings viaAliasChoicesin case MLB ever corrects it.Three further bugs the coverage work surfaced, each in its own commit:
get_venueandget_venuessetparams['hydrate']to a Python list, whichrequestsencodes as repeatedhydratequery params. MLB answers those without hydrating, soVenue.locationandVenue.timezonewereNoneregardless of aliases. The existing venue test assertedhasattr, which passes on a pydantic model whether or not a value arrived.SimpleFieldingSplit.catcher_erareadcatcherEra; MLB sendscatcherERA. Only visible on a catcher's fielding split.get_player_stats(stats=['outsAboveAverage'])raisedValidationErrorwith 17 missing fields for every player.OutsAboveAveragedeclared the statistics directly on the split, but MLB nests them understatlike every other split type. Now nested in anOutsAboveAverageSplit, matchingPitchArsenal, with the fields optional andfieldingRunsPreventedadded.CI. All three workflows ran
pytest tests/external_tests/only, sotests/test_base_model.pyandtests/test_gamepk_validation_regressions.pyhave never executed on a push and the new offline tests would not have either. They now runpytest tests/.Commits are ordered so the story is reviewable: the first test commit fails on
mainand reports exactly the fields that are broken, and the following commits fix them.Tests
poetry run pytest tests/goes from 191 passed to 294 passed, 2 skipped, including the live-API checks. CI passes on 3.10, 3.11 and 3.12.Every alias in this PR was read off a live response rather than inferred — all 32 distinct alias strings introduced are present in the recorded vocabulary. Verified end to end against the live API:
Also mutation-tested the guard: reverting
SimpleFieldingSplit.putoutsto its broken alias fails three tests with a message naming the field, the alias it expects, and the key the API sends.Known blind spot. 50 of 2787 model fields still never appear in the crawl, because MLB only sends them in situations that cannot be requested on demand — resumed and rescheduled games, ejections, injury play events, delay durations — plus
Stat.total_splits, whichmlb_modulecomputes rather than reads. Those fields are unverified either way; the guard reports a field only when the API demonstrably uses the key, so they stay quiet rather than producing false positives.Risk and impact
Risk level: Normal
The alias corrections are mechanical and additive — fields that were unreachable now populate, and no field name, type or default changes, so existing attribute access keeps working. The one behavioural change users could notice is that code branching on these values being
Nonewill now take the other branch, which is the intended fix.OutsAboveAverageis the only shape change: its statistics move from the split tosplit.stat. Nothing can depend on the old shape, because constructing it from a real response has always raised.The venue hydrate change alters an outgoing request parameter, covered by both a live assertion and an offline assertion on the encoded value.
If something does go wrong it is contained to reading a field that was already unusable, and every alias change can be reverted individually.
Not included
Issue #245 (schedule requiring a date) is a separate bug in
get_schedule/get_scheduled_games_by_dateand is left out to keep this reviewable. Open PR #241 (str to float stat conversion) touches several of the same files, so it will need a rebase whichever lands second.