sneak/rgoue
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases Wiki Activity

Unit test coverage: traps (all 8 trapHandlers entries untested) #14

New Issue
Closed
opened 2026-08-09 03:45:14 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:45:14 +02:00
Collaborator
Copy Link

Problem

The trapHandlers dispatch table has 8 entries and zero of them are
directly tested. Trap effects live in game/move.go:332-441.

Traps are one of the few subsystems that can kill the player outright
(teleport into a monster pack, dart poison, trapdoor fall), so a divergence
here is both easy to introduce and hard to notice.

For comparison, the other effect tables all have at least token coverage:

Table Entries Directly tested
quaffHandlers 14 2
readHandlers 18 2
zapHandlers 13 2
hitHandlers 8 1
daemonHandlers 13 ~3
trapHandlers 8 0

This unit was not part of the TODO.md "rings, sticks, wizard commands" Next
Step — it is an additional gap found while surveying, and it is arguably more
consequential than the wizard commands.

Definition of done

  1. A new game/traps_test.go (or additions to an existing move/trap test
    file) covering all 8 trap kinds, each asserting its characteristic
    effect:
    • trapdoor: level changes, new level generated
    • teleport: hero relocated to a valid square
    • dart: HP loss and/or strength effect
    • sleeping gas / bear trap: the corresponding freeze/stuck counter set
    • arrow, poison dart, rust — per C's actual effects
  2. springTrap discovery behavior: a previously-hidden trap becomes visible
    and is recorded on the level.
  3. The message text emitted for each trap matches C exactly.
  4. Death-safety: several traps can reduce HP to zero, and death calls
    os.Exit, which would kill the test binary. Use the existing
    fortify()-style HP-pinning pattern from the playtest tests.
  5. Fixed seeds for anything RNG-dependent.
  6. make check fully green.
  7. TODO.md updated in the same commit.
  8. Commit title ends with (closes #N).

Implementation requirements

  • Cross-check every effect and message against C
    (git show origin/c-master:move.c, the be_trapped switch). Do NOT check
    out or modify origin/c-master. Do not write tests that merely restate
    current Go behavior — the point is catching divergence from C.
  • If you find a real divergence, stop and report it; do not fix gameplay
    in a test-coverage commit, and do not encode the bug as expected behavior.
  • Tests need t.Parallel() and the //nolint:testpackage header.
  • make targets only. Do NOT modify .golangci.yml. No game behavior changes.
  • Never mention Claude or Anthropic anywhere.
## Problem The `trapHandlers` dispatch table has 8 entries and **zero** of them are directly tested. Trap effects live in `game/move.go:332-441`. Traps are one of the few subsystems that can kill the player outright (teleport into a monster pack, dart poison, trapdoor fall), so a divergence here is both easy to introduce and hard to notice. For comparison, the other effect tables all have at least token coverage: | Table | Entries | Directly tested | | --- | --- | --- | | `quaffHandlers` | 14 | 2 | | `readHandlers` | 18 | 2 | | `zapHandlers` | 13 | 2 | | `hitHandlers` | 8 | 1 | | `daemonHandlers` | 13 | ~3 | | **`trapHandlers`** | **8** | **0** | This unit was not part of the `TODO.md` "rings, sticks, wizard commands" Next Step — it is an additional gap found while surveying, and it is arguably more consequential than the wizard commands. ## Definition of done 1. A new `game/traps_test.go` (or additions to an existing move/trap test file) covering **all 8** trap kinds, each asserting its characteristic effect: - trapdoor: level changes, new level generated - teleport: hero relocated to a valid square - dart: HP loss and/or strength effect - sleeping gas / bear trap: the corresponding freeze/stuck counter set - arrow, poison dart, rust — per C's actual effects 2. `springTrap` discovery behavior: a previously-hidden trap becomes visible and is recorded on the level. 3. The message text emitted for each trap matches C exactly. 4. Death-safety: several traps can reduce HP to zero, and death calls `os.Exit`, which would kill the test binary. Use the existing `fortify()`-style HP-pinning pattern from the playtest tests. 5. Fixed seeds for anything RNG-dependent. 6. `make check` fully green. 7. `TODO.md` updated in the same commit. 8. Commit title ends with ` (closes #N)`. ## Implementation requirements - Cross-check every effect and message against C (`git show origin/c-master:move.c`, the `be_trapped` switch). Do NOT check out or modify `origin/c-master`. Do not write tests that merely restate current Go behavior — the point is catching divergence from C. - If you find a real divergence, **stop and report it**; do not fix gameplay in a test-coverage commit, and do not encode the bug as expected behavior. - Tests need `t.Parallel()` and the `//nolint:testpackage` header. - `make` targets only. Do NOT modify `.golangci.yml`. No game behavior changes. - Never mention Claude or Anthropic anywhere.
clawbot commented 2026-08-09 17:13:27 +02:00
Author
Collaborator
Copy Link

Implementation plan

Branch test/traps-coverage off main @ bf820e3. New white-box
game/traps_test.go; no game code touched.

First: a correction to the issue body's trap list

The definition-of-done paraphrases the trap set as "trapdoor, teleport,
dart, sleeping gas, bear trap, arrow, poison dart, rust". Read against
origin/c-master that is not the set. rogue.h 192-200 defines exactly
eight:

T_DOOR 00  T_ARROW 01  T_SLEEP 02  T_BEAR 03
T_TELEP 04 T_DART 05   T_RUST 06   T_MYST 07   NTRAPS 8

There is no separate "poison dart" trap — T_DART is the poison dart
(its death message is "a poisoned dart killed you") — and the list omits
T_MYST, the mystery trap, whose be_trapped arm is an eleven-way
rnd(11) message switch. The Go TrapKind iota (game/types.go
202-209) matches C index-for-index, and trapHandlers
(game/tables.go 835-844) is keyed by it. So the work covers T_MYST
in place of the phantom second dart. Flagging it here rather than
silently reinterpreting the issue.

What gets asserted, per C move.c be_trapped

Prologue, before the switch, tested on its own: on(player, ISLEVIT)
returns T_RUST with no side effects; otherwise running = FALSE,
count = FALSE, pp->p_ch = TRAP, pp->p_flags |= F_SEEN — that
last pair is the discovery behaviour in item 2 — and the return value is
the trap kind.

  • T_DOOR — level++, new_level(), "you fell into a trap!". Depth
    incremented and the map actually regenerated (snapshot of
    Level.Places + Stairs before/after).
  • T_BEAR — no_move += BEARTIME, "you are caught in a bear trap".
    rogue.h 108 is #define BEARTIME spread(3), so the delta is checked
    by replaying spread(3) from a snapshotted generator state, not by
    accepting a range.
  • T_SLEEP — no_command += SLEEPTIME (spread(5), same replay),
    player.t_flags &= ~ISRUN (Go Awake, same bit 0o020000), and the
    mist message.
  • T_ARROW — both arms of swing(pstats.s_lvl - 1, pstats.s_arm, 1),
    forced deterministically by picking level/AC so the roll cannot
    matter. Hit: HP down by a replayed roll(1,6), "oh no! An arrow shot
    you". Miss: an ARROW object exists on the floor at the hero, with
    o_count == 1, and "an arrow shoots past you".
  • T_TELEP — hero relocated to a walkable square, and the trap glyph
    written to the old square via mvaddch (the comment in C is the
    point of the case), plus teleport()'s own no_move = 0.
  • T_DART — both arms of swing(pstats.s_lvl + 1, ...). Miss: "a small
    dart whizzes by your ear and vanishes" and no HP change. Hit: HP down
    by a replayed roll(1,4), "a small dart just hit you in the
    shoulder", and the !ISWEARING(R_SUSTSTR) && !save(VS_POISON)
    gate on chg_str(-1) — including a sustain-strength ring pinning
    strength.
  • T_RUST — "a gush of water hits you on the head", then
    rust_armor(cur_armor): with no armor the gush is the last message
    and nothing changes; with rustable armor o_arm++.
  • T_MYST — the whole rnd(11) switch, checked by snapshotting the
    generator, running the trap, and recomputing C's expected string from
    the identical state (including the rainbow[rnd(cNCOLORS)] second
    draw in cases 1, 4, 6 and 10). That pins the message set, the case
    numbering, the rnd(11) bound, and the exact number of RNG draws each
    arm spends — which matters, because a stray draw desynchronises the
    seed-compatible stream.

Plus a table-completeness check that all eight TrapKind values have a
handler, so a new C trap arm cannot be silently dropped.

Practicalities

Message text is compared against constants transcribed from C, via
Msgs.Huh. Death paths (death('a'), death('d')) reach myExit →
os.Exit, so HP is pinned high in the fortify() style; the two death
messages are the one thing left unassertable in-process and will be
called out as such. Fixed seed throughout, t.Parallel(), the approved
//nolint:testpackage header, scripted input carries an abort tail
(space then ESCAPE) so a reprompt loop fails fast instead of hanging on
the headless terminal's filler input.

Every handler will be mutated away one at a time to prove the tests are
non-vacuous, and the results reported. TODO.md gets a Completed Steps
entry in the same commit; Next Step is not rotated, since this is
out-of-band issue work.

## Implementation plan Branch `test/traps-coverage` off `main` @ `bf820e3`. New white-box `game/traps_test.go`; no game code touched. ### First: a correction to the issue body's trap list The definition-of-done paraphrases the trap set as "trapdoor, teleport, dart, sleeping gas, bear trap, arrow, poison dart, rust". Read against `origin/c-master` that is not the set. `rogue.h` 192-200 defines exactly eight: T_DOOR 00 T_ARROW 01 T_SLEEP 02 T_BEAR 03 T_TELEP 04 T_DART 05 T_RUST 06 T_MYST 07 NTRAPS 8 There is no separate "poison dart" trap — `T_DART` is the poison dart (its death message is "a poisoned dart killed you") — and the list omits `T_MYST`, the mystery trap, whose `be_trapped` arm is an eleven-way `rnd(11)` message switch. The Go `TrapKind` iota (`game/types.go` 202-209) matches C index-for-index, and `trapHandlers` (`game/tables.go` 835-844) is keyed by it. So the work covers `T_MYST` in place of the phantom second dart. Flagging it here rather than silently reinterpreting the issue. ### What gets asserted, per C `move.c be_trapped` Prologue, before the switch, tested on its own: `on(player, ISLEVIT)` returns `T_RUST` with **no** side effects; otherwise `running = FALSE`, `count = FALSE`, `pp->p_ch = TRAP`, `pp->p_flags |= F_SEEN` — that last pair is the discovery behaviour in item 2 — and the return value is the trap kind. - `T_DOOR` — `level++`, `new_level()`, "you fell into a trap!". Depth incremented and the map actually regenerated (snapshot of `Level.Places` + `Stairs` before/after). - `T_BEAR` — `no_move += BEARTIME`, "you are caught in a bear trap". `rogue.h` 108 is `#define BEARTIME spread(3)`, so the delta is checked by replaying `spread(3)` from a snapshotted generator state, not by accepting a range. - `T_SLEEP` — `no_command += SLEEPTIME` (`spread(5)`, same replay), `player.t_flags &= ~ISRUN` (Go `Awake`, same bit `0o020000`), and the mist message. - `T_ARROW` — both arms of `swing(pstats.s_lvl - 1, pstats.s_arm, 1)`, forced deterministically by picking level/AC so the roll cannot matter. Hit: HP down by a replayed `roll(1,6)`, "oh no! An arrow shot you". Miss: an `ARROW` object exists on the floor at the hero, with `o_count == 1`, and "an arrow shoots past you". - `T_TELEP` — hero relocated to a walkable square, and the trap glyph written to the *old* square via `mvaddch` (the comment in C is the point of the case), plus `teleport()`'s own `no_move = 0`. - `T_DART` — both arms of `swing(pstats.s_lvl + 1, ...)`. Miss: "a small dart whizzes by your ear and vanishes" and no HP change. Hit: HP down by a replayed `roll(1,4)`, "a small dart just hit you in the shoulder", and the `!ISWEARING(R_SUSTSTR) && !save(VS_POISON)` gate on `chg_str(-1)` — including a sustain-strength ring pinning strength. - `T_RUST` — "a gush of water hits you on the head", then `rust_armor(cur_armor)`: with no armor the gush is the last message and nothing changes; with rustable armor `o_arm++`. - `T_MYST` — the whole `rnd(11)` switch, checked by snapshotting the generator, running the trap, and recomputing C's expected string from the identical state (including the `rainbow[rnd(cNCOLORS)]` second draw in cases 1, 4, 6 and 10). That pins the message set, the case numbering, the `rnd(11)` bound, and the exact number of RNG draws each arm spends — which matters, because a stray draw desynchronises the seed-compatible stream. Plus a table-completeness check that all eight `TrapKind` values have a handler, so a new C trap arm cannot be silently dropped. ### Practicalities Message text is compared against constants transcribed from C, via `Msgs.Huh`. Death paths (`death('a')`, `death('d')`) reach `myExit` → `os.Exit`, so HP is pinned high in the `fortify()` style; the two death messages are the one thing left unassertable in-process and will be called out as such. Fixed seed throughout, `t.Parallel()`, the approved `//nolint:testpackage` header, scripted input carries an abort tail (space then ESCAPE) so a reprompt loop fails fast instead of hanging on the headless terminal's filler input. Every handler will be mutated away one at a time to prove the tests are non-vacuous, and the results reported. `TODO.md` gets a Completed Steps entry in the same commit; `Next Step` is **not** rotated, since this is out-of-band issue work.
clawbot commented 2026-08-09 17:40:56 +02:00
Author
Collaborator
Copy Link

Done in #37 (branch
test/traps-coverage, rebased onto main at c0741ad).

