diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/design/naming-audit.org | 91 | ||||
| -rw-r--r-- | docs/subr-mock-migration-spec.org | 158 |
2 files changed, 249 insertions, 0 deletions
diff --git a/docs/design/naming-audit.org b/docs/design/naming-audit.org new file mode 100644 index 000000000..f053ccb25 --- /dev/null +++ b/docs/design/naming-audit.org @@ -0,0 +1,91 @@ +#+TITLE: Naming Audit — Public/Private Boundaries in Owned Elisp +#+AUTHOR: Craig Jennings +#+DATE: 2026-06-29 + +* Purpose + +A one-time audit of symbol-naming boundaries across config-owned Elisp, plus +the standing allowlist future reviews cite instead of re-arguing. The +convention itself lives in =.claude/rules/elisp.md=; this file records what the +scan found and how each finding was classified. + +* Convention (summary) + +- User-facing commands and shared helpers: =cj/name=. +- Private helpers inside cj-owned modules: =cj/--name=. +- Package-like standalone modules: =package-name= for public API, + =package--name= for internals (e.g. =calendar-sync--=, =mouse-trap--=, + =localrepo-=). +- No new unprefixed =defun=/=defvar=/=defcustom=/=defconst= in owned modules. +- Tested private helpers may stay private; the test references them directly. + +* Scan method + +#+begin_src sh +rg -n '^\((defun|defvar|defcustom|defconst|defgroup|defmacro) [^ )/]+' modules custom +#+end_src + +The raw scan over-reports. Two large classes are not findings: + +- *Foreign forward-declarations.* A bare =(defvar foreign-var)= with no value + declares another package's special variable to quiet the byte-compiler. It + defines nothing owned. The bulk of the raw hits are these (=org-*=, =emms-*=, + =lsp-*=, =eat-*=, =mu4e-*=, and so on). +- *Vendored third-party files* under =custom/= (=elpa-mirror.el= =elpamr-=, + =eplot.el= =eplot-=). These keep their upstream prefixes by policy. + +* Findings + +** Renamed (clear low-risk, done 2026-06-29) + +| Old | New | Why | +|------------------------------------------+-------------------------------+-------------------------------------------| +| =car-member= (local-repository.el) | =localrepo--car-member= | Unprefixed, generic name with real | +| | | collision risk; pure non-interactive | +| | | helper used only within its module + | +| | | test. | +|------------------------------------------+-------------------------------+-------------------------------------------| +| =unpropertize-kill-ring= | =cj/--unpropertize-kill-ring= | Unprefixed; non-interactive | +| (system-defaults.el) | | =kill-emacs-hook= function, contained to | +| | | its module + tests. | +|------------------------------------------+-------------------------------+-------------------------------------------| + +** Acceptable package prefixes (allowlist — no change) + +These are deliberate, consistent module prefixes, not unprefixed leaks: + +- =env-= / =env--= — host-environment.el. +- =localrepo-= — local-repository.el (public API + defcustoms). +- =mouse-trap-= / =mouse-trap--= — mousetrap-mode.el. +- =show-kill-= — show-kill-ring.el. +- =my-eww-= / =my-eww--= — eww-config.el. +- =signel-= / =signel--= — signal-config.el. + +** Deferred — needs a focused, approved pass (not unattended) + +Each carries a risk that argues for Craig's eyes rather than a no-approvals +rename: + +- *Keybound / interactive commands*: =toggle-window-split= (bound =M-S-t=), + =ensure-macros-file=, =empty-kill-ring=. A rename touches muscle-memory keys + and =M-x= history; do it with =defalias= shims and a live check. +- *Cross-module macro*: =with-timer= (config-utilities.el, used in wrap-up.el + and an architecture test). A macro rename forces recompilation of every + caller; verify the whole graph. +- *defcustoms*: =theme-file=, =fallback-theme-name= (ui-theme.el). Renaming a + =defcustom= orphans any persisted Customize value; use + =define-obsolete-variable-alias=. (No custom-file currently sets them, so the + risk is latent, not active.) +- *High-blast user constants*: the unprefixed =*-dir= / =*-file= constants in + user-constants.el (=org-dir=, =projects-dir=, =books-dir=, and so on) are + referenced across the whole config. They want their own dedicated rename + task, not a drive-by. + +* Acceptance + +- The scan resolves to: two renames done, a documented allowlist of acceptable + prefixes, and a deferred list with rationale. No unexplained unprefixed owned + symbols remain. +- The two renames are covered by their existing module tests (updated in place). +- Future module reviews cite this file and =elisp.md= rather than re-deriving + the boundary. diff --git a/docs/subr-mock-migration-spec.org b/docs/subr-mock-migration-spec.org new file mode 100644 index 000000000..26f1dd576 --- /dev/null +++ b/docs/subr-mock-migration-spec.org @@ -0,0 +1,158 @@ +#+TITLE: Spec: Migrate Tests Off Mocking C Primitives +#+AUTHOR: Craig Jennings +#+DATE: 2026-06-30 +#+STATUS: Draft — for discussion + +* Status + +Draft. Pulled out of =todo.org= (=** TODO [#C] Migrate tests off mocking +primitives (native-comp robustness) :test:refactor:solo:=) so the scope and +approach can be settled before any code moves. Execution is deferred; this +document is the discussion vehicle. + +Companion reference: [[file:native-comp-subr-mocking.org][native-comp-subr-mocking.org]] holds the full mechanism, the +upstream research, and the 2026-06-21 decision. This spec does not restate the +mechanism; it plans the remaining work that decision deferred. + +* Background — how we hit this + +We re-enabled native compilation config-wide (early-init.el, commit 3fd28987, +2026-06-20). Tests that had been green for months immediately started failing +with no change to their source — the first 8 were window-primitive mocks in +=test-dirvish-config-wrappers.el= and =test-calibredb-epub-config.el= +(=window-body-width=, =window-margins=, =current-window-configuration=, +=get-buffer-window=), throwing =wrong-number-of-arguments= for a zero-arg mock +lambda called with one argument. + +** What we struggled with (the consequences) + +- *Intermittent, non-deterministic failure.* The same test passed, then + crashed, then passed again within a session. Native-comp generates a + per-primitive trampoline =.eln= lazily and caches it on disk; whether a mock + "works" depends on whether that trampoline has been built yet. The + non-determinism was the tell, and it made the failures hard to trust or + reproduce. +- *Three distinct failure modes from one cause* (full detail in the companion + doc): (1) trampoline generation failure under =--batch=; (2) silent bypass — + natively-compiled callers ignore the mock and run the real primitive, so a + test passes for the wrong reason; (3) arity mismatch — the trampoline calls + the mock with the primitive's *maximum* arity, so a fixed-arity mock narrower + than the primitive throws. Mode 3 is the one that bit us; modes 1 and 2 sit + latent. +- *The tempting quick fix is the dangerous one.* Disabling subr trampolines + (=native-comp-enable-subr-trampolines nil=) is the most-cited workaround, but + in our native-comp-heavy setup it produces mode 2 — tests that pass while + asserting against the real primitive. A quiet false pass is worse than a loud + crash. +- *Scale of the latent surface.* The suite mocks subrs in hundreds of places. + The variadic sweep touched 188 arity-narrow mocks. + +** What is already done (the stopgap, 2026-06-21) + +Not currently broken — two things shipped (commits 571da499, b62c3c88): + +1. *Variadic sweep.* Every arity-narrow subr mock got =&rest _= appended, which + tolerates the trampoline's full-arity call. Deterministic, keeps trampolines + on, so no silent bypass. Fixes mode 3. +2. *Meta-test gate.* =tests/test-meta-subr-mock-arity.el= statically scans every + test file for =symbol-function= / =fset= / =setf= subr redefinitions and + fails =make test= if any mock can't accept the primitive's maximum arity. A + new arity-narrow mock can't merge silently. + +The stopgap fixes the mode we actually suffered. It leaves modes 1 and 2 latent. +The durable fix the ecosystem (and our own =elisp-testing.md=) points to is to +*not redefine primitives at all*. That is the work this spec scopes. + +* The real scope — most mocks should NOT move + +The raw inventory is large, but the headline number is misleading. =testing.md= +says to mock external boundaries; converting those to "drive real state" would +mean running real shells and touching the real filesystem in unit tests — the +opposite of what we want. So the actual migration target is narrow. + +Current subr-mock sites across 261 test files (=cl-letf= / =fset= / =advice-add= +on the named primitive): + +| Primitive | Sites | Classification | Disposition | +|-------------------------+-------+-------------------------+-----------------------------------| +| shell-command-to-string | 62 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| executable-find | 60 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| shell-command | 29 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| call-process | 17 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| current-time | 11 | time boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| save-buffer | 10 | file I/O boundary | keep, or real temp-file fixture | +|-------------------------+-------+-------------------------+-----------------------------------| +| write-region | 4 | file I/O boundary | keep, or real temp-file fixture | +|-------------------------+-------+-------------------------+-----------------------------------| +| message | 69 | output-silencing | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| completing-read | 25 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| +| read-string | 16 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| +| yes-or-no-p | 14 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| +| read-from-minibuffer | 6 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| + +The genuine migration target is the UI-prompt bucket: ~61 sites. Per +=elisp-testing.md='s Interactive-vs-Internal rule, the fix is to extract a pure +internal that takes the value as an argument and test that directly, leaving the +interactive wrapper a thin un-tested (or smoke-tested) shell. That removes the +prompt mock entirely — immune to all three failure modes — and improves the +production code's testability as a side effect. + +Boundary mocks (shell, file I/O, time, =executable-find=, =call-process=) stay +mocked: that is correct unit-test practice, and the variadic form already +handles native-comp. =message= is output-silencing, not logic — keep it. + +* Proposed approach + +Not a single sweep. The migration touches production code (each extraction is a +small design change), so it is incremental and reviewable, not mechanical. + +1. *Scoped exemplar pass first.* Pick one module with a few prompt-mocks, do the + extract-internal conversion there, set the pattern, and measure the per-case + effort. This calibrates the rest. +2. *Batch by module afterward.* Convert remaining UI-prompt sites a module at a + time, each its own commit, with the suite green between. +3. *Leave boundaries alone.* No conversion of shell / file / time / process + mocks. The meta-test keeps them arity-safe. + +* Open decisions (resolve in discussion) + +** TODO Confirm the scope: UI-prompt mocks only, boundaries stay +Is the migration scoped to the ~61 UI-prompt sites (completing-read, read-string, +yes-or-no-p, read-from-minibuffer), with all boundary mocks explicitly out of +scope? Or is there an appetite to also convert the file-I/O mocks +(=save-buffer=, =write-region=) to real temp-file fixtures where it reads +cleaner? + +** TODO Reframe the todo.org task title to match the real scope +The current title — "Migrate tests off mocking primitives" — reads as all 300+ +sites. If we agree on UI-prompt-only, retitle to something like "Extract pure +internals for UI-prompt-mocked tests" so a future session does not re-scope it +as a wholesale sweep. + +** TODO Pick the exemplar module for the first pass +Which module gets the calibrating conversion? A small one with 2-4 prompt-mocks +is ideal. Candidate selection needs a per-module breakdown of the ~61 sites +(not yet collected). + +** TODO Decide priority and timing +Currently =[#C]=, =:solo:=. The suite is not broken (stopgap holds), so this is +test-quality debt, not urgent. Confirm it stays low and gets done in batches +between other work, rather than as a dedicated push. + +* Non-goals + +- Re-deriving or re-documenting the native-comp trampoline mechanism (see the + companion doc). +- Converting boundary mocks (shell, file I/O, time, process, executable-find). +- Removing the variadic-mock convention or the meta-test gate — both stay; they + are the standing protection for every mock that legitimately remains. |