Skip to content

Make main.py importable and cap its render loop - #23

Open
dmccoystephenson wants to merge 2 commits into
mainfrom
feature/testable-main-entrypoint
Open

Make main.py importable and cap its render loop#23
dmccoystephenson wants to merge 2 commits into
mainfrom
feature/testable-main-entrypoint

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

Three defects in main.py are addressed together, because the first is the one that makes the other two testable.

  • main.py no longer runs at import time. The entry point is guarded with if __name__ == "__main__":, and the argument parsing and Viron service construction are moved out of module scope into parseArgs(argv) and into main()'s parameters. main(gridSize, exitAfterCreate, locationService, environmentService) defaults the two services to None and builds them against http://localhost:9999 when they are not supplied, so the command line behaviour is unchanged while a test can pass mocks instead. The cache-key expression is extracted as getEnvironmentKey(gridSize).
  • The render loop is capped. window.tick(targetFramesPerSecond) is called at the end of the loop body. RenderWindow.tick() had been exposed since PR Add RenderWindow class and use it for main.py's window and render loop #10 but was called from nowhere in the repository, so the loop re-drew and re-randomised every location as fast as the machine allowed.
  • The cached-environment progress message is flushed. pygame.display.update() is added after the "Loading existing environment, please wait..." text, so the message is presented before the blocking get_environment_by_id call rather than being erased by the render loop's first fill(). As noted in The "Loading existing environment" message is never displayed, because the cached path omits pygame.display.update() #21, the creation path's hardcoded 400, 400 text position is aligned to displayWidth/2, displayHeight/2 to match; the two are identical at the current display size.

Incidental to the restructure, the mis-indented try: body on the cached path (a 1-space indent) is normalised and three trailing-whitespace-only lines inside the rewritten function are cleaned.

tests/test_main.py is added — 23 tests covering argument parsing, the cache key, the environments.json contents, the --exit-after-create early return, the loading-message flush ordering, the error path, and the render loop's frame cap and location caching. The Viron submodule's service modules are replaced with sys.modules stubs, so no server is contacted and the suite also runs on Python versions below 3.9, where Viron's PEP 585 annotations are unimportable.

Test plan

  • python3 -m py_compile main.py graphik.py render_window.py tests/test_main.py — clean
  • python3 -m unittest discover -s tests32 tests executed, OK (9 pre-existing RenderWindow tests, 23 new)
  • Regression evidence gathered empirically, not by reasoning. With main.py reverted, tests/test_main.py fails to import at all (pygame.error: No available video device, raised from the module-level main() call) — which is the main.py runs at import time, so its render loop cannot be unit tested #18 defect itself. Reverting only the window.tick(...) line fails test_every_frame_is_capped_at_the_target_frame_rate with 0 != 3; reverting only the added pygame.display.update() fails test_loading_message_is_flushed_before_the_blocking_fetch on call ordering. Both pass once restored.
  • Headless end-to-end run against the real RenderWindow, real Graphik and real pygame under SDL_VIDEODRIVER=dummy, with the Viron services stubbed: --exit-after-create wrote the expected environments.json entry and exited, and the render loop was measured at 63 display updates over 1.08 s (~58 fps against the 60 cap).

Manual validation status

The project's live anchor — a Viron server on :9999 — is UNVERIFIED and could not be run in this environment: Docker is unavailable in this WSL distro, and the interpreter present is Python 3.8, on which Viron's own service modules raise TypeError: 'type' object is not subscriptable at import. The headless stub run described above and the new mocked unit tests are what stands in for it. Every code path changed here is covered by one or both, so this is recorded as UNVERIFIED-not-applicable rather than as a blocking gap, but confirmation against a live Viron by a reviewer with Docker would still be worth having before this is relied upon.

Issues deferred this cycle

Recorded here rather than as comments on each issue:

Closes #18
Closes #19
Closes #21

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

dmccoystephenson and others added 2 commits September 10, 2026 01:05
Guard main.py's entry point with `if __name__ == "__main__":` and move the
argument parsing and Viron service construction out of module scope, so the
module can be imported without running the program. `main()` now takes the
grid size, the --exit-after-create flag and the two services as arguments,
which lets tests drive it with mocks.