New game/traps_test.go: 17 tests, 13 subtests, covering all eight arms
of move.c be_trapped, the prologue every trap runs through
(ISLEVIT early return; running/count reset; p_ch = TRAP and
p_flags |= F_SEEN, which is item 2 of the definition of done), and the
rust_armor tail T_RUST calls. No game code changed. Every expected
value is transcribed from origin/c-master and quoted in the file.

No divergence from C was found.

One correction to the issue body, raised before writing any code and
repeated here for the record: the eight-trap list in the definition of
done is wrong. rogue.h 192-200 has T_DOOR, T_ARROW, T_SLEEP,
T_BEAR, T_TELEP, T_DART, T_RUST, T_MYST. There is no separate
"poison dart" trap — T_DART is the poisoned dart — and the list
omitted T_MYST, the mystery trap, whose arm is an eleven-way rnd(11)
message switch. T_MYST is covered in place of the phantom second dart.

The per-trap table of C effect and message against what is asserted, the
full mutation log, and the uncovered items are all in the PR body. The
three things worth surfacing here:

  • Two mutations were not caught on the first pass and the tests were
    strengthened until they were. Deleting new_level() from T_DOOR left
    the suite green, because be_trapped's own prologue stamps the trap
    glyph into the cell the hero fell through, so "the map changed" is true
    with no new level dug; the test now counts differing cells. And
    roll(1,6) -> roll(1,5) on the arrow left it green, because rnd(n)
    is "raw value % n" — one draw agrees five times in six and leaves the
    generator identical — so the damage checks are sweeps now. 31 mutations
    total, each reverted; 29 failed their own test and only their own, all
    fast rather than by timeout.
  • The two death messages are deliberately uncovered. "an arrow killed
    you" and "a poisoned dart killed you" are each printed immediately
    before death(), which reaches myExit and os.Exit, so provoking
    either would take the test binary with it. The hero is pinned with
    fortify() and the damage is checked by replaying C's arithmetic
    rather than by letting HP reach zero.
  • T_TELEP's mvaddch(tc, TRAP) is not isolable, and that is a
    property of C rather than a port defect. Its comment claims look()
    will not redraw the vacated square, but the prologue has already set
    p_ch to TRAP and teleport() opens by drawing floor_at() — which
    returns chat(hero) — over the departing square, while leave_room
    only blanks squares showing FLOOR. The glyph is there before the line
    runs. Stated in the test rather than papered over.

