diff --git a/src/nodrift/cli.py b/src/nodrift/cli.py index 12b7c2e..57f5c59 100644 --- a/src/nodrift/cli.py +++ b/src/nodrift/cli.py @@ -194,13 +194,15 @@ def _print_not_covered(abandoned: list[str], verbose: bool) -> None: """Say which functions the verdict above does not speak for. Without this, `check` reports "no behaviour change" while silently - omitting every function whose inputs were too large to record — on - `sqlparse` that was 40 of the most important ones. + omitting every function whose inputs could not be recorded — because they + were too large, or because they were never picklable in the first place + (a function taking a callback on every call, say). On `sqlparse` that is + 42 of the most important ones. """ if not abandoned: return print(f" {len(abandoned)} function(s) not fully recorded " - f"(inputs too large to capture) — not covered by this check") + f"(inputs too large, or not picklable) — not covered by this check") if verbose: for name in abandoned: print(f" {name}") diff --git a/src/nodrift/plugin.py b/src/nodrift/plugin.py index abec5f4..b3a394d 100644 --- a/src/nodrift/plugin.py +++ b/src/nodrift/plugin.py @@ -183,7 +183,7 @@ def pytest_unconfigure(config): if summary.get("abandoned"): print( f"[nodrift] {len(summary['abandoned'])} functions not recorded " - f"(inputs too large to capture); they are NOT covered", + f"(inputs too large, or not picklable); they are NOT covered", file=sys.stderr, ) _recorder = None diff --git a/src/nodrift/recorder.py b/src/nodrift/recorder.py index 07e8eea..abda56b 100644 --- a/src/nodrift/recorder.py +++ b/src/nodrift/recorder.py @@ -17,8 +17,6 @@ import types from collections import defaultdict -from .fingerprint import digest, fingerprint - _local = threading.local() _GZIP_MAGIC = b"\x1f\x8b" @@ -93,7 +91,23 @@ def __init__( # recording, so oversized inputs are counted and dropped. self.max_blob_bytes = max_blob_bytes self.abandon_after = 3 + # An oversized blob is evidence about that one input; a pickling + # failure is usually evidence about the signature. But "usually" is + # not "always" — a function that takes a callback on some paths and + # plain data on others is still worth recording — so a failure gets + # considerably more rope than an oversized input before the target is + # written off. It costs almost nothing to be generous here: a + # signature that genuinely cannot be pickled fails thousands of times + # in a run, so it trips any threshold in this range immediately. + self.abandon_unpicklable_after = 16 self._oversized_streak: dict[str, int] = defaultdict(int) + # Some functions take a callback — a lambda, a local function, a + # closure — on every single call. Those arguments can never be + # pickled, and the pickler only discovers that after walking the rest + # of the argument graph, so the full serialisation cost is paid and + # then thrown away. Failure is a property of the signature far more + # often than of one input, so it is worth remembering. + self._unpicklable_streak: dict[str, int] = defaultdict(int) self._abandoned: set[str] = set() # Adaptive sampling: how many consecutive duplicates before backing # off, and how far back-off is allowed to go. @@ -102,7 +116,7 @@ def __init__( self._dup_streak: dict[str, int] = defaultdict(int) self._skip: dict[str, int] = defaultdict(lambda: 1) self._countdown: dict[str, int] = defaultdict(int) - self.calls: dict[str, dict[str, bytes]] = defaultdict(dict) + self.calls: dict[str, dict[bytes, bytes]] = defaultdict(dict) self.recorded_outcome: dict[str, dict[str, list]] = defaultdict(dict) self.stats = defaultdict(int) # Guards every counter below. A threaded suite would otherwise exceed @@ -246,6 +260,10 @@ def _capture(self, target: str, args, kwargs) -> None: return with self._lock: if target in self._abandoned: + # Counted, not just skipped: every call has to land in some + # bucket or the totals stop adding up, and abandonment is now + # the common reason a call is never looked at. + self.stats["skipped_abandoned"] += 1 return bucket = self.calls[target] if len(bucket) >= self.max_per_target: @@ -275,11 +293,26 @@ def _capture(self, target: str, args, kwargs) -> None: except Exception: with self._lock: self.stats["unpicklable"] += 1 + # Same reasoning as oversized inputs: paying the serialisation + # cost only to discard the result is exactly what makes a + # library slow to record. A target whose arguments keep + # refusing to pickle is not going to start, so stop asking — + # and say out loud that it is not covered, rather than + # retrying it silently for the rest of the run. + self._unpicklable_streak[target] += 1 + if (self._unpicklable_streak[target] + >= self.abandon_unpicklable_after): + self._abandoned.add(target) + self.stats["abandoned_targets"] += 1 + self.stats["abandoned_unpicklable"] += 1 return finally: _local.busy = False with self._lock: + # The pickle succeeded, so whatever made earlier calls fail was + # about those inputs and not about this function. + self._unpicklable_streak[target] = 0 if len(blob) > self.max_blob_bytes: self.stats["oversized"] += 1 # Paying the pickle cost only to discard the result is what @@ -293,8 +326,11 @@ def _capture(self, target: str, args, kwargs) -> None: self._oversized_streak[target] = 0 self.stats["captured"] += 1 - key = digest(["blob", len(blob), _cheap_hash(blob)]) - if key in bucket: + # The pickled bytes are themselves the identity of the input, so + # they are the key. Hashing them into a digest first cost a full + # blake2b pass over every blob plus a JSON round-trip, to produce + # something no more precise than the bytes it replaced. + if blob in bucket: # Seen before. Back off, but never so far that a function which # later receives novel inputs stays invisible. self._dup_streak[target] += 1 @@ -313,7 +349,7 @@ def _capture(self, target: str, args, kwargs) -> None: # fully again. self._dup_streak[target] = 0 self._skip[target] = 1 - bucket[key] = blob + bucket[blob] = blob # -- teardown ------------------------------------------------------- @@ -349,12 +385,6 @@ def dump(self, path: str) -> dict: return summary -def _cheap_hash(blob: bytes) -> str: - import hashlib - - return hashlib.blake2b(blob, digest_size=16).hexdigest() - - def merge_recordings(shards: list[str], out: str, cap: int | None = None) -> dict: """Combine per-worker recordings into one, dropping duplicates. diff --git a/tests/test_fingerprint.py b/tests/test_fingerprint.py index f0db323..47fd90e 100644 --- a/tests/test_fingerprint.py +++ b/tests/test_fingerprint.py @@ -125,6 +125,54 @@ def test_sampling_resets_when_a_function_stays_productive(): assert recorder.stats.get("sampled_out", 0) == 0 +def test_a_function_whose_arguments_never_pickle_is_given_up_on(): + """Unpicklable arguments must not be re-attempted for the whole run. + + A function taking a lambda on every call can never be recorded, but the + pickler only discovers that after walking the rest of the argument graph. + On `sqlparse` two such functions cost 5.2s of a 10s run — more than half + the recording time, spent entirely on serialisation that was thrown away. + """ + from nodrift.recorder import Recorder + + recorder = Recorder(["_none_"], max_per_target=600) + attempts = 0 + for _ in range(5000): + before = recorder.stats["unpicklable"] + recorder._capture("m:cb", (lambda t: t, "payload"), {}) + attempts += recorder.stats["unpicklable"] - before + + assert "m:cb" in recorder._abandoned + assert attempts <= recorder.abandon_unpicklable_after, ( + f"{attempts} wasted pickles; should stop at " + f"{recorder.abandon_unpicklable_after}" + ) + # The gap has to be declared as a gap, not merely skipped quietly. + assert recorder.stats["abandoned_unpicklable"] == 1 + + +def test_a_function_that_only_sometimes_takes_a_callback_stays_recorded(): + """Giving up must be about the signature, not one awkward input. + + This is the coverage half of the trade: on `sqlparse`, abandoning after 3 + consecutive failures lost 66 inputs from two filters that take a callback + on some paths and plain data on others. Nothing is gained by writing those + functions off, because a truly unpicklable one fails thousands of times. + """ + from nodrift.recorder import Recorder + + recorder = Recorder(["_none_"], max_per_target=600) + for i in range(300): + # A short burst of failures, then real inputs — never long enough in a + # row to look like a signature that cannot be pickled. + recorder._capture("m:mixed", (lambda t: t,), {}) + recorder._capture("m:mixed", (lambda t: t,), {}) + recorder._capture("m:mixed", (i,), {}) + + assert "m:mixed" not in recorder._abandoned + assert len(recorder.calls["m:mixed"]) > 100, "novel inputs were lost" + + def test_the_cap_holds_when_threads_record_at_once(): """A threaded suite must not be able to record past max_per_target. @@ -181,6 +229,7 @@ def test_stats_add_up_when_threads_record_at_once(): + recorder.stats["skipped_at_cap"] + recorder.stats["unpicklable"] + recorder.stats["oversized"] + + recorder.stats["skipped_abandoned"] ) assert accounted == total, f"{total - accounted} calls unaccounted for" assert len(recorder.calls["m:i"]) == recorder.stats["captured"]