Call `RenderWindow.tick()` at the end of the render loop so it no longer spins
as fast as the machine can redraw, and flush the "Loading existing environment"
message with `pygame.display.update()` so it is actually presented before the
blocking fetch, matching the creation path.

Add tests/test_main.py covering argument parsing, the environments.json cache
key and file, the --exit-after-create early return, the loading-message flush
and the render loop's frame cap.

Closes #18
Closes #19
Closes #21

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tests section claimed only that Pygame is mocked. The suite also stubs
Viron's service modules, so it needs neither a populated submodule nor the
Python version those modules require.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric

Scored against the diff and against command output, not against judgement alone.

  • Scope: PASS (with a noted caveat) — the three files changed are main.py, tests/test_main.py and README.md, each required by one of the referenced issues. The caveat is that the restructure of main() also normalised the 1-space try: body indent on the cached path and removed three trailing-whitespace-only lines. Those lines are inside the function being rewritten rather than a tree-wide sweep, so they are disclosed here rather than split out.
  • Tests-new: PASS — every function introduced or given a new signature is exercised. parseArgs has five tests, getEnvironmentKey two, and main() fourteen across creation, cache reuse, the --exit-after-create early return, the error path and the render loop.
  • Tests-fix: PASS — confirmed empirically by stash-and-run, not by reasoning. Reverting main.py wholesale makes tests/test_main.py fail to import (pygame.error: No available video device, raised from the module-level main() call), which is main.py runs at import time, so its render loop cannot be unit tested #18 reproduced. Reverting only window.tick(targetFramesPerSecond) fails test_every_frame_is_capped_at_the_target_frame_rate with 0 != 3. Reverting only the added pygame.display.update() fails test_loading_message_is_flushed_before_the_blocking_fetch on call ordering. All three pass once restored.
  • Sibling structure: PASStests/test_main.py follows tests/test_render_window.py: stdlib unittest, MagicMock/patch, a setUp that registers patchers via addCleanup, and a unittest.main() footer.
  • Sibling renames: PASSexit_after_create became exitAfterCreate at both its definition and its single use, matching the camelCase already used for drawEnvironment, gridSize, numGrids and locationsCache in this module. The pre-existing snake_case locals inside main() (env_file, env_key, start_time) were deliberately left alone rather than swept up in this PR.
  • Docs: FAIL, then fixed — the README's "Running the tests" section stated only that Pygame is mocked, which is no longer the whole story now that the Viron service modules are stubbed in sys.modules. Corrected in e3bae72, which also notes that the command is expected to be run from the repository root. The remaining rows of the documentation table were re-checked against the source and hold: the grid-size argument still defaults to 50 and still falls back to 50 on unparseable input, --exit-after-create is still positional and still has no effect on a cached grid size (asserted by test_exit_after_create_is_ignored_for_a_cached_environment), and create_environments.bat still invokes python main.py <size> --exit-after-create, which the __main__ guard preserves exactly.
  • Issue resolution: PASSmain.py runs at import time, so its render loop cannot be unit tested #18's named surface area (module-level argv parsing, module-level service construction, the bare main() call, and the absent tests/test_main.py) is fully addressed. main.py's render loop is uncapped and RenderWindow.tick() is never called #19's window.tick() call is added; note that main.py's render loop is uncapped and RenderWindow.tick() is never called #19's second, separable suggestion — making the per-location colour stable instead of re-randomised each frame — is deliberately not implemented here, since that issue itself flags it as changing what the tool displays and as needing a decision first. main.py's render loop is uncapped and RenderWindow.tick() is never called #19 is nonetheless being closed on the frame-cap half, so a follow-up issue for the colour-stability question is being filed separately rather than left implicit. The "Loading existing environment" message is never displayed, because the cached path omits pygame.display.update() #21's flush is added, along with the text-centring alignment that issue proposes as part of the same change.
  • Manual validation: UNVERIFIED-not-applicable, with the substitute anchors PASS — the live anchor (a Viron server on :9999) cannot run in this environment: Docker is unavailable in this WSL distro, and under the Python 3.8 interpreter present Viron's service modules raise TypeError: 'type' object is not subscriptable on import. It is therefore not claimed green. What did run against the PR head: python3 -m py_compile main.py graphik.py render_window.py tests/test_main.py clean, and python3 -m unittest discover -s tests reporting 32 tests executed, OK — a real executed-test count, not a count-free banner. Beyond the mocked suite, a headless run under SDL_VIDEODRIVER=dummy drove the real RenderWindow, real Graphik and real pygame with only the Viron services stubbed: the --exit-after-create path wrote the expected environments.json entry and exited, and the render loop was measured at 63 display updates over 1.08 s (~58 fps against the 60 cap), which is direct evidence for main.py's render loop is uncapped and RenderWindow.tick() is never called #19 rather than an assertion about it. Since every changed path is covered by one of these, this is recorded as not-applicable rather than as a blocking gap.

