Add UI tests for chess.py and fix four latent bugs they exposed#1
Open
BrandonZacharie wants to merge 7 commits into
Open
Add UI tests for chess.py and fix four latent bugs they exposed#1BrandonZacharie wants to merge 7 commits into
BrandonZacharie wants to merge 7 commits into
Conversation
Covers Menu pagination/navigation/display and Chess._fileprompt by mocking the curses Window plus the panel/doupdate/curs_set helpers, so the suite runs in milliseconds and does not require a terminal. Four bugs surfaced in review are pinned with xfail(strict=True) so the suite stays green today, but flips red the moment a fix lands and the marker needs removing: - Menu mutates the caller's items list (appends "exit" in place) - Menu.navigate clamps self.page to len(items)//page_size, which is one past the end when items align with page_size - Menu.display hides the panel inside the keypress loop instead of on exit - Chess._fileprompt feeds every unhandled keycode through chr(), so curses special keys leak into the path
Adds three test classes that round out the UI test file: - TestChessInit constructs Chess with ESC queued as the first keypress so the root-menu loop exits immediately, then asserts the root-menu entries, the presence of all sub-menus, and that cli.main was not invoked. - TestChessPlay drives Chess.play() on the same instance with cli.main patched out, pinning the "continue…" / "save file" insertions and verifying repeat calls don't duplicate entries. - TestLoadPgn runs _load_pgn against tests/test1.pgn and tests/test2.pgn (covering the partial-date branch in the menu label formatter) and a missing path, all via a _BareChess stand-in. Lifts _load_pgn onto _BareChess and switches the chess_instance fixture to yield inside the patch context so the mock outlives construction.
Menu.__init__ appended 'exit' to the list it received. Today the callers only pass throwaway literals, so the mutation is invisible — but the moment two Menus shared a list (or a caller kept a reference expecting the original entries) the side effect would surface as duplicate 'exit' rows or surprise list contents. Copy the list on the way in so the caller's reference is unaffected. Internal behavior is unchanged: self.items is still the working list, the appended 'exit' still lands at the end of it.
The forward branch clamped self.page to len(items)//page_size, which is one past the last valid page when len(items) is an exact multiple of page_size (e.g. 50 items / 25 per page → clamp said 2, but only pages 0 and 1 exist). Navigating off the end of the last page in that configuration set self.page out of bounds, and the next access to self.pages[self.page] would IndexError on the next render. Switching the clamp to len(self.pages) - 1 is mathematically the same in every other case but always lands on a valid index.
The catch-all match arm inserted chr(key) into the prompt for any keycode that wasn't named in an earlier case. For curses special keys (F1, KEY_RESIZE, KEY_MOUSE, …) that meant either a garbage character in the filename or a ValueError once the codepoint exceeded the Unicode range. Gate on key < 0x100 so every byte that worked before (including the high range used for UTF-8 continuation bytes) still inserts, and only the curses-specific KEY_* codes (which all live at 0o401+) are ignored.
The panel.hide() / panel.update_panels() / doupdate() block lived inside the keypress loop, so the panel was removed from the panel deck after every navigation key. Rendering still worked because the loop's window.refresh() bypasses the panel mechanism — but the deck state diverged from what was on screen, which would surface as wrong z-ordering when multiple Menus were layered (the file-prompt flow can stack one over another). Move only the panel hide / update / doupdate calls out of the loop; the per-iteration window.clear() that clears stale rendering stays where it is. As a bonus, the cleanup now also runs when display() exits via a callback's KeyboardInterrupt (which is how submenus unwind), where previously it was skipped.
Cosmetic only — no behavior change. Brings the file in line with the formatter pinned in pyproject.toml so the CI lint job passes.
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
2 participants
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.
Summary
Adds a UI test layer for
chess.py(the curses front end) and fixes four bugs the new tests pinned down. The tests came first; each fix is a separate commit that flips one strict-xfail to green so individual fixes can be reverted in isolation if anything looks off when exercised manually.The fixes are deliberately minimal — none of them change the visible behavior of paths that were already working, only the paths that crashed, corrupted input, or left the curses panel deck out of sync with the screen.
Tests
tests/test_chess_ui.py(37 tests, ~0.3 s). The cursesWindowand module-level helpers (panel,doupdate,curs_set) are replaced withunittest.mockfakes so the suite runs anywhere without a terminal and without re-parsing the heavy PGN fixtures.check_window_maxyxMenupagination &insertMenu.navigatelen(items) % page_size == 0edgeMenu.displayKeyboardInterrupt, panel.hide() call countConfigurationlog_styleChess._filepromptChess.__init__cli.mainnot called on ESCChess.playmainarguments, position resetChess._load_pgntest1.pgn,test2.pgn(covers the.??partial-date branch)Fixes
Each commit is a focused diff; the bug-fix commits are in increasing order of risk so a
git reverton the top commit alone can roll back the panel-lifecycle change while leaving the others.4a77e1f— StopMenufrom mutating the caller's items list.__init__appended"exit"to the list it received. Today every caller passes a throwaway literal, so the side effect is invisible — but twoMenus sharing a list would have produced duplicate"exit"rows. Fix:self.items = list(items).6e6a28d— ClampMenu.navigateto the actual last page index. The forward branch clampedself.pagetolen(items) // page_size, which is one past the last valid page whenlen(items)is an exact multiple ofpage_size(e.g. 50 items / 25 per page → clamp said 2, but only pages 0 and 1 exist). Switching tolen(self.pages) - 1is identical in every other case.60bdb59— Drop curses special keys in_fileprompt's default branch. The catch-all arm calledchr(key)for every unhandled keycode, so F-keys,KEY_RESIZE,KEY_MOUSE, etc. either inserted garbage into the filename or raisedValueErrorfor codes outside the Unicode range. Now gated onkey < 0x100, which preserves every byte 0–255 (including UTF-8 continuation bytes) and ignores only the cursesKEY_*constants (all at 0o401+).282a026— HideMenupanel once ondisplay()exit, not after every keypress.panel.hide() / panel.update_panels() / doupdate()lived inside the keypress loop, so the panel was removed from the panel deck after every navigation key. Rendering still worked becausewindow.refresh()bypasses the panel mechanism — but the deck state diverged from what was on screen, which would surface as wrong z-ordering once the file-prompt + nested-game-picker flow stacked panels. The per-iterationwindow.clear()that clears stale rendering stays where it is. As a bonus, cleanup now also runs whendisplay()exits via a callback'sKeyboardInterrupt(the way submenus unwind), where it was previously skipped.Test plan
coverage run -m pytest -xsvv— full suite (UI + engine) passes.python -m pytest tests/test_chess_ui.py— UI suite passes on its own in well under a second..pgn, pick a game from the nested menu, return to root.https://claude.ai/code/session_01RnQvxtXfn9JFTN8jpNyriL
Generated by Claude Code