diff --git a/changelog/11418.improvement.rst b/changelog/11418.improvement.rst new file mode 100644 index 00000000000..6517ba422fd --- /dev/null +++ b/changelog/11418.improvement.rst @@ -0,0 +1,11 @@ +The cached rewritten modules in ``__pycache__`` are now invalidated by a hash of +the source instead of its modification time, using the checked-hash pyc format +from :pep:`552`. + +Timestamps make the cache useless wherever source files get fresh modification +times, most notably in CI: a checkout gives every file a new mtime, so a +restored cache is discarded in full and every test module is rewritten again. +Hashing the source costs roughly 15 microseconds per file and makes the cache +survive that. + +Existing timestamp based caches are ignored and rewritten once. diff --git a/changelog/13292.bugfix.rst b/changelog/13292.bugfix.rst new file mode 100644 index 00000000000..279541e7661 --- /dev/null +++ b/changelog/13292.bugfix.rst @@ -0,0 +1,3 @@ +Fixed a stale rewritten module being used when a test file is modified twice +within the same second without changing its size -- the pyc header could only +record whole-second timestamps. The cache is now keyed on a hash of the source. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index dcc37df2c98..10abbe743af 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -19,7 +19,6 @@ import os from pathlib import Path from pathlib import PurePath -import struct import sys import tokenize import types @@ -176,11 +175,11 @@ def exec_module(self, module: types.ModuleType) -> None: co = _read_pyc(fn, pyc, state.trace) if co is None: state.trace(f"rewriting {fn!r}") - source_stat, co = _rewrite_test(fn, self.config) + source_hash, co = _rewrite_test(fn, self.config) if write: self._writing_pyc = True try: - _write_pyc(state, co, source_stat, pyc) + _write_pyc(state, co, source_hash, pyc) finally: self._writing_pyc = False else: @@ -297,34 +296,28 @@ def get_resource_reader(self, name: str) -> TraversableResources: return FileReader(types.SimpleNamespace(path=self._rewritten_names[name])) # type: ignore[arg-type] -def _write_pyc_fp( - fp: IO[bytes], source_stat: os.stat_result, co: types.CodeType -) -> None: +def _write_pyc_fp(fp: IO[bytes], source_hash: bytes, co: types.CodeType) -> None: # Technically, we don't have to have the same pyc format as # (C)Python, since these "pycs" should never be seen by builtin # import. However, there's little reason to deviate. fp.write(importlib.util.MAGIC_NUMBER) - # https://www.python.org/dev/peps/pep-0552/ - flags = b"\x00\x00\x00\x00" - fp.write(flags) - # as of now, bytecode header expects 32-bit numbers for size and mtime (#4903) - mtime = int(source_stat.st_mtime) & 0xFFFFFFFF - size = source_stat.st_size & 0xFFFFFFFF - # " bool: proc_pyc = f"{pyc}.{os.getpid()}" try: with open(proc_pyc, "wb") as fp: - _write_pyc_fp(fp, source_stat, co) + _write_pyc_fp(fp, source_hash, co) except OSError as e: state.trace(f"error writing pyc file at {proc_pyc}: errno={e.errno}") return False @@ -340,15 +333,15 @@ def _write_pyc( return True -def _rewrite_test(fn: Path, config: Config) -> tuple[os.stat_result, types.CodeType]: - """Read and rewrite *fn* and return the code object.""" - stat = os.stat(fn) +def _rewrite_test(fn: Path, config: Config) -> tuple[bytes, types.CodeType]: + """Read and rewrite *fn* and return its source hash and code object.""" source = fn.read_bytes() + source_hash = importlib.util.source_hash(source) strfn = str(fn) tree = ast.parse(source, filename=strfn) rewrite_asserts(tree, source, strfn, config) co = compile(tree, strfn, "exec", dont_inherit=True) - return stat, co + return source_hash, co def _read_pyc( @@ -364,9 +357,6 @@ def _read_pyc( return None with fp: try: - stat_result = os.stat(source) - mtime = int(stat_result.st_mtime) - size = stat_result.st_size data = fp.read(16) except OSError as e: trace(f"_read_pyc({source}): OSError {e}") @@ -378,16 +368,16 @@ def _read_pyc( if data[:4] != importlib.util.MAGIC_NUMBER: trace(f"_read_pyc({source}): invalid pyc (bad magic number)") return None - if data[4:8] != b"\x00\x00\x00\x00": + if data[4:8] != b"\x03\x00\x00\x00": trace(f"_read_pyc({source}): invalid pyc (unsupported flags)") return None - mtime_data = data[8:12] - if int.from_bytes(mtime_data, "little") != mtime & 0xFFFFFFFF: - trace(f"_read_pyc({source}): out of date") + try: + source_hash = importlib.util.source_hash(source.read_bytes()) + except OSError as e: + trace(f"_read_pyc({source}): OSError {e}") return None - size_data = data[12:16] - if int.from_bytes(size_data, "little") != size & 0xFFFFFFFF: - trace(f"_read_pyc({source}): invalid pyc (incorrect size)") + if source_hash[:8] != data[8:16]: + trace(f"_read_pyc({source}): out of date") return None try: co = marshal.load(fp) diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 9740bf3c05e..12e12449693 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -9,6 +9,7 @@ from functools import partial import glob import importlib +from importlib.util import source_hash import inspect import marshal import os @@ -1324,13 +1325,14 @@ def test_write_pyc(self, pytester: Pytester, tmp_path) -> None: config = pytester.parseconfig() state = AssertionState(config, "rewrite") tmp_path.joinpath("source.py").touch() - source_path = str(tmp_path) + source_bytes = tmp_path.joinpath("source.py").read_bytes() pycpath = tmp_path.joinpath("pyc") co = compile("1", "f.py", "single") - assert _write_pyc(state, co, os.stat(source_path), pycpath) + hash = source_hash(source_bytes) + assert _write_pyc(state, co, hash, pycpath) with mock.patch.object(os, "replace", side_effect=OSError): - assert not _write_pyc(state, co, os.stat(source_path), pycpath) + assert not _write_pyc(state, co, hash, pycpath) def test_resources_provider_for_loader(self, pytester: Pytester) -> None: """ @@ -1403,8 +1405,37 @@ def test_read_pyc_success(self, tmp_path: Path, pytester: Pytester) -> None: fn.write_text("def test(): assert True", encoding="utf-8") - source_stat, co = _rewrite_test(fn, config) - _write_pyc(state, co, source_stat, pyc) + hash, co = _rewrite_test(fn, config) + _write_pyc(state, co, hash, pyc) + assert _read_pyc(fn, pyc, state.trace) is not None + + pyc_bytes = pyc.read_bytes() + assert pyc_bytes[4] == 3 # checked-hash flag set + assert pyc_bytes[8:16] == hash[:8] + + def test_read_pyc_ignores_mtime(self, tmp_path: Path, pytester: Pytester) -> None: + """A pyc stays valid when only the mtime of the source changes. + + This is what makes the cache survive a fresh checkout or a restored + CI cache, where every source file gets a new mtime. + """ + from _pytest.assertion import AssertionState + from _pytest.assertion.rewrite import _read_pyc + from _pytest.assertion.rewrite import _rewrite_test + from _pytest.assertion.rewrite import _write_pyc + + config = pytester.parseconfig() + state = AssertionState(config, "rewrite") + + fn = tmp_path / "source.py" + pyc = Path(str(fn) + "c") + fn.write_text("def test(): assert True", encoding="utf-8") + + hash, co = _rewrite_test(fn, config) + _write_pyc(state, co, hash, pyc) + + new_mtime = os.stat(fn).st_mtime + 3600 + os.utime(fn, (new_mtime, new_mtime)) assert _read_pyc(fn, pyc, state.trace) is not None def test_read_pyc_more_invalid(self, tmp_path: Path) -> None: @@ -1417,40 +1448,71 @@ def test_read_pyc_more_invalid(self, tmp_path: Path) -> None: source.write_bytes(source_bytes) magic = importlib.util.MAGIC_NUMBER - - flags = b"\x00\x00\x00\x00" - - mtime = b"\x58\x3c\xb0\x5f" - mtime_int = int.from_bytes(mtime, "little") - os.utime(source, (mtime_int, mtime_int)) - - size = len(source_bytes).to_bytes(4, "little") - + flags = b"\x03\x00\x00\x00" + hash = source_hash(source_bytes)[:8] code = marshal.dumps(compile(source_bytes, str(source), "exec")) # Good header. - pyc.write_bytes(magic + flags + mtime + size + code) + pyc.write_bytes(magic + flags + hash + code) assert _read_pyc(source, pyc, print) is not None # Too short. - pyc.write_bytes(magic + flags + mtime) + pyc.write_bytes(magic + flags + hash[:4]) assert _read_pyc(source, pyc, print) is None # Bad magic. - pyc.write_bytes(b"\x12\x34\x56\x78" + flags + mtime + size + code) + pyc.write_bytes(b"\x12\x34\x56\x78" + flags + hash + code) assert _read_pyc(source, pyc, print) is None - # Unsupported flags. - pyc.write_bytes(magic + b"\x00\xff\x00\x00" + mtime + size + code) + # Unsupported flags -- including the timestamp based pycs written by + # pytest<9.3 and by CPython itself. + for bad_flags in ( + b"\x00\x00\x00\x00", + b"\x01\x00\x00\x00", + b"\x00\xff\x00\x00", + ): + pyc.write_bytes(magic + bad_flags + hash + code) + assert _read_pyc(source, pyc, print) is None + + # Bad hash. + pyc.write_bytes(magic + flags + b"\x00" * 8 + code) assert _read_pyc(source, pyc, print) is None - # Bad mtime. - pyc.write_bytes(magic + flags + b"\x58\x3d\xb0\x5f" + size + code) + # Missing source. + pyc.write_bytes(magic + flags + hash + code) + source.unlink() assert _read_pyc(source, pyc, print) is None - # Bad size. - pyc.write_bytes(magic + flags + mtime + b"\x99\x00\x00\x00" + code) - assert _read_pyc(source, pyc, print) is None + def test_rewrite_picks_up_edit_within_one_mtime_second( + self, pytester: Pytester + ) -> None: + """Regression test for #13292. + + The pyc header can only hold a whole-second timestamp, so a file + edited twice within the same second used to be served from a stale + pyc. Hashing the source instead sidesteps the resolution problem. + """ + source = pytester.path / "test_edited.py" + pyc_dir = source.parent / "__pycache__" + + # both revisions are the same size, so only the content differs + before = "def test_aaa(): assert True\n" + after = "def test_bbb(): assert None\n" + assert len(before) == len(after) + + source.write_text(before, encoding="utf-8") + assert pytester.runpytest_subprocess("-q").ret == 0 + (pyc,) = pyc_dir.glob("test_edited.*.pyc") + mtime = os.stat(source).st_mtime + + source.write_text(after, encoding="utf-8") + # pin the mtime so the edit is indistinguishable by timestamp + os.utime(source, (mtime, mtime)) + assert pyc.exists() # the pyc written by the first run is still there + + result = pytester.runpytest_subprocess("-q") + result.stdout.fnmatch_lines(["*test_bbb*"]) + assert result.ret != 0 def test_reload_is_same_and_reloads(self, pytester: Pytester) -> None: """Reloading a (collected) module after change picks up the change."""