Observations folded in from the diff

Offered as notes, not as blockers, and no change is being made for them in this PR.

  • tests/test_main.py:34installVironStubs() mutates the global sys.modules at test-module import time and never restores it. Nothing in the current suite is affected, but a future test module wanting the real Viron services on Python 3.10+ would silently receive the stub instead. A setUpModule/tearDownModule pair, or patch.dict(sys.modules, ...) scoped per test, would be the tighter form if that ever matters.
  • tests/test_main.py:96self.mockTime.time.side_effect = [1.0, 3.5] supplies exactly two clock readings, which is what the creation path consumes. A future test that walks a path calling time.time() a third time will hit StopIteration rather than a clear failure.
  • main.py:57getEnvironmentKey(gridSize) reads numGrids from module scope rather than taking it as a parameter, so the multi-grid keys contemplated by Support drawing environments with multiple grids #2 cannot be exercised without patching the module. This was left as-is because Support drawing environments with multiple grids #2 is the issue that would properly change it.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener). It is a self-review by the author of the change, not an independent one.


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson

Copy link
Copy Markdown
Member Author

Merge gate: held, and why

The pre-merge checks were run and the outcome is a deliberate hold rather than a merge, even though this session was pre-authorized to merge.

What passed.

  • Do-not-auto-merge path check: clean. The three changed paths are README.md, main.py and tests/test_main.py. Nothing under .github/workflows/, nothing under a security/ directory, .gitmodules untouched, and no file loses more than 50 lines (main.py is +89/-25).
  • Regression gate: satisfied empirically for all three referenced issues, by stash-and-run rather than by reasoning. The details are in the self-review above.
  • Test gate: python3 -m unittest discover -s tests reports 32 tests executed, OK, and python3 -m py_compile is clean.
  • Documentation sweep: completed, with one correction found and pushed as e3bae72.

What blocks the merge.

This project's external validation anchor is a manual run against a live Viron server on :9999, and that anchor cannot be run in this environment at all — not because of anything in this diff. Docker is unavailable in this WSL distro, and under the Python 3.8 interpreter present Viron's service modules raise TypeError: 'type' object is not subscriptable on import. The same failure reproduces on main, so it is a property of the environment rather than a verdict on this change. It has now been filed as #25.

That condition would ordinarily be recorded as not-applicable and waved through for a docs-only change. It is not being waved through here, because this PR does modify exactly what the anchor exists to check: the render loop and the Viron service call sites. The substitutes assembled instead are strong — a mocked suite covering every changed path, plus a headless run under SDL_VIDEODRIVER=dummy that drove the real RenderWindow, real Graphik and real pygame with only the network layer stubbed, measuring 63 display updates over 1.08 s against a 60 fps cap — but they are substitutes, and describing them as the anchor would overstate what was verified.

What is actually at residual risk. Narrower than the hold might suggest, and worth stating plainly so the decision is an easy one. The three service calls — create_environment("Test", numGrids, gridSize), get_environment_by_id(env_id) and get_locations_in_environment(...) — are textually unchanged; they are merely reached through a parameter now instead of a module-level global. The two behavioral additions, window.tick(...) and the extra pygame.display.update(), touch pygame only and were both exercised against real pygame. What remains unconfirmed is solely that a real Viron Environment and Location behave as the stubs do, which no line of this diff alters.

Requested action. A confirming run of python main.py 50 and python main.py 10 --exit-after-create against a live Viron on a machine with Docker, after which this can be merged. Landing #17 and #25 would remove this hold from every future change to this file.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant