Test suite, contributor evidence rules, and a maintainer guide - #3
Merged
Conversation
…uide Before this the only tests covered the XIRR solver. Everything that has ever produced a wrong number in this project — symbol normalisation, FIFO matching, cost basis, the statement parser — had none. 80 unit tests across five modules, using unittest and a TempDB fixture that gives each test its own SQLite file and its own empty config, so nothing can reach portfolio.db and results do not depend on whose machine runs them. Verified: the suite leaves a 16,034-row database untouched. Nearly every test encodes a specific failure that reached a user, and says so in a comment: a trade id reused across two years, a split that made a steady holding look re-entered, "\b20\d\d\b" reading 2073 out of a unit count, a folio carrying three different SBI schemes, a rename added after import that must not re-import the same trades. The identity realised + unrealised + dividends == net profit is asserted across four different portfolio shapes, because that is what caught most errors while this was being built. Writing them found two things. config.load() never closed the file it opened, leaking a handle on every call. And the tests initially read the developer's own config.json, which would have made assertions depend on local ticker_aliases — a suite whose result varies by machine is not one. run_tests.py runs everything with one command and takes a filter while iterating. CI calls it instead of test_xirr.py alone. CONTRIBUTING now asks for pasted output of run_tests.py and check_clean.py, states which figure moved, and explains how to write a test against the fixture. The PR template collects that as structure rather than prose. docs/MAINTAINING.md records the other half: what to read in a PR and in what order, when to squash, and a release plan whose versioning promise is about the data rather than an API — a corrected figure is MAJOR even when the fix is plainly right, because someone's XIRR changing without warning is the thing to avoid. It also fixes the branch protection in writing: one ruleset, not the three overlapping layers that blocked merges, with an admin bypass that exists so a solo maintainer can merge their own work and is not for anyone else's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Follows the review in #2. Three things, all things that repo asked for and did not have.
1. A real test suite
The only tests covered the XIRR solver. Everything that has actually produced a wrong
number here — symbol normalisation, FIFO matching, cost basis, the Paytm parser — had
none.
80 unit tests across five modules,
unittestfrom the stdlib, no runner to install.test_markets.pytest_portfolio.pytest_analytics.pytest_ingest.pytest_paytm.pyNearly every test encodes a specific failure that reached a user and says so in a
comment — a trade id reused across two years, a split that made a steady holding look
re-entered,
\b20\d\d\breading2073out of a unit count, one SBI folio carrying threeschemes.
tests/fixtures.pygives each test its own SQLite file and its own empty config.Verified the suite leaves a 16,034-row database untouched.
Two bugs found while writing them:
config.load()never closed the file it opened — a leaked handle on every call.config.json, so a localticker_aliasesentry would change what they asserted. A suite whose result depends on who runs it is
not a suite.
2. Contributor evidence
CONTRIBUTING.mdnow asks for pasted output ofrun_tests.pyandcheck_clean.py,a statement of which figure moved and by how much, and a test that names the failure
it guards. The PR template collects it as structure.
3. Maintainer guide
docs/MAINTAINING.md— what to read in a PR and in what order, when to squash, and arelease plan whose versioning promise is about the data: a corrected figure is MAJOR
even when the fix is plainly right, because an XIRR changing without warning is the thing
to avoid.
It also records the branch protection now in place: one ruleset, replacing three
overlapping layers, with an admin bypass that exists so a solo maintainer can merge their
own work.
Evidence