Cover the typed batch payloads with offline tests and typechecking - #47
Merged
vigneshwerv merged 4 commits intoAug 10, 2026
Merged
Conversation
Contributor
Author
Correction to the original description: I claimed The churn came from my own environment using isort 8.0.1 where |
The typed-entry tests assert on render_module's output as a string, which cannot tell a working module from one that merely contains the right substrings. Nothing imported the per-entry classes, so a duplicated field declaration or an annotation the header does not import passed every test. tests/test_typed_entries_generated.py renders into a throwaway package and imports it, then uses the classes: the optional-parameter branch (no snapshot query has a nullable parameter), field-name collisions, escaped names, and to_entry_inputs. It also exercises every model in the committed snapshot, which is the artifact a customer gets. tests/test_typed_entries_warnings.py covers the paths where codegen degrades rather than fails. Each leaves a working but quietly worse SDK, and the warning is the only signal, so a silent version of any of them passed the whole suite. tests/type_checks/ asserts what a caller sees, which no runtime test can: a runtime test passes just as happily against `entries: Any`. Negative cases are written as `# type: ignore[...]` and warn_unused_ignores makes them fail if the call ever starts passing, so the signature cannot loosen unnoticed. CI ran mypy -p fragment only, leaving both tests/ and the mypy_path setting unchecked. `make typecheck` now covers tests/ as well, and typecheck plus the offline tests run in a job with no secrets -- on forks the credentialed job errors on every test, so nothing ran at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vigneshwerv
force-pushed
the
claude/typed-entries-test-coverage
branch
from
August 10, 2026 19:13
fbcd5fe to
1a1baa4
Compare
Merging typed-batch-ledger-entries dropped the `pytestmark` line from tests/test_add_ledger_entries.py. Nothing failed loudly: `make unit` deselected 1 test instead of 3, ran the two credential-bound tests offline, and they errored on missing environment variables. conftest now marks anything whose fixture closure includes `credentials`, so the marker follows from what a test actually needs. A new integration test cannot forget it and a merge cannot drop it. The per-module markers are redundant under that rule and are removed, leaving one mechanism.
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.
Stacked on #45 (base
typed-batch-ledger-entries). Tests and typechecking only — no behaviour changes. #45's five review items are all resolved there; this fills the coverage holes flagged alongside them.What was uncovered
Nothing imported the generated per-entry classes. The existing tests assert on
render_module's output as a string, and thesamplefixture builds a model by hand fromBASE_CLASS_SOURCE. A rendered module that declared the same field twice, or referenced a name the header does not import, satisfied all of them.The optional-parameter branch had no coverage at all. Every CLI-generated parameter is non-null, so no snapshot query reaches the renderer's
= Nonepath.The warning paths had none either. Each leaves a working but quietly worse SDK — a parameter that lost its type, a batch method that does not typecheck, models nothing accepts. A silent version of any of them passed the whole suite.
CI ran
mypy -p fragmentonly.tests/was never typechecked, so themypy_pathsetting added in #45 was dead config, and the PR's central claim — that a caller gets real type errors — was verified by nothing.On forks, no test ran at all. The only job that ran pytest needs secrets, and
conftest.credentialsfails the run without them.What this adds
tests/test_typed_entries_generated.pyto_entry_inputs, the empty-module shape thatfragment/sdk/typed_entries.pyactually is, and every model in the committed snapshot.tests/test_typed_entries_warnings.pytests/type_checks/batch_entries.py# type: ignore[...], andwarn_unused_ignoresfails the day one starts passing.make typechecknow coverstests/too,make unitruns everything offline, and a new credential-free CI job runs both.76 offline tests, 0.2s, no network.
Verification
Every new test was checked by breaking the thing it guards:
= Nonefor optional parameterstypeVersionfromto_inputlist(entries)coercionentriestoAnySequenceback tolistThe last two are the point of
type_checks/: it fails both when the signature loosens and when it tightens too far.One-line change to #45's files
pytestmark = pytest.mark.integrationin the two integration modules. Theirsnapshot_clientfixture is untouched, including the comment about**credentials— that stays accurate, since the fixture still annotatesDict[str, str].🤖 Generated with Claude Code