{{ message }}
Repository navigation
Conversation
Member
Author
Code reviewFound 3 issues:
tmuxp/tests/workspace/test_freezer.py Lines 33 to 37 in f30986a
|
tony
force-pushed
the
pytest-improvements
branch
from
June 28, 2026 14:56
f30986a to
8a39075
Compare
Member
Author
Code review (re-review after force-push)The three findings from the earlier review are resolved: the no-op Found 1 issue:
tmuxp/tests/workspace/test_finders_local.py Lines 268 to 276 in 8a39075 |
why: The pytest-optimizer tooling writes durable profiling and benchmark JSON under .pytest-optimizer/; it is local state, not source, and should never be committed. what: - Add .pytest-optimizer/ to .gitignore
why: test_freeze_config and test_freeze_logs_debug each slept a fixed time.sleep(0.50) to let tmux settle after build() before freeze() — ~1.0s of dead wait per run. what: - Replace both sleeps with retry_until polling on len(session.windows) >= len(session_config["windows"]) - Import retry_until (libtmux), drop now-unused import time - Suite 39.69s -> 38.53s (-1.16s), above the 0.68s k*MAD noise band
why: the suite is ~88% call-bound across independent, tmux-driven test bodies (each test spawns its own daemon on a unique socket), so it parallelizes cleanly. Measured: serial 39.69s -> ~16-19s under capped xdist (best 13.7s) ~= 2.1-2.9x, far above the 0.68s noise band; fully green under -n 8 in clean CI conditions. what: - Add pytest-xdist to the dev and testing dependency groups - Add a `just test-parallel` recipe with a capped worker count (-n 8); `just test` stays serial - Keep -n out of addopts: -n auto (=cores) thrashes at full core saturation (~40s) and -n breaks --pdb/-x ergonomics
why: CLI color is decided live from FORCE_COLOR/NO_COLOR (see tmuxp._internal.colors.Colors). A contributor who exports FORCE_COLOR in their shell gets ANSI-wrapped help text, so test_help_examples.py fails (len(examples) == 0). The same leak made the suite look order-dependent under xdist when FORCE_COLOR reached the workers. what: - Add autouse _pin_test_color_env clearing FORCE_COLOR/NO_COLOR for every test; color-specific tests still set them explicitly and monkeypatch restores the contributor's values at teardown - Suite now green with FORCE_COLOR exported and under xdist regardless of ambient color env
why: CLAUDE.md mandates functional tests only ("avoid class TestFoo
groupings"). test_finders_local.py had two class-based groupings,
the only such violation in the suite.
what:
- Flatten TestFindLocalWorkspaceEdgeCases (7 methods) and
TestLocalWorkspaceFilesConstant (2 methods) into module-level
def test_* functions; drop the self receiver
- Rename the constant tests to test_local_workspace_files_constant_*
so they read at module scope without the class for context
- No behavior change: same tests collected, assertions verbatim
why: without --strict-markers an unregistered or mistyped marker (e.g. @pytest.mark.flakey) is a silent warning, so a typo'd mark quietly stops applying. Strict mode turns it into a hard collection error. what: - Add --strict-markers to [tool.pytest.ini_options] addopts - No markers ini entry needed: the only custom marker (flaky) is registered by pytest-rerunfailures; there are no project-defined custom markers - Verified: suite green and an unregistered mark now errors at collection
why: WorkspaceBuilder.build() already calls _wait_for_pane_ready() for each normal pane, so the session is fully materialized when build() returns. The post-build retry_until on window count was a no-op (windows register synchronously) and its comment overstated what it did. what: - Remove the retry_until window-count waits from test_freeze_config and test_freeze_logs_debug, plus the now-unused retry_until import - build() flows straight to freeze(); the existing freezer tests remain the regression coverage
why: test_at_filesystem_root requested tmp_path and monkeypatch but used neither — it exercises traversal against the real filesystem root and patches nothing. what: - Remove the unused tmp_path/monkeypatch parameters from the test signature
why: the comment said "should find both" above an assertion that tolerates more than two results — when traversal continues past home it can also pick up configs in ancestor directories. what: - Reword the comment to match the >= 2 assertion
why: test_local_workspace_files_constant_is_list only asserted the constant is a list of length 3 — both already proven by the sibling test_local_workspace_files_constant_order via its equality assertion against the list literal. what: - Remove the subsumed is_list test
This branch has not been deployed
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
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
pytest-xdistand ajust test-parallelrecipe, so the largely-independent, tmux-driven suite can fan out across cores for a much faster local run while the defaultjust teststays serial.time.sleep(0.50)settle waits in the freezer tests —WorkspaceBuilder.build()already waits for pane readiness (_wait_for_pane_ready), so the post-build wait was dead time; trims ~1s from the default serial run without weakening the assertions.FORCE_COLOR/NO_COLORfor every test, so the CLI help-text assertions are hermetic regardless of a contributor's exported shell color settings (and stay green under xdist).--strict-markersso a mistyped or unregistered marker becomes a hard collection error instead of a silent warning.Changes by area
Parallelism (opt-in)
pyproject.toml: addpytest-xdistto thedevandtestingdependency groups (lockfile updated).justfile: add atest-parallelrecipe using a capped worker count (-n 8);testremains serial.Test robustness & speed
tests/workspace/test_freezer.py: drop the fixedtime.sleep(0.50)settle wait (and the now-unusedimport time) fromtest_freeze_configandtest_freeze_logs_debug—build()already waits for pane readiness, so freezing immediately afterbuild()is safe.conftest.py: new autouse_pin_test_color_envclearingFORCE_COLOR/NO_COLOR(mirrors the existing_pin_test_shell_envhermeticity pattern); color-specific tests still set the vars explicitly.Test hygiene & convention
pyproject.toml: append--strict-markersto[tool.pytest.ini_options]addopts.tests/workspace/test_finders_local.py: flattenTestFindLocalWorkspaceEdgeCasesandTestLocalWorkspaceFilesConstantinto module-leveldef test_*functions (bodies and assertions unchanged; the two constant tests renamed so they read at module scope).Tooling
.gitignore: ignore.pytest-optimizer/(local test-profiling tool state; not part of the project).Design decisions
-n auto: on a high-core machine-n autosaturates every core and regresses below serial (each worker re-imports and re-collects the whole suite). A fixed-n 8sits in the sweet spot.-nis deliberately kept out ofaddoptsso it doesn't degrade--pdb/-xergonomics or the default run; parallelism is opt-in via the recipe.WorkspaceBuilder.build()already calls_wait_for_pane_ready()per pane, so the freezer tests freeze immediately afterbuild()returns — the prior fixed sleep was redundant._pin_test_shell_envpins$SHELL, pinning the color env once (autouse) makes every test independent of the contributor's ambient environment rather than patching each color-sensitive test.markersini entry: the only non-builtin marker (flaky) is owned and registered bypytest-rerunfailures(a mandatory dependency); the project defines no custom markers of its own, so--strict-markersalone is complete.Verification
No fixed sleeps remain in the freezer tests:
$ rg 'time\.sleep' tests/workspace/test_freezer.pyNo class-based groupings remain in the finders tests:
$ rg '^class Test' tests/workspace/test_finders_local.pyStrict-markers is active:
$ rg 'strict-markers' pyproject.tomlTest plan
uv run py.testjust test-parallel(uv run py.test -n 8)FORCE_COLORexported — proves the color-env hermeticity fix--strict-markersis enforced viaaddoptstests/workspace/test_finders_local.pycollects the same set after flattening (no class prefixes)uv run ruff check .anduv run ruff format .uv run mypy(strict)