Skip to content

The suite can silently test a different checkout's source and still report green #435

Description

@willhea

What's wrong

The test suite can silently run against a different copy of the source than the one you are
editing, and report a full green while doing it. Reviewing a pull request in a git worktree
is the case where this happens, and it makes red-green verification meaningless: reverting
the change under review does not affect the result, because the test never imported that
file.

pyproject.toml sets pythonpath = [".", "tools"] and deliberately excludes src, so the
engine is importable only through the installed package (#398, the installable-engine
work). That decision is sound and worth keeping. Its consequence is that import deltatrack resolves to whatever single absolute path the editable install recorded, which
is the checkout where uv sync was last run. A worktree that reuses another checkout's
virtualenv therefore runs the worktree's tests/ against the other checkout's product
code, because pythonpath = ["."] picks up tests/ from the worktree while deltatrack
comes from elsewhere.

How it surfaced

Found while clearing local git state, after a virtualenv was left pointing at a deleted
worktree. Demonstrated deliberately in a throwaway worktree: a top-level
raise RuntimeError("FAULT INJECTED IN WORKTREE") was appended to the worktree's
src/deltatrack/bill_tree.py, then the suite was run from that worktree using another
checkout's interpreter.

$ /path/to/other-checkout/.venv/bin/python -m pytest tests/test_bill_tree.py -q
133 passed, 1 skipped in 1.78s

The fault is unreachable code as far as that run is concerned. Setting
PYTHONPATH=$PWD/src makes the same command fail immediately at
src/deltatrack/bill_tree.py:1164: RuntimeError: FAULT INJECTED IN WORKTREE, confirming the
import is the variable and nothing else.

A related failure has the same root: if the editable install points at a path that has since
been deleted, every run in the owning checkout dies at tests/conftest.py:13 with
ModuleNotFoundError: No module named 'deltatrack'. That message reads as "this branch
broke the package" rather than "the environment points somewhere that no longer exists," and
it costs real time to diagnose.

Checked on develop at 08b9b80.

Why it matters

A green suite is the project's evidence that a change is safe, and every verification norm
here depends on it describing the code in front of you. This failure produces a green that
describes different code, with nothing in the output to say so. It is worst exactly where it
is most likely: reviewing someone else's pull request, which is when a worktree gets used and
when red-green is the whole point of the exercise.

Nothing currently detects it. git status is clean, the suite is green, and the virtualenv
is gitignored, so every git-based habit for noticing that something is off is blind to it.

What to do

Add a guard in tests/conftest.py, which runs before any test in every invocation, covering
both halves:

  • deltatrack resolves outside the rootdir being tested: fail immediately, naming the path
    it actually resolved to and the tree it should have come from.
  • deltatrack does not import at all: replace ModuleNotFoundError with a message that
    names the stale editable pointer and the one-line repair.

Do not fix this by adding src to pythonpath. That would make the suite import the
source tree directly and quietly retire the guarantee that it exercises the installed
package, which is the thing tests/test_engine_installs.py and the pythonpath comment
exist to preserve.

The convention that avoids the situation should be written down alongside it: a worktree
gets its own virtualenv. The reason it was avoided before does not hold — uv sync in a
fresh worktree completes in about a second on a warm cache, and the resulting environment
imports the worktree's own code (verified: the same injected fault fails there, as it
should).

Verification

"The suite passes" cannot demonstrate this fix, because the suite passed while the defect
was active — that is the defect. The guard must be shown to fire against the known-bad
setup: run the suite from a worktree using another checkout's interpreter and confirm it
now fails with the new message where it previously reported 133 passed.

Unverified

Whether any past review reached a wrong conclusion this way. The mechanism is confirmed;
its history is not reconstructed.

Refs #398, #401

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions