diff --git a/README.md b/README.md index 90005bb..bb55c4d 100644 --- a/README.md +++ b/README.md @@ -40,11 +40,8 @@ here is the difference.** pip install nodrift ``` -Requires Python 3.9+. Unix only for now: the per-call timeout is portable as -of 0.1.2, but per-run temporary directories are still scrubbed with POSIX -paths, so on Windows identical code would compare unequal ([#17]). - -[#17]: https://github.com/LuShadowX/nodrift/issues/17 +Requires Python 3.9+. Tested in CI on Linux, macOS and Windows across Python +3.9, 3.11 and 3.13. ## Use @@ -142,10 +139,9 @@ most of it touches one file and needs one test. Start with the [good first issues][gfi]. The broad areas that need help: -- **Side-effect capture** — intercept `requests`, `sqlalchemy`, `open()` -- **Comparators** for types that need tolerance, e.g. numpy arrays -- **Performance** — recording currently costs 15-20x -- **Windows support** — replace the `SIGALRM` timeout +- **Side-effect capture** — intercept `requests`, `sqlalchemy` +- **Comparators** for types that need a policy, e.g. datetimes and UUIDs +- **Performance** — recording currently costs ~8x - **Framework adapters** beyond pytest The most valuable bug report of all is a **false positive**: if `nodrift check` diff --git a/src/nodrift/cli.py b/src/nodrift/cli.py index de2e476..168e510 100644 --- a/src/nodrift/cli.py +++ b/src/nodrift/cli.py @@ -56,6 +56,28 @@ def _git(*argv: str, cwd: str | None = None) -> str: ).stdout.strip() +def _strip_bytecode(root: str) -> None: + """Delete compiled bytecode from a staged tree. + + A repository with `__pycache__` committed — or a stray `.pyc` — hands the + replay bytecode compiled from *other* source. Python will run it in + preference to the file we just materialised, so `check` reports no + behaviour change while the two versions genuinely differ. Silent, and + worse than an error. + """ + for dirpath, dirnames, filenames in os.walk(root): + if os.path.basename(dirpath) == "__pycache__": + shutil.rmtree(dirpath, ignore_errors=True) + dirnames[:] = [] + continue + for name in filenames: + if name.endswith((".pyc", ".pyo")): + try: + os.remove(os.path.join(dirpath, name)) + except OSError: + pass + + def _export(ref: str, dest: str) -> None: """Materialise `ref` into `dest` (which is emptied first).""" if os.path.exists(dest): @@ -66,6 +88,7 @@ def _export(ref: str, dest: str) -> None: capture_output=True, check=True, ).stdout subprocess.run(["tar", "-x", "-C", dest], input=archive, check=True) + _strip_bytecode(dest) def _export_worktree(dest: str) -> None: @@ -80,6 +103,7 @@ def _export_worktree(dest: str) -> None: target = os.path.join(dest, rel) os.makedirs(os.path.dirname(target), exist_ok=True) shutil.copy2(src, target) + _strip_bytecode(dest) def _replay(recording: str, source_root: str, out: str, sub: str) -> None: @@ -136,6 +160,8 @@ def cmd_check(args: argparse.Namespace) -> int: base_2 = os.path.join(results, "base2.json") head = os.path.join(results, "head.json") + candidate = getattr(args, "against", None) + print(f"nodrift: replaying {args.ref} ...", file=sys.stderr) _export(args.ref, stage) _replay(recording, stage, base_1, args.subdir) @@ -143,8 +169,14 @@ def cmd_check(args: argparse.Namespace) -> int: # nondeterministic and cannot support a claim either way. _replay(recording, stage, base_2, args.subdir) - print("nodrift: replaying working tree ...", file=sys.stderr) - _export_worktree(stage) + # With no second ref the candidate is the working tree, which is the + # common case: check what you are about to commit. + print(f"nodrift: replaying {candidate or 'working tree'} ...", + file=sys.stderr) + if candidate: + _export(candidate, stage) + else: + _export_worktree(stage) _replay(recording, stage, head, args.subdir) report = compare(base_1, head, base_2) @@ -233,7 +265,10 @@ def main(argv: list[str] | None = None) -> int: chk = sub.add_parser("check", help="compare a git ref against the working tree") chk.add_argument("ref", nargs="?", default="HEAD", - help="git ref to compare against (default HEAD)") + help="git ref to treat as the baseline (default HEAD)") + chk.add_argument("against", nargs="?", default=None, + help="second ref to compare with; defaults to the " + "working tree") chk.add_argument("--recording", "-r", default=None) chk.add_argument("--subdir", default="", help="path within the repo holding the package (e.g. src)") diff --git a/tests/test_end_to_end.py b/tests/test_end_to_end.py index d98ea3b..a7f3978 100644 --- a/tests/test_end_to_end.py +++ b/tests/test_end_to_end.py @@ -473,3 +473,86 @@ def test_both_timeout_mechanisms_replay_a_recording_identically( stable = {key for key, value in first.items() if second.get(key) == value} assert stable, "replay produced nothing deterministic to compare" assert {k: with_watchdog[k] for k in stable} == {k: first[k] for k in stable} + + +def test_check_compares_two_arbitrary_refs(tmp_path): + """`nodrift check A B` must compare the two commits, not the worktree. + + Reviewing someone else's branch, or auditing a release, means comparing + two commits neither of which is checked out. + """ + repo = _git_repo(tmp_path) + _nodrift(repo, "record", "--package", "shapes") + + def commit(message): + subprocess.run(["git", "add", "-A"], cwd=str(repo), check=True, + capture_output=True) + subprocess.run( + ["git", "-c", "user.email=t@t", "-c", "user.name=t", + "commit", "-qm", message], + cwd=str(repo), check=True, capture_output=True, + ) + + source = (repo / "shapes.py").read_text() + (repo / "shapes.py").write_text( + source.replace('return "positive"', 'return "POSITIVE"')) + commit("change the label") + + # Put the working tree back to the original, so a comparison that used it + # instead of the second ref would wrongly report no change. + (repo / "shapes.py").write_text(source) + + changed = _nodrift(repo, "check", "HEAD~1", "HEAD") + assert changed.returncode == 1, changed.stdout + changed.stderr + assert "behave differently" in changed.stdout + + same = _nodrift(repo, "check", "HEAD~1", "HEAD~1") + assert same.returncode == 0, same.stdout + same.stderr + + +def test_check_still_defaults_to_the_working_tree(tmp_path): + repo = _git_repo(tmp_path) + _nodrift(repo, "record", "--package", "shapes") + + source = (repo / "shapes.py").read_text() + (repo / "shapes.py").write_text( + source.replace('return "positive"', 'return "POSITIVE"')) + + changed = _nodrift(repo, "check", "HEAD") + assert changed.returncode == 1, changed.stdout + changed.stderr + assert "working tree" in changed.stderr + + +def test_committed_bytecode_cannot_shadow_the_staged_source(tmp_path): + """Stale `.pyc` in a ref must not be executed in place of its source. + + A repository with `__pycache__` committed hands the replay bytecode + compiled from other source. Python prefers it to the file just + materialised, so both versions behave identically and `check` reports no + change while the code genuinely differs — a confident wrong answer. + """ + repo = _git_repo(tmp_path) + _nodrift(repo, "record", "--package", "shapes") + + source = (repo / "shapes.py").read_text() + (repo / "shapes.py").write_text( + source.replace('return "positive"', 'return "POSITIVE"')) + + # Recording imported the original module, so __pycache__ now holds + # bytecode for the *old* source. Committing it is what sets the trap. + cache = repo / "__pycache__" + assert cache.exists(), "expected recording to have left bytecode behind" + subprocess.run(["git", "add", "-A"], cwd=str(repo), check=True, + capture_output=True) + subprocess.run( + ["git", "-c", "user.email=t@t", "-c", "user.name=t", + "commit", "-qm", "commit the bytecode too"], + cwd=str(repo), check=True, capture_output=True, + ) + + result = _nodrift(repo, "check", "HEAD~1", "HEAD") + assert result.returncode == 1, ( + "stale bytecode hid a real behaviour change\n" + + result.stdout + result.stderr + ) + assert "classify" in result.stdout