Test suite, contributor evidence rules, and a maintainer guide

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,18 +1,43 @@
 ## What this changes
 
-<!-- and why it matters -->
+<!-- And why it mattered. If a number moves, say which one, from what to what. -->
+
+## Which figure changes, if any
+
+<!-- e.g. "PCBL XIRR: 6154% -> 5.0%, because its pre-rename purchases now attach" -->
+
+## Evidence
+
+<details>
+<summary><code>python3 run_tests.py</code></summary>
+
+```
+paste the output here
+```
+</details>
+
+<details>
+<summary><code>python3 scripts/check_clean.py</code></summary>
+
+```
+paste the output here
+```
+</details>
+
+## Checked by hand
+
+<!-- For UI changes: which screens, which market, what you clicked. -->
 
 ## Checklist
 
-- [ ] `python3 scripts/check_clean.py` says **clean** — no real broker data, anywhere
-- [ ] `python3 test_xirr.py` passes
-- [ ] Developed and tested against the demo (`./run.sh --demo`), not real holdings
-- [ ] No new dependency in `app/` or the repo root (stdlib only)
-- [ ] If this touches how a number is derived, I have said which figure changes and by
-      how much
+- [ ] `run_tests.py` passes and the output is pasted above
+- [ ] `check_clean.py` says **clean** — no real broker data anywhere
+- [ ] Developed against the demo (`./run.sh --demo`), not real holdings
+- [ ] No new dependency in `app/` or the repo root — standard library only
+- [ ] A test covers the behaviour I changed, and its comment says which failure it guards
 
 ## If this adds or changes a data importer
 
-- [ ] Validated against the source's own totals where it publishes them, and the import
-      refuses a file that does not reconcile rather than importing it partially
+- [ ] Validated against the source's own totals where it publishes them, and a file that
+      does not reconcile is skipped whole rather than imported partially
 - [ ] `docs/IMPORTING.md` updated

.github/workflows/ci.yml