Verification: make check fully green after the rebase, with
GOFLAGS=-count=1 so the run is not served from cache
(ok git.eeqj.de/sneak/rgoue/game 2.326s); golangci-lint 0 issues on a
private empty cache with no lock collision and no path outside the
worktree; .golangci.yml sha256 unchanged; nothing under
game/testdata/ touched and TestSeedCompatItemTables green. Package
coverage 56.2% -> 57.9% measured on the branch point bf820e3.

Done in https://git.eeqj.de/sneak/rgoue/pulls/37 (branch `test/traps-coverage`, rebased onto `main` at `c0741ad`). New `game/traps_test.go`: 17 tests, 13 subtests, covering all eight arms of `move.c be_trapped`, the prologue every trap runs through (`ISLEVIT` early return; `running`/`count` reset; `p_ch = TRAP` and `p_flags |= F_SEEN`, which is item 2 of the definition of done), and the `rust_armor` tail `T_RUST` calls. No game code changed. Every expected value is transcribed from `origin/c-master` and quoted in the file. **No divergence from C was found.** One correction to the issue body, raised before writing any code and repeated here for the record: the eight-trap list in the definition of done is wrong. `rogue.h` 192-200 has `T_DOOR`, `T_ARROW`, `T_SLEEP`, `T_BEAR`, `T_TELEP`, `T_DART`, `T_RUST`, `T_MYST`. There is no separate "poison dart" trap — `T_DART` **is** the poisoned dart — and the list omitted `T_MYST`, the mystery trap, whose arm is an eleven-way `rnd(11)` message switch. `T_MYST` is covered in place of the phantom second dart. The per-trap table of C effect and message against what is asserted, the full mutation log, and the uncovered items are all in the PR body. The three things worth surfacing here: - **Two mutations were not caught on the first pass** and the tests were strengthened until they were. Deleting `new_level()` from `T_DOOR` left the suite green, because `be_trapped`'s own prologue stamps the trap glyph into the cell the hero fell through, so "the map changed" is true with no new level dug; the test now counts differing cells. And `roll(1,6)` -> `roll(1,5)` on the arrow left it green, because `rnd(n)` is "raw value % n" — one draw agrees five times in six and leaves the generator identical — so the damage checks are sweeps now. 31 mutations total, each reverted; 29 failed their own test and only their own, all fast rather than by timeout. - **The two death messages are deliberately uncovered.** "an arrow killed you" and "a poisoned dart killed you" are each printed immediately before `death()`, which reaches `myExit` and `os.Exit`, so provoking either would take the test binary with it. The hero is pinned with `fortify()` and the damage is checked by replaying C's arithmetic rather than by letting HP reach zero. - **`T_TELEP`'s `mvaddch(tc, TRAP)` is not isolable**, and that is a property of C rather than a port defect. Its comment claims `look()` will not redraw the vacated square, but the prologue has already set `p_ch` to `TRAP` and `teleport()` opens by drawing `floor_at()` — which returns `chat(hero)` — over the departing square, while `leave_room` only blanks squares showing `FLOOR`. The glyph is there before the line runs. Stated in the test rather than papered over. Verification: `make check` fully green after the rebase, with `GOFLAGS=-count=1` so the run is not served from cache (`ok git.eeqj.de/sneak/rgoue/game 2.326s`); `golangci-lint` 0 issues on a private empty cache with no lock collision and no path outside the worktree; `.golangci.yml` sha256 unchanged; nothing under `game/testdata/` touched and `TestSeedCompatItemTables` green. Package coverage 56.2% -> 57.9% measured on the branch point `bf820e3`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed and gate is green; ready to merge
 needs-checks
make check is failing on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found problems; author must rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#14
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?