refactor: route writer-handle patching in tests through one helper#127
Merged
Conversation
Clears the largest remaining pyrefly cluster: all 23 entries in tests/test_incremental_writer.py (100 -> 77). They were one root cause, not 23 problems. `IncrementalAssocWriter._file` is None until `__enter__` runs, so every test that drives the retry and rollback paths by replacing a method on the live handle -- `writer._file.write = flaky_write` and friends -- indexed into an optional. Adds `open_handle(writer)`, which asserts the writer is open once and returns the narrowed handle, and rewrites the 23 sites to use it. Typed as the concrete `io.TextIOWrapper` rather than `typing.TextIO`: the latter declares `__slots__`, so assigning to `.write` on it is itself an error, which would have traded 23 entries for 14. `failing_seek` also grew the real `seek(cookie, whence=0)` signature; the one-parameter stub was not assignable. Test-only, and a pure expression rewrite: `open_handle(writer)` returns the identical object the tests patched before, verified by identity, and a patch applied through it is still visible as `writer._file.write` and still runs on the write path. 2242 passed, 10 skipped, 3 xfailed -- unchanged. Checked that the suite still bites rather than passing vacuously, by mutating `_write_buf`: dropping the retry loop fails 4 tests, dropping the `_count` increment fails 8. That sweep did surface a real pre-existing gap, left alone here because closing it is a test-coverage change rather than a burn-down: deleting the rollback `handle.seek(pos)` or `handle.truncate()` fails nothing. The rollback's *failure* is covered (test_write_buf_deletes_partial_on_non_retryable_rollback_failure); its success -- that a failed write is actually truncated back and the file left consistent -- is not. Baseline regenerated on Python 3.11 / numpy 2.4.6 per #121; `pyrefly check --baseline` reports 0 errors on both 3.11.13/2.4.6 and 3.13.5/2.5.1.
michael-denyer
added a commit
that referenced
this pull request
Jul 26, 2026
Closes the gap #127 found by mutation testing: deleting `handle.seek(pos)` or `handle.truncate()` from `_write_buf` failed no test. The rollback's *failure* was covered by test_write_buf_deletes_partial_on_non_retryable_rollback_failure; its success -- that a failed attempt is discarded and the retry writes over clean state -- was not, so the two lines that guarantee a consistent output file were free to disappear. The new test makes the first attempt put more bytes into the stream than the retry will, then fail with a retryable errno, and asserts on the resulting file rather than on the call sequence. Both halves of the rollback are load-bearing under that setup: - Without `seek(pos)`, `truncate()` cuts at the position left after the debris, so the retried line lands after it. - Without `truncate()`, `seek(pos)` rewinds but the retried line is shorter than the debris, so the debris tail survives past the end of the line. Either way the writer reports success over a malformed file, which is why the assertion is on content. Confirmed to bite: with `handle.truncate()` stubbed out, 1 failed; with `handle.seek(pos)` stubbed out, 1 failed; with both, 1 failed. Each fails on this test's own assertion ("Rollback must discard the failed attempt's bytes"), not incidentally elsewhere. Before this test all three mutants survived. Test-only. `pytest tests/` -- 2243 passed, 10 skipped, 3 xfailed. Baseline unchanged at 77 entries; this adds none.
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
1 participant
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.
Clears the largest remaining pyrefly cluster. 100 → 77 entries.
All 23 entries in
tests/test_incremental_writer.pywere one root cause, not 23problems.
IncrementalAssocWriter._fileis None until__enter__runs, so every test thatdrives the retry and rollback paths by replacing a method on the live handle indexes into an
optional:
Adds
open_handle(writer), which asserts the writer is open once and returns the narrowedhandle, and rewrites the 23 sites through it.
Two details that shaped the fix:
io.TextIOWrapper, nottyping.TextIO.TextIOdeclares__slots__, so assigning to.writeon it is itself an error — that annotation would havetraded 23 entries for 14. Found by trying it.
failing_seekgrew the realseek(cookie, whence=0)signature; the one-parameter stub wasnot assignable to
TextIOWrapper.seek.Verification
Test-only, and a pure expression rewrite — but "the tests still pass" is a weak claim for a
change that edits the very lines doing the fault injection, so it was checked directly.
The helper returns the same object the tests patched before:
The suite still bites — mutating
_write_bufand re-running:self._count += countincrementpytest tests/test_incremental_writer.pypytest tests/ruff check/ruff format --checkpyrefly check --baselineon 3.11.13/numpy 2.4.6 and 3.13.5/numpy 2.5.1A gap that sweep exposed — not fixed here
Deleting the rollback
handle.seek(pos)orhandle.truncate()from_write_buffailsnothing. The rollback's failure is covered
(
test_write_buf_deletes_partial_on_non_retryable_rollback_failure); its success — that afailed write is actually truncated back and the file left consistent — is not.
Left alone deliberately: closing it is a test-coverage change, not a burn-down, and it wants
its own PR.
Running total
174 (#121) → 152 (#122) → 135 (#123) → 113 (#124) → 107 (#125) → 100 (#126) → 77.
A 56% reduction. Largest remaining clusters are all small now:
tests/test_jlinalg_dgemm.py(8),
tests/test_kinship_io.py(7),tests/test_runner_numpy.py(5),scripts/bench_all_backends.py(4),src/jamma/lmm/runner_numpy_streaming.py(4).src/holds 18 of the 77.