@@ -32,8 +32,8 @@ jobs:
       - name: Compile everything
         run: python -m py_compile app/*.py *.py scripts/*.py samples/*.py tools/*.py
 
-      - name: Unit tests
-        run: python test_xirr.py
+      - name: Tests
+        run: python run_tests.py
 
       - name: Personal-data guard
         run: python scripts/check_clean.py

CONTRIBUTING.md

@@ -36,15 +36,68 @@ Rebuild it after a schema change: `python3 samples/build_demo.py`.
 
 ## Before you open a PR
 
+Run these and **paste the output of the first two into the pull request**. The template
+asks for it, and a PR without it goes back — not out of ceremony, but because "I ran the
+tests" and "here is what they said" are different claims.
+
 ```bash
-python3 test_xirr.py                                        # 16/16
-python3 -m py_compile app/*.py *.py scripts/*.py samples/*.py tools/*.py
+python3 run_tests.py                                        # every test
 python3 scripts/check_clean.py                              # must say "clean"
+python3 -m py_compile app/*.py *.py scripts/*.py samples/*.py tools/*.py
 ```
 
+`run_tests.py` takes a filter while you iterate — `python3 run_tests.py paytm` runs only
+`tests/test_paytm.py`. Run the whole suite before pushing.
+
 If you touched `app/static/index.html`, check the JavaScript parses. There is no build
 step and no bundler — it is one file, deliberately.
 
+```bash
+python3 -c "import re;open('/tmp/app.js','w').write(chr(10).join(re.findall(r'<script>(.*?)</script>',open('app/static/index.html').read(),re.S)))"
+node --check /tmp/app.js
+```
+
+### What a good pull request contains
+
+1. **What changed, and which number moved.** "Fixed XIRR" tells a reviewer nothing.
+   "CEMPRO read +91,882 at 3603% and now reads -51,475 at -45.8%, because its purchases
+   were filed under the pre-rename symbol" tells them everything.
+2. **Pasted output** of `run_tests.py` and `check_clean.py`.
+3. **A test for the thing you changed**, if behaviour changed.
+4. **How you checked it by hand** for UI changes — which screens, which market.
+
+### Writing tests
+
+`tests/` uses `unittest` from the standard library. `tests/fixtures.py` hands you a
+throwaway database:
+
+```python
+from tests.fixtures import TempDB, ago
+
+with TempDB() as db:
+    db.buy("AAPL", ago(400), 10, 100.0)
+    db.position("AAPL", 10, 150.0, avg_cost=100.0)
+    rows, ov = db.results("us")
+```
+
+Each test gets its own SQLite file in a temp directory and its own empty config, so
+nothing can reach `portfolio.db` and results do not depend on whose machine runs them.
+
+**Test the failure you are fixing, not the feature in general.** Nearly every test here
+encodes a specific bug that reached a user — a split that made a holding look re-entered,
+a trade id reused across two years, a regex that read "2073" out of a unit count. Each
+says so in a comment, so the next person knows why it cannot be tidied away. Yours
+should too.
+
+For anything touching money, assert the identity that must hold whatever you changed:
+
+```python
+self.assertAlmostEqual(ov["realized"] + ov["unrealized"] + ov["dividends"],
+                       ov["net_profit"], places=4)
+```
+
+That one assertion caught more errors during development than everything else combined.
+
 ## Adding a broker
 
 This is the most useful contribution anyone can make, and the codebase is shaped for it.

README.md

@@ -466,7 +466,8 @@ Security issues: [SECURITY.md](SECURITY.md) — please do not open a public issu
 | [Backups](docs/BACKUP.md) | snapshots, Drive, restore, scheduling |
 | [Research setup](docs/RESEARCH.md) | the optional Research tab |
 | [CLAUDE.md](CLAUDE.md) | how Claude Code should operate this repo — this is what makes the trigger words work, so it stays at the root |
-| [CONTRIBUTING.md](CONTRIBUTING.md) | the two hard rules, and how to add a broker |
+| [CONTRIBUTING.md](CONTRIBUTING.md) | the two hard rules, how to add a broker, how to write tests |
+| [docs/MAINTAINING.md](docs/MAINTAINING.md) | reviewing, merging and releasing — for maintainers |
 | [SECURITY.md](SECURITY.md) | what Trans does with your data, and how to report an issue |
 
 ## Cost policy

app/config.py

@@ -14,10 +14,10 @@ class ConfigError(RuntimeError): pass
 def load(reload=False):
     global _cache
     if _cache is not None and not reload: return _cache
-    if os.path.exists(PATH):
-        _cache = json.load(open(PATH))
-    elif os.path.exists(EXAMPLE):
-        _cache = json.load(open(EXAMPLE))
+    src = PATH if os.path.exists(PATH) else (EXAMPLE if os.path.exists(EXAMPLE) else None)
+    if src:
+        with open(src) as fh:          # was leaking the handle on every call
+            _cache = json.load(fh)
     else:
         _cache = {}
     return _cache

docs/MAINTAINING.md

@@ -0,0 +1,139 @@
+# Maintaining Trans
+
+For whoever holds the merge button. Written down so the decisions are consistent whether
+a PR arrives on a good week or a busy one.
+
+## Branch protection, and why it is set this way
+
+`main` is governed by **one** ruleset — deliberately one, because it was previously three
+overlapping layers and a single missed setting in any of them blocked everything.
+
+| Rule | Effect |
+|---|---|
+| `pull_request`, 1 approval | changes arrive by PR and need a review |
+| `required_status_checks` | CI must pass on 3.11, 3.12 and 3.13 |
+| `dismiss_stale_reviews_on_push` | a new commit invalidates an old approval |
+| `required_review_thread_resolution` | conversations must be resolved before merge |
+| `deletion`, `non_fast_forward` | `main` cannot be deleted or force-pushed |
+| **bypass: Admin, always** | the maintainer can merge without a second reviewer |
+
+The bypass exists because a solo maintainer cannot obtain an approval. It is not a licence
+to skip review — it is what makes a one-person project possible. **Use it for your own
+changes; never for someone else's.**
+
+## Reviewing a pull request
+
+Read in this order. Most PRs are decided by the first three.
+
+**1. Does it touch personal data?** `check_clean.py` runs in CI, but read the diff anyway:
+a screenshot with an account number, a test fixture built from a real tradebook, a
+`config.json` committed by accident. This is the only class of mistake that cannot be
+undone after a merge.
+
+**2. Does a number move, and does the PR say so?** Any change to `analytics.py`,
+`portfolio.py`, `ingest.py` or `xirr.py` can silently change what someone believes about
+their own money. The PR must state which figure changes and by how much. If it does not
+say, ask — do not work it out yourself and assume you got it right.
+
+**3. Is there a test, and does it name the failure it guards?** A test that asserts the
+new behaviour without saying which bug it prevents will be deleted by someone in a year
+who thinks it is redundant.
+
+**4. Does it hold the two hard rules?** No dependency in `app/` or the root; nothing that
+places, modifies or cancels a broker order.
+
+**5. Has the contributor pasted their test output?** Not because CI cannot be trusted, but
+because it shows they ran it before pushing rather than using CI as their test runner.
+
+### When to ask for changes rather than fixing it yourself
+
+Fix it yourself for typos and one-line adjustments. Ask for changes when the contributor
+needs to understand something — an importer without reconciliation, a test without a
+reason, a figure that moved unexplained. A repo about transparency should be reviewed
+transparently.
+
+### Merging
+
+**Squash** for most PRs: one logical change, one commit, message rewritten to say what
+changed and why it mattered. **Merge commit** only when the individual commits each stand
+alone and the history is worth keeping. Never rebase-merge onto `main` — it rewrites
+authorship dates and confuses the record.
+
+Delete the branch after merging. GitHub offers it; take it.
+
+## Releases
+
+There is no build and no package. A release is a **tag plus notes** saying what changed
+for someone who already has a clone.
+
+### Versioning
+
+`MAJOR.MINOR.PATCH`, where the promise is about **the data**, not the API:
+
+- **MAJOR** — the database schema changes in a way an existing `portfolio.db` cannot
+  simply be opened with, or a figure is redefined such that the same data now reports a
+  different number. Both need migration notes.
+- **MINOR** — a new broker, a new tab, a new analysis. Existing databases keep working.
+- **PATCH** — fixes and corrections that leave the schema and every definition alone.
+
+A corrected figure is **MAJOR** even when the fix is obviously right. If someone's XIRR
+reads differently after an upgrade, they must be told before it happens, not after they
+notice.
+
+### Cutting one
+
+```bash
+python3 run_tests.py                     # everything green
+python3 scripts/check_clean.py           # clean
+git checkout main && git pull
+
+# a clean checkout, the way a new user gets it
+git archive HEAD | tar -x -C /tmp/rel && cd /tmp/rel
+./run.sh --demo && ./run.sh 8799         # then click through both markets
+
+git tag -a v1.2.0 -m "…" && git push origin v1.2.0
+gh release create v1.2.0 --title "…" --notes-file notes.md
+```
+
+The clean-checkout step is not optional. `./run.sh --demo` shipped broken for two releases
+because everything was tested from a working directory that already had a database.
+
+### Release notes
+
+Write for someone deciding whether to upgrade today:
+
+- **Numbers that changed**, first and plainly — which figure, why, and roughly how much.
+  This is the section people actually need.
+- **New capability** — brokers, tabs, analyses.
+- **Fixes**, with the symptom rather than the cause: "a holding sold before it was bought
+  showed as pure profit" beats "fixed cost basis in results()".
+- **Anything requiring action** — re-import a file, run `ingest.py remap`, take a backup.
+
+Say what a release does **not** fix when it is likely to be asked. India dividends are
+still invisible; that belongs in the notes until it is not true.
+
+### Cadence
+
+Tag when something is worth telling people about, not on a schedule. A dormant month with
+no release is fine. A fix to a wrong figure is worth a release on its own, the same day.
+
+## Keeping the demo honest
+
+`samples/demo.db` is generated. After any schema change:
+
+```bash
+python3 samples/build_demo.py && python3 run_tests.py
+```
+
+It is deliberately untidy — a re-entered position, a renamed ticker, a fund with no order
+history, a demerged holding with no cost. Keep it that way. A demo where every number
+behaves teaches people to trust numbers that should be questioned.
+
+## What to say no to
+
+- **Advice, signals, recommendations, price targets.** Trans computes and displays.
+- **Anything that can place an order.** Read-only is a boundary, not a gap.
+- **Dependencies in `app/`.** The stdlib-only property is the whole install story.
+- **Telemetry**, of any kind, however anonymous.
+- **A figure without a definition.** If it cannot be explained in a tooltip, it does not
+  go in the header.

run_tests.py

@@ -0,0 +1,56 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: Apache-2.0
+"""Run everything. One command, stdlib only, no runner to install.
+
+    python3 run_tests.py            all of it
+    python3 run_tests.py markets    just tests/test_markets.py
+
+Prints a copy-pasteable summary, because CONTRIBUTING asks contributors to paste the
+result of this into their pull request.
+"""
+import os, sys, subprocess, unittest
+
+ROOT = os.path.dirname(os.path.abspath(__file__))
+
+
+def main():
+    which = sys.argv[1] if len(sys.argv) > 1 else None
+    failures = 0
+
+    # The XIRR solver has its own harness predating unittest; keep it, it is thorough.
+    if not which or which in ("xirr", "solver"):
+        print("=" * 62)
+        print("xirr solver")
+        print("=" * 62)
+        r = subprocess.run([sys.executable, os.path.join(ROOT, "test_xirr.py")],
+                           capture_output=True, text=True)
+        tail = [l for l in r.stdout.strip().split("\n") if l.strip()][-1:]
+        print(r.stdout if r.returncode else "\n".join(tail))
+        failures += r.returncode != 0
+
+    if which in ("xirr", "solver"):
+        return failures
+
+    print("=" * 62)
+    print("unit tests")
+    print("=" * 62)
+    loader = unittest.TestLoader()
+    if which:
+        suite = loader.loadTestsFromName(f"tests.test_{which}")
+    else:
+        suite = loader.discover(os.path.join(ROOT, "tests"), top_level_dir=ROOT)
+    result = unittest.TextTestRunner(verbosity=1).run(suite)
+    failures += (len(result.failures) + len(result.errors))
+
+    print()
+    print("=" * 62)
+    if failures:
+        print(f"FAILED — {failures} problem(s)")
+    else:
+        print(f"PASSED — {result.testsRun} unit tests, plus the XIRR solver suite")
+    print("=" * 62)
+    return 1 if failures else 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())