From 61a7fdda9119a7fc2e5168fef08f5abbe8de0838 Mon Sep 17 00:00:00 2001 From: Michael Denyer Date: Mon, 27 Jul 2026 00:40:33 +0100 Subject: [PATCH] refactor: route writer-handle patching in tests through one helper 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. --- pyrefly-baseline.json | 276 ------------------------------- tests/test_incremental_writer.py | 66 +++++--- 2 files changed, 39 insertions(+), 303 deletions(-) diff --git a/pyrefly-baseline.json b/pyrefly-baseline.json index dc4a188..e00ce2c 100644 --- a/pyrefly-baseline.json +++ b/pyrefly-baseline.json @@ -516,282 +516,6 @@ "concise_description": "Expected a callable, got `Self@TestFilteringProperties`", "severity": "error" }, - { - "line": 138, - "column": 30, - "stop_line": 138, - "stop_column": 48, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `flush`", - "concise_description": "Object of class `NoneType` has no attribute `flush`", - "severity": "error" - }, - { - "line": 146, - "column": 13, - "stop_line": 146, - "stop_column": 31, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `flush`", - "concise_description": "Object of class `NoneType` has no attribute `flush`", - "severity": "error" - }, - { - "line": 211, - "column": 30, - "stop_line": 211, - "stop_column": 48, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 222, - "column": 13, - "stop_line": 222, - "stop_column": 31, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 241, - "column": 38, - "stop_line": 241, - "stop_column": 56, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 248, - "column": 21, - "stop_line": 248, - "stop_column": 39, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 280, - "column": 38, - "stop_line": 280, - "stop_column": 56, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 287, - "column": 21, - "stop_line": 287, - "stop_column": 39, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 348, - "column": 30, - "stop_line": 348, - "stop_column": 48, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 358, - "column": 13, - "stop_line": 358, - "stop_column": 31, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 393, - "column": 30, - "stop_line": 393, - "stop_column": 48, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 404, - "column": 13, - "stop_line": 404, - "stop_column": 31, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 430, - "column": 38, - "stop_line": 430, - "stop_column": 56, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 439, - "column": 21, - "stop_line": 439, - "stop_column": 39, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 473, - "column": 21, - "stop_line": 473, - "stop_column": 39, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 525, - "column": 17, - "stop_line": 525, - "stop_column": 35, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `flush`", - "concise_description": "Object of class `NoneType` has no attribute `flush`", - "severity": "error" - }, - { - "line": 541, - "column": 34, - "stop_line": 541, - "stop_column": 52, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `close`", - "concise_description": "Object of class `NoneType` has no attribute `close`", - "severity": "error" - }, - { - "line": 547, - "column": 17, - "stop_line": 547, - "stop_column": 35, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `close`", - "concise_description": "Object of class `NoneType` has no attribute `close`", - "severity": "error" - }, - { - "line": 571, - "column": 30, - "stop_line": 571, - "stop_column": 48, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `flush`", - "concise_description": "Object of class `NoneType` has no attribute `flush`", - "severity": "error" - }, - { - "line": 586, - "column": 13, - "stop_line": 586, - "stop_column": 31, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `flush`", - "concise_description": "Object of class `NoneType` has no attribute `flush`", - "severity": "error" - }, - { - "line": 612, - "column": 38, - "stop_line": 612, - "stop_column": 56, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 622, - "column": 21, - "stop_line": 622, - "stop_column": 39, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `write`", - "concise_description": "Object of class `NoneType` has no attribute `write`", - "severity": "error" - }, - { - "line": 628, - "column": 21, - "stop_line": 628, - "stop_column": 38, - "path": "tests/test_incremental_writer.py", - "code": -2, - "name": "missing-attribute", - "description": "Object of class `NoneType` has no attribute `seek`", - "concise_description": "Object of class `NoneType` has no attribute `seek`", - "severity": "error" - }, { "line": 606, "column": 32, diff --git a/tests/test_incremental_writer.py b/tests/test_incremental_writer.py index 275af86..9e6f12d 100644 --- a/tests/test_incremental_writer.py +++ b/tests/test_incremental_writer.py @@ -1,6 +1,7 @@ """Tests for incremental association result writer.""" import errno +from io import TextIOWrapper from pathlib import Path from unittest.mock import patch @@ -10,6 +11,19 @@ from jamma.lmm.stats import AssocResult +def open_handle(writer: IncrementalAssocWriter) -> TextIOWrapper: + """Return an open writer's file handle, for injecting I/O failures. + + The tests below drive the retry and rollback paths by replacing methods on + the live handle, which means reaching past the public API. ``_file`` is + None until ``__enter__`` runs, so going through here asserts the writer is + open once instead of at every call site. + """ + handle = writer._file + assert handle is not None, "writer must be open before patching its handle" + return handle + + @pytest.fixture def sample_result() -> AssocResult: """Create a sample AssocResult for testing.""" @@ -135,7 +149,7 @@ def test_write_batch_single_flush(self, tmp_path: Path, sample_results: list): output_path = tmp_path / "test.assoc.txt" with IncrementalAssocWriter(output_path) as writer: - original_flush = writer._file.flush + original_flush = open_handle(writer).flush flush_count = 0 def counting_flush(): @@ -143,7 +157,7 @@ def counting_flush(): flush_count += 1 return original_flush() - writer._file.flush = counting_flush + open_handle(writer).flush = counting_flush writer.write_batch(sample_results) # Count flushes only during write_batch, not __exit__ batch_flush_count = flush_count @@ -208,7 +222,7 @@ def test_retries_on_oserror(self, tmp_path: Path, sample_result: AssocResult): call_count = 0 with IncrementalAssocWriter(output_path) as writer: - original_write = writer._file.write + original_write = open_handle(writer).write def flaky_write(data): nonlocal call_count @@ -219,7 +233,7 @@ def flaky_write(data): raise OSError("Disk full") return original_write(data) - writer._file.write = flaky_write + open_handle(writer).write = flaky_write with patch("jamma.lmm.io.time.sleep") as mock_sleep: writer.write(sample_result) @@ -238,14 +252,14 @@ def test_deletes_partial_on_final_failure( with pytest.raises(OSError): with IncrementalAssocWriter(output_path) as writer: # Make every write after header fail - original_write = writer._file.write + original_write = open_handle(writer).write def always_fail(data): if "\t" in data: # data lines have tabs raise OSError("Disk full") return original_write(data) - writer._file.write = always_fail + open_handle(writer).write = always_fail writer.write(sample_result) # Partial file should be cleaned up @@ -277,14 +291,14 @@ def test_cleanup_on_context_exit_with_error( with patch("jamma.lmm.io.time.sleep"): with pytest.raises(OSError): with IncrementalAssocWriter(output_path) as writer: - original_write = writer._file.write + original_write = open_handle(writer).write def always_fail(data): if "\t" in data: raise OSError("Disk full") return original_write(data) - writer._file.write = always_fail + open_handle(writer).write = always_fail writer.write(sample_result) # File should be cleaned up by __exit__ @@ -345,7 +359,7 @@ def test_write_batch_retries_on_flaky_write( call_count = 0 with IncrementalAssocWriter(output_path) as writer: - original_write = writer._file.write + original_write = open_handle(writer).write def flaky_write(data): nonlocal call_count @@ -355,7 +369,7 @@ def flaky_write(data): raise OSError("Transient I/O error") return original_write(data) - writer._file.write = flaky_write + open_handle(writer).write = flaky_write with patch("jamma.lmm.io.time.sleep") as mock_sleep: writer.write_batch(sample_results) @@ -390,7 +404,7 @@ def test_write_retries_on_transient_eio( call_count = 0 with IncrementalAssocWriter(output_path) as writer: - original_write = writer._file.write + original_write = open_handle(writer).write def eio_write(data): nonlocal call_count @@ -401,7 +415,7 @@ def eio_write(data): raise err return original_write(data) - writer._file.write = eio_write + open_handle(writer).write = eio_write with patch("jamma.lmm.io.time.sleep") as mock_sleep: writer.write(sample_result) @@ -427,7 +441,7 @@ def test_write_fails_immediately_on_eacces( with patch("jamma.lmm.io.time.sleep") as mock_sleep: with pytest.raises(OSError): with IncrementalAssocWriter(output_path) as writer: - original_write = writer._file.write + original_write = open_handle(writer).write def eacces_write(data): if "\t" in data: @@ -436,7 +450,7 @@ def eacces_write(data): raise err return original_write(data) - writer._file.write = eacces_write + open_handle(writer).write = eacces_write writer.write(sample_result) # No retry sleeps should have occurred @@ -470,7 +484,7 @@ def fail_after_first(data): write_call_count += 1 raise OSError("Disk full") - writer._file.write = fail_after_first + open_handle(writer).write = fail_after_first # This should fail after retries and clean up writer.write(sample_results[1]) @@ -522,7 +536,7 @@ def test_flush_failure_on_normal_exit_raises( def failing_flush(): raise OSError("Disk full") - writer._file.flush = failing_flush + open_handle(writer).flush = failing_flush # Partial file should be cleaned up assert not output_path.exists(), ( @@ -538,13 +552,13 @@ def test_keyboard_interrupt_with_close_failure( with pytest.raises(KeyboardInterrupt): with IncrementalAssocWriter(output_path) as writer: writer.write(sample_result) - original_close = writer._file.close + original_close = open_handle(writer).close def failing_close(): original_close() raise OSError("Stale NFS file handle") - writer._file.close = failing_close + open_handle(writer).close = failing_close raise KeyboardInterrupt() @pytest.mark.tier0 @@ -568,7 +582,7 @@ def test_write_buf_retries_enospc_and_succeeds( failed_once = False with IncrementalAssocWriter(output_path) as writer: - original_flush = writer._file.flush + original_flush = open_handle(writer).flush def flaky_flush(): nonlocal flush_call_count, failed_once @@ -583,7 +597,7 @@ def flaky_flush(): raise err return original_flush() - writer._file.flush = flaky_flush + open_handle(writer).flush = flaky_flush with patch("jamma.lmm.io.time.sleep") as mock_sleep: writer.write(sample_result) @@ -609,7 +623,7 @@ def test_write_buf_deletes_partial_on_non_retryable_rollback_failure( with patch("jamma.lmm.io.time.sleep"): with pytest.raises(OSError): with IncrementalAssocWriter(output_path) as writer: - original_write = writer._file.write + original_write = open_handle(writer).write def eperm_write(data): # Only fail on data lines (which contain tabs), not the header @@ -619,13 +633,13 @@ def eperm_write(data): raise err return original_write(data) - writer._file.write = eperm_write + open_handle(writer).write = eperm_write # Also make seek fail to trigger _cleanup_partial - def failing_seek(pos): + def failing_seek(cookie: int, whence: int = 0) -> int: raise OSError("Seek failed") - writer._file.seek = failing_seek + open_handle(writer).seek = failing_seek writer.write(sample_result) @@ -657,9 +671,7 @@ def failing_tell(): err.errno = errno.ESPIPE raise err - handle = writer._file - assert handle is not None - handle.tell = failing_tell + open_handle(writer).tell = failing_tell writer.write(sample_result)