diff options
| author | Scott Gasch <[email protected]> | 2026-10-15 13:11:22 -0700 |
|---|---|---|
| committer | Scott Gasch <[email protected]> | 2026-10-15 13:11:22 -0700 |
| commit | 993b6eeb2a9ac7b659c473b6883fa53af1ad3a0b (patch) | |
| tree | c5388824dec15a7691ce9952a6eb4af66e837127 /src/docs/TESTING_PLAN.md | |
| parent | 53b380c658f828c4c0e096be2763e6b012c23abd (diff) | |
Phase A NNUE: feature-indexing spec, annotation/training pipeline, dump nnuefeatures
Builds out the NNUE Phase A prototype per docs/NNUE.md:
- src/nnuefeatures.c: standalone HalfKAv2_hm feature-index reference
implementation (new `dump nnuefeatures` command), sourced from
Stockfish's half_ka_v2_hm.{h,cpp} (sf_16 tag), including the 0x88->64
square conversion typhoon's own board representation needs.
- annotate_pgns/: sharded TWIC/self-play annotation harness
(annotate.py), position extraction, quiet-position filtering (flat
and game-shaped, plus an upstream --prefilter-quiet option),
train/holdout game-level stratified split, WDL-sigmoid K fitting,
the Python HalfKAv2_hm reference encoder (nnue_feature_spec.py) and
its bit-for-bit cross-checker against the C side
(cross_check_features.py), and a first PyTorch training pipeline
(nnue_model.py, nnue_train.py, build_training_tensors.py) -- a
256x2-32-32-1-ish net trained locally on CPU against ~1.85M quiet
TWIC positions as a pipeline/framework validation pass.
- Finish an in-progress InitMagic/InitLMRTable/InitEval ->
InitializeBitboardMagic/InitializeLMRTable/InitializeEval rename:
chess.h's prototypes hadn't been updated to match the already-renamed
definitions/call sites, leaving 4 implicit-function-declaration
warnings.
- tests/arasan.epd: new curated EPD suite.
- docs/: add the project's design docs (previously untracked).
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Diffstat (limited to 'src/docs/TESTING_PLAN.md')
| -rw-r--r-- | src/docs/TESTING_PLAN.md | 298 |
1 files changed, 298 insertions, 0 deletions
diff --git a/src/docs/TESTING_PLAN.md b/src/docs/TESTING_PLAN.md new file mode 100644 index 0000000..7d2bfc2 --- /dev/null +++ b/src/docs/TESTING_PLAN.md @@ -0,0 +1,298 @@ +# Testing plan: closing coverage gaps in test*.c + +Written 2026-09-16 after an audit of every `src/test*.c` file and how +(or whether) each is wired into `main.c`'s self-test sequence. Goal +per Scott: not more test code for its own sake -- find the +years-old bugs that asserts have been missing. Priorities: search, +eval, generate (not exclusively). Perf testing is also explicitly +in scope. + +## Dead/disconnected infrastructure (fix first -- near-zero cost) + +Several tests already exist but don't run. Recovering this coverage is +strictly cheaper than writing anything new, so do this before anything +else below. + +1. **SEE has zero active correctness testing.** `testsee.c`'s own + header comment admits it: `DebugSEE`/`DebugSEERecursive` (a + brute-force make/unmake exchange-walk oracle meant to check `SEE()` + against) is wrapped in `#ifdef TEST_BROKEN` -- a define nothing + sets. `testsee.c`'s only *called* functions (`TestGetAttacks`, + `TestIsAttackedBB`) test attack generation, not SEE. Given SEE is + what `GetMoveSee`/extensions/QSearch pruning all trust (and is the + exact code reworked in commit c0ac3d7), this is the single + highest-value fix here. Get `TEST_BROKEN` -> live (`TEST`), fix + whatever bitrot surfaces, and confirm it's called from `main.c`. +2. **`TestEvalWithKnownPositions`** (`testeval.c`, ~20 hand-curated + "side X should be better here" positions from chess literature) is + defined but **never called from anywhere** -- not `main.c`, not + `command.c`. Wire it into the self-test sequence. +3. **`TestRecognExhaustiveKNKP`** (`testrecogn.c`, exhaustive KNKP + endgame-recognizer enumeration) is defined and prototyped in + `chess.h` but referenced only in a comment -- never invoked. Only + the smaller table-driven `TestRecogn` runs. Wire it in. +4. **The hash table has no correctness self-test at all.** + `testhash.c` contains `SanityCheckHashEntry` (an entry-shape + validator) and `AnalyzeFullHashTable` -- but the latter is a + post-search *diagnostic dump* called from `root.c`, not a test, and + it's never invoked from the `TEST` self-test sequence. See "New + test: hash correctness" below for what should replace this gap, not + just a wiring fix. +5. **`TestEvalWithSymmetry`** only runs a *single* random symmetric + position per invocation, and only when a human types `eval` at the + prompt in an `EVAL_DUMP` build (`command.c:2203`) -- not part of the + automated suite, never loops. See "New test: eval symmetry fuzz" + below. + +## New tests, by area + +### Search (thinnest area relative to its bug surface) + +- `TestSearch` (`testsearch.c`) runs 20 random positions at fixed + **depth 2** only, checking solely "returned move is legal" and + "didn't blow the time limit." + - **DONE -- hash-consistency test** (`TestSearchHashConsistency`, + testsearch.c): searches a position to depth D twice in a row with + a warm hash table (scores must match), and separately confirms a + depth-D2 search seeded by a shallower depth-D1 warm-up agrees with + a from-scratch depth-D2 search. Wired into the `TEST=1` self-test + sequence (main.c). + - **DONE (partially) -- QSearch sanity test** + (`TestQSearchSanity`, testsearch.c): checks search termination + (no `INVALID_SCORE`/garbage) and a whole-search node-count + ceiling. Caveat found and documented in the function's own header + comment: the self-test suite builds with plain `gmake TEST=1`, + which doesn't define `PERF_COUNTERS` -- every `sCounters` field + (including `u64QNodeCount`, the qsearch-only counter this was + originally meant to check) is wrapped in `chess.h`'s `INC()` macro, + a no-op without that define. Confirmed live (`sd 3; go` on a + `TEST`-only binary reports "0 qnodes" regardless of real qsearch + activity). The test currently checks + `ctx->sCounters.tree.u64TotalNodeCount` instead (root.c's + unconditional per-node `++`, always live) -- a real but strictly + weaker signal, since it can't distinguish "qsearch exploded" from + "full-width search exploded." Building a `PERF_COUNTERS`-enabled + `TEST` profile (or adding an always-live qsearch-only counter) is + the natural follow-up to close this for real. The hand-auditable- + tree idea (`Trace()` every qsearch node on a small position) is + still worth doing but wasn't implemented this session. + - **NOT DONE -- determinism test** (same position/depth searched + twice, PV must match): explicitly descoped -- Scott's call is that + a stable-PV requirement makes the test break every time move + ordering changes for unrelated reasons, which is just noise, not a + real regression signal. A same-*score* (not same-PV) check is + already covered by the hash-consistency test above. + - **NOT DONE -- MP-vs-single-thread agreement test**: still open. + `split.c` has no dedicated correctness test. + - **DONE (as an EPD suite, not a C test) -- null-move/zugzwang + soundness**: `tests/zugzwang.epd` (5 positions, run via the new + `epd` command -- see below) checks actual best-move outcome + instead of an internal counter. An earlier attempt asserted + against `ctx->sCounters.extension.uZugzwang` (the verification- + search hit counter) using the 3 classic literature zugzwang + positions, but none of them structurally reach that code path on + this build: 2 have too little material for + `WeShouldTryNullmovePruning`'s own `> 2` non-pawn-count gate to + attempt null-move pruning at all, and the 3rd (despite landing in + the eligible window) gets resolved directly off this box's local + Syzygy tablebase before search ever touches null-move logic -- + confirmed live via the `TABLEBASE MOVE` trace line. Removed in + favor of the EPD suite, which doesn't care about internal + mechanism, only the final answer. See `testsearch.c`'s comment + where `TestNullMoveZugzwang` used to be for the full writeup. + +**New infrastructure that came out of chasing the above (search.c +touches none of this, but it's now how any future EPD-based search +test should be added):** + +- **`epd <file>` engine command** (`EpdCommand`, script.c) parses and + runs a raw EPD suite file directly -- no perl preprocessing step + first. Backed by `ParseEpdRecord` (fen.c), real per-record EPD + parsing (4 position fields, optional FEN-style halfmove/fullmove + counters, then `;`-separated `bm`/`am`/`id` opcodes). +- **Found and fixed while building this**: `tests/convert_epd.pl` (the + old perl-based EPD-to-script converter) had a real, silent- + corruption bug -- its `bm`/`am` regex used `[^b]+` as the filler + between the board and the opcode, which breaks on *every* + Black-to-move record (the side-to-move field is literally the + character `b`), and since `$fen`/`$id`/`$solution` were unscoped + globals rather than per-line locals, a failed match silently carried + the *previous* record's values forward instead of erroring. + Confirmed live: feeding it a 5-position batch with one Black-to-move + entry produced a byte-for-byte duplicate of the previous record for + that one line, no diagnostic at all. The script is left in place + (marked superseded in its own header) in case something still shells + out to it, but new suites should use `epd` instead. + Cross-checked the new parser against the old pipeline on + `ecm_ringers`: `script` (old `.ep_` format) and `epd` (new + `tests/ecm_ringers.epd`, produced by `ep_to_epd.pl`, see next item) + produced byte-identical node counts and solve stats. +- **`tests/ep_to_epd.pl`**: one-off reverse converter, turns the + existing `setboard`/`solution`/`avoid`/`id`/`go` script-format + suites back into real EPD now that `epd` can read it directly. Run + against every genuine tactical suite in `tests/` (not `bench.ep_` or + `trial.ep_`, which use `sd`/`st`/`easy`/`quit` and aren't EPD + underneath) -- produced `.epd` siblings for `arasan`, + `ecm_confident_quick`, `ecm_confident_sample2`, `ecm_confident`, + `ecm_hard_quick`, `ecm_hard`, `ecm_late`, `ecm_quick`, `ecm_ringers`, + `ecm-gcp`, `ecm-hard`, `ecm`, `ecm016_only`, `LCT2`, `mes400`, + `newiq`, `quick`, `swiss`, `wac`. `ecm.epd` came out to 879 records, + matching the `606/879`/`622/879` solve-count baselines already + quoted elsewhere in CLAUDE.md. + +### Eval + +- Automate `TestEvalWithSymmetry` into a real fuzz loop (N random + symmetric positions, assert `Eval() == 0` for all, as part of the + standard self-test run) instead of the current single-shot + manual-only invocation. +- **Adversarial/degenerate-position term-bounds test**: 0-pawn + positions, 9-queen positions, fully-open-board positions -- extend + `GenerateRandomLegalPosition` or hand-build cases, assert no eval + term produces an out-of-range score (reuse the existing + `EvalTrace`/`EVAL_DUMP` term-tracing machinery for this). +- Audit for any **incrementally-maintained eval state** (pawn hash, + king-safety cache) and add a from-scratch-recompute-vs-incremental + consistency check if any exists -- classic "works until the hash + collides" bug class. +- **Eval speed regression test**: no benchmark currently exists for + eval itself despite it being the named NPS bottleneck vs. Crafty + (see CLAUDE.md). Same interleaved-comparison shape as the existing + `TestGenerate*Speed` benchmarks, broken down by game phase. + +### Generate (best-covered area, but gaps remain) + +- Perft (`TestMoveGenerator`) only checks to **ply 4** even though + `uLeaves[]` already has reference values to ply 6/7 sitting unused -- + extend to a `--perft-deep` slow path, and add perft-**divide** + (per-root-move leaf breakdown) to localize failures to a specific + move instead of just tripping a panic. +- Add perft positions specifically chosen to stress **en passant, + underpromotion, and castling-through-check** individually (the + current 5 FENs give general coverage but weren't chosen for this). +- `TestGenerateAllMovesSpeed` and the per-piece-type speed tests are + **pure microbenchmarks with no correctness assertion** -- they print + cycles and never diff the move *sets* produced by the mailbox vs. + bitboard generators being compared. Add a sorted-move-list-equality + assertion alongside each (both generators already run back-to-back + in the same loop, so this is nearly free) -- otherwise a bitboard + generator silently missing a move class just prints a suspiciously + fast number and passes. +- `GetMoveSee`'s new non-pawn-capture SEE path (c0ac3d7) has no + dedicated test yet -- covered by the SEE oracle fix above once live. + +### Move/Position plumbing + +- Reasonably solid already (`TestMakeUnmakeMove`, `TestExposesCheck`, + `TestIsAttacked`). Add a **long-game fuzz test**: play N random games + of ~200 plies each (not just 1-ply-at-a-time exercises), asserting + `VerifyPositionConsistency` and `ComputeSig`-from-scratch agreement + with the incrementally-maintained signature at every ply. Confirm + `VerifyPositionConsistency` is actually compiled live in the + `TEST`/`DEBUG` build, not just present in source. + +### Hash (currently zero correctness coverage) + +New test needed, not just a wiring fix: +- Store/probe roundtrip correctness. +- Replacement-scheme behavior under forced collisions (fill table past + capacity with distinct signatures, confirm replacement policy + behaves as designed, not just "doesn't crash"). +- `ADJUST_MATE_IN_N` ply-distance adjustment on `HASH_FLAG_EXACT` + entries specifically -- the exact mechanism CLAUDE.md flags as + audited-by-hand but not test-covered after this session's `ASSERT` + removals in `QSearch()`. + +### Recognizer + +- Wire in `TestRecognExhaustiveKNKP` (see dead-infra list above). + Consider the same exhaustive-enumeration treatment for any other + small recognized endgame class in `recogn.c` (KPK, KRK) if one + exists without a test. + +## Perf testing + +Current coverage is narrow: five isolated per-piece-type +mailbox-vs-bitboard microbenchmarks (`TestGenerate*Speed`), nothing for +search, eval, or hash. + +- **Eval NPS/cycles-per-call benchmark**, same interleaved-comparison + shape as the existing generate benchmarks, by game phase -- eval is + the named bottleneck (CLAUDE.md's 1.5M vs 7.5M nps gap vs. Crafty) + and has no dedicated speed test today. +- **Whole-search NPS benchmark**: `bench.c` already exists for this -- + confirm whether it's part of the automated `TEST` self-test pass or + purely a manual `bench` command; if manual-only, wire a + fixed-node-budget version into the automated sequence so a + regression surfaces without someone remembering to run it by hand. +- **Hash-probe-under-load benchmark**: cycles/probe at low occupancy + vs. near-full table -- replacement-scheme cost can regress silently + today with nothing to measure it. +- **Stored baselines with pass/fail thresholds**, not print-only + output: today every perf test prints cycles/call and leaves + interpretation to a human reading the log. Store a last-known-good + baseline (same idea as `head_reference/` for solve-counts) and fail + loud if a call site regresses past some tolerance. + +## Suggested priority order + +1. Un-bitrot the three dead tests (SEE oracle, + `TestEvalWithKnownPositions`, `TestRecognExhaustiveKNKP`) -- + near-zero cost, immediate coverage recovery. +2. Hash-table correctness test (store/probe/replace/mate-adjust) -- + currently the single biggest correctness blind spot. +3. Search determinism + MP-vs-single-thread agreement test -- targets + the exact class of bug most likely to hide in threading/hash/ + extension interactions that per-function unit tests can't see. +4. Extend perft to deeper plies + divide output; add move-set-equality + assertions to the existing generate speed benchmarks (turns + dead-weight microbenchmarks into correctness tests for free). +5. Eval: automate the symmetry fuzz test (loop N, not 1) and add + adversarial/degenerate-position term-bounds checks. +6. Round out perf testing per the list above, with stored baselines + and pass/fail thresholds instead of print-only output. + +## "Fuzz" testing + +**Implemented: `fuzz_test.sh [binary] [depth] [batch_size] [cpus]`** +(defaults: `./_typhoon`, `sd 6`, 200 positions/batch, `--cpus 2`). Loops +forever: each iteration draws a fresh random batch of `setboard`/`go` +records from the full 400,000-line / 200k-position +`tests/cross_sectional_200k.ep_` pool, runs them through a `DEBUG=1` +build under gdb, and scans the gdb output for a crash/assert signature +before starting the next batch. Stops (exit 1) and prints the offending +batch + engine log paths as soon as one is found; otherwise runs +indefinitely (`^C` to stop). Logs land under `/usr/local/tmp/typhoon/fuzz/` +(one sampled `.ep_` batch + one `.gdb.txt` + one engine `--logfile` per +iteration). + +**Two build/invocation bugs found and fixed getting this working, neither +a sandbox limitation despite first appearances:** + +1. **The DEBUG binary must be built with `MP=1` as well as `DEBUG=1`** + (`gmake clean; gmake DEBUG=1 MP=1`) -- `--cpus` is compiled in only + under `#ifdef MP` (`main.c:311`). A `DEBUG=1`-only build silently + treats `--cpus` and its numeric argument as two separate unrecognized + arguments (`main.c:400`'s "Error (unknown argument)" path) and falls + back to the 1-cpu default with no hard failure -- easy to miss, and it + also means `split.c`'s parallel-search code never actually links in, + defeating the point of passing `--cpus 2` at all. +2. **`gdb -batch -ex "handle SIGTRAP nostop noprint pass" -ex run ...` + breaks thread control against this MP/pthread build specifically**, + producing `ptrace (PT_SUSPEND): Operation not permitted` followed by + `Child process unexpectedly missing: No child processes` -- looked + exactly like a sandboxed environment blocking ptrace outright (and was + initially misdiagnosed as that), but reproduced identically outside + any harness/sandbox, and disappeared entirely once that one `-ex` line + was dropped. `fuzz_test.sh` omits it; a plain `run`/`bt full`/ + `info registers` gdb batch invocation debugs the MP binary fine, + including catching a genuine `int3` breakpoint from a failed `ASSERT`. + +Not yet done: no crash has been found by a run so far, so the +crash-detection path (the `grep -qE` in `fuzz_test.sh`) is exercised by +its own logic reading correctly, not yet by a real hit. Worth periodically +running this for stretches (it's unbounded/`^C`-driven, unlike the other +gates) rather than treating it as a one-time pass/fail check like +`debug_smoke_test.sh`. See also: `./debug_smoke_test.sh` (fast, curated +sample, single pass/fail) for the pre-commit-sized version of the same +idea. |
