Repository navigation
test(plugin-contract): prove reload after unload (#34) - #35
Conversation
The lifecycle suite proved repeated source and exact unload but never loaded a plugin again after unloading it, so a plugin broken only on its second load passed every case. Add a reload scenario that loads, unloads and loads again in a clean shell, compares the second load with the first, and calls the restored callback to observe its effect. Give the fixture callback an observable effect, and add two negative fixtures: one whose unload leaves an inert stub (caught by the surface comparison) and one whose callback survives byte-identical but has lost its effect (caught only by the effect check). Document reload in the README lifecycle example. Refs #34
|
Review request record for head 8b074c5.
Outcome: review request not registered; cause unconfirmed. This is a class 2 repository, and the maintainer has elected the ADR-0026 fallback for this case, so the review of record will be a fallback review on this head. It opens with the marker for a request that did not register. |
ss-o
left a comment
There was a problem hiding this comment.
Fallback review under ADR-0026: Copilot request not registered on 8b074c5
Findings
No blocking or major findings. One minor finding and three nits, each on an inline thread:
- Minor,
README.md:54. The reload paragraph says to observe that "a restored resource" still does its job. The testing standard asks that restored "handlers, hooks, widgets, or other resources perform their intended behavior". A reader who checks one resource can miss another. An adversarial fixture whose callback works after reload but whose restoredprecmdhandler has lost its effect passes thereloadscenario (rc 0, non-interactive and interactive). The shipped fixture is not exposed: its hook and widget both dispatch to the one checked callback, and the surface comparison pins those registrations. Remedy: say "each restored handler, hook or widget", and state that the scenario checks the fixture's single callback. - Nit,
tests/plugin-contract.zunit:163. Thereload-stalecase asserts only the exit status. Remedy: also assert empty output, as the positive cases do. - Nit,
tests/_support/plugin-contract/reload-stale.plugin.zsh:14. The stale state lives under_zunit_*, a name the observer excludes. This is not a vacuous pass: if the exclusion were removed, the surface comparison would fail first with exit 1 and the case (expecting 24) would fail loudly. The same defect with file-backed state also exits 24. No change needed; the comment says why the name was chosen. - Nit,
tests/_support/plugin-contract/scenario.zsh:14. A fixture name that passes validation but has no file ends with exit 127 and a rawsourceerror, instead of the scenario's own exit 2 message. Remedy: check the file is readable before sourcing.
Scope: commit 8b074c5, the complete six-file diff against main (1dc51f1), for #34.
Note
This is the maintainer-elected fallback review of record, based on a separate read-only review session and on reruns by the author's session. It is not an independent human review, and it is not approval to merge.
Validation
- Passed, hosted on this head: zunit (native), zsh-n, Zsh Lint, Observe Zsh Lint, Trunk Code Quality, Org Routing, and every Commit Lint policy job.
- Passed, local: full suite 129 of 129 on Zsh 5.9.2;
tests/plugin-contract.zunit15 of 15 on Zsh 5.5.1 (the declared floor) and 5.8.1; six hand mutants of the new checks, all killed. - Passed, probe rerun on this head: the shipped fixture passes
reload(rc 0) in both modes.reload-brokenexits 1 and namesfunction:_contract_fixture_callback.reload-staleexits 24 underreloadand passesordinary. A file-backed variant of the stale defect also exits 24. - Not run: the scheduled Zsh matrix (5.6.2, 5.7.1, 5.8, 5.9).
Checklist verdicts and supporting evidence
- Acceptance (#34): met. A fixture correct only on its first load fails the new case (
reload-stale, exit 24), and the shipped fixture passes it. The reload case checks an effect:scenario.zshclears_contract_fixture_effect, calls the callback and requiresran. The native suite passes, and the README shows reload after unload.Closes #34is justified. - Correctness: sound for its stated scope. The second load is compared with the first through
plugin_restored. The effect variable is reset just before the call, so the effect check cannot pass on a stale value. - Negative fixtures: fail for their stated reasons.
reload-brokenis also rejected byordinary, because its unload leaves the stub; its comment only claims that the first load and repeated source are correct, which holds. - Zsh standard: conforms.
extended_globis set before the unquoted validation pattern, and../fixtureis refused with exit 2. The fixtures use an anonymous function withemulate -L zsh, explicittypeset -gglobals and self-removing unload functions. No construct needs a Zsh newer than the 5.5.1 floor. - Tests: one behavior each, exact assertions, apart from finding 2.
- README example: matches the API.
assert A plugin_restored Bdispatches to_zunit_assert_plugin_restored;runsets$stateand$output;same_asexists. - Security and state integrity: no issues. The new argument is validated, and the scenario runs under
zsh -f.
Limits and follow-up
The findings will be fixed in one push, and that push voids this review. A new fallback review will be posted on the new head. Merging needs the maintainer's decision.
Refuse a fixture name that has no file with the scenario's own exit 2 message instead of a raw source error, and cover it with a case. Assert empty output for the reload-stale case as the positive cases do. Say in the README that each restored handler, hook or widget needs its own effect check, and that the scenario checks the fixture's one callback. Refs #34
ss-o
left a comment
There was a problem hiding this comment.
Fallback review under ADR-0026: Copilot request not registered on 8799b23
Findings
No blocking or major findings. One minor finding and two nits, each on an inline thread. The minor finding is open for a maintainer decision (see Limits).
- Minor,
tests/_support/plugin-contract/scenario.zsh:68. Thereloadscenario never unloads after the second load, andordinarynever unloads a reloaded plugin. A plugin whose unload is correct once but leaks after a reload passes every case. An adversarial fixture shows this: its unload removes itsprecmdhook only the first time, keyed on a flag in an observer-excluded name. It passes bothreloadandordinary(rc 0). #34's own reproduction checked that "a second unload is exact", but none of its acceptance criteria require it. Remedy: snapshot before the first load, then after the effect check unload again and assertplugin_restoredagainst that first snapshot. Alternatively, record the gap in a follow-up issue. - Nit,
tests/_support/plugin-contract/reload-broken.plugin.zsh:1.ordinaryalso rejects this fixture (rc 1,function:_contract_fixture_callback), because its first unload leaves the stub. The comment is accurate, but onlyreload-staleis caught byreloadalone. Remedy: note that the reload case pins the z-shell/z-a-meta-plugins#54 shape as a regression rather than adding unique coverage. - Nit,
tests/_support/plugin-contract/scenario.zsh:11. The invalid-name branch works (../fixtureexits 2 withinvalid fixture: ../fixture), but no case covers it. Remedy: add one case, or accept the gap.
The pull request body's stale counts (four cases, 129 and 15) were corrected in the body; that edit changes no code.
Scope: commit 8799b23, the complete six-file diff against main (1dc51f1), plus the delta since the previous review on 8b074c5, whose four threads were acted on and resolved.
Note
This is the maintainer-elected fallback review of record, based on a second separate read-only review session and on reruns by the author's session. It is not an independent human review, and it is not approval to merge.
Validation
- Passed, hosted on this head: zunit (native), zsh-n, Zsh Lint, Observe Zsh Lint, Trunk Code Quality, Org Routing and the four Commit Lint policy jobs (10 of 10 check runs, confirmed by head SHA).
- Passed, local: full suite 130 of 130 on Zsh 5.9.2;
tests/plugin-contract.zunit16 of 16 on Zsh 5.5.1 (the declared floor) and 5.8.1; eight hand mutants of the new checks and guard, all killed. - Passed, probe rerun on this head: the shipped fixture passes every scenario.
reload-brokenexits 1 andreload-staleexits 24. Adversarial fixtures for a leftover load guard, a helper lost on reload and a widget bound to an inert function each exit 1. A missing fixture exits 2 withmissing fixture, and../fixtureexits 2 withinvalid fixture. - Not run: the scheduled Zsh matrix (5.6.2, 5.7.1, 5.8, 5.9).
Checklist verdicts and supporting evidence
- Acceptance (#34): met.
reload-staleis correct only on its first load and fails the new case with exit 24, while the shipped fixture passes. The effect is checked by value (_contract_fixture_effect == ran), the native suite passes, and the README shows reload after unload.Closes #34is justified. - Reload correctness: sound for load, unload, load.
plugin_restored loaded reloadedcompares every snapshot family. The effect variable is created after thereloadedsnapshot, so it cannot affect the comparison. The one escape found is finding 1. - Negative fixtures: each fails for its stated reason, and the tests pin that reason (exit status plus resource name, or exit status plus empty output).
- Zsh standard: conforms.
extended_globis set before the validation pattern; output usesprint -u2 -r --;$fixtureis quoted; the fixtures use anonymous functions withemulate -L zsh. No construct needs a Zsh newer than the 5.5.1 floor. - Tests: one behavior each, exact assertions, none vacuous. The
same_asliteral contains no pattern characters. - README example: matches the API.
- Security and state integrity: no issues. A leading
-in a fixture name only becomes a path suffix and ends asmissing fixture.
Limits and follow-up
Finding 1 is a new kind of gap found by a second review round. Fixing it in this pull request needs another push and a third review round, so it waits for the maintainer to choose: fix it here, or record it in a follow-up issue and continue. The threads stay open until then. Merging needs the maintainer's decision.
Summary
Refs #34. Refs z-shell/.github#585. Follow-up: #36 (an exact unload after a reload, found by the second review and left out of this PR by maintainer decision).
The portable lifecycle suite proved repeated source (load, load) and exact unload, but it never loaded a plugin again after unloading it. A plugin broken only on its second load therefore passed every case. This adds that case.
tests/_support/plugin-contract/scenario.zsh: areloadscenario loads, unloads and loads again in one clean shell. It compares the second load's snapshot with the first (plugin_restored), then calls the restored callback and checks its effect, not only its presence or exit status. An optional third argument names the fixture, validated against[A-Za-z0-9_-]##; a name with no file exits 2 withmissing fixture.tests/_support/plugin-contract/fixture.plugin.zsh: the callback now has an observable effect, so the reload case has something to observe.reload-broken.plugin.zshleaves an inert stub on unload (the refactor(plugin): adopt Zsh Plugin Standard 2 z-a-meta-plugins#54 shape). The surface comparison rejects it: exit 1, namingfunction:_contract_fixture_callback.reload-stale.plugin.zshrestores a byte-identical callback that has lost its effect, because unload forgets a one-time-setup flag. Only the effect check rejects it: exit 24.tests/plugin-contract.zunit: five cases: reload inzsh -fandzsh -f -i, both negative fixtures, and a fixture name with no file (exit 2,missing fixture).README.md: the lifecycle section shows reload after unload.Exact restoration after unload stays the
ordinaryscenario's job and is not repeated inreload.Verification
All runs are local; this is the evidence until the hosted checks report on this head.
./zunit --tap testson zsh 5.9.2: 130 of 130 ok.tests/plugin-contract.zunitin local images on zsh 5.5.1 (the floor inzsh-lint.json) and 5.8.1: 16 of 16 ok on each. The scheduled matrix (5.5.1 to 5.9) is not run here.zsh -nover thezsh-n.ymlfile set, andzsh -f -npluszcompile -U -zon every changed test source: clean.Agent handoff
Status: ready to merge by maintainer decision, with the post-reload unload gap moved to #36. #34 stays open after merge: every acceptance criterion it lists is met by this PR, so the maintainer can close it once the native job on
mainconfirms. Remaining after merge: re-check z-shell/z-a-meta-plugins and z-shell/zsh-eza against the new case (z-shell/.github#585 says both already satisfy it), then close z-shell/.github#585, which completes initiative z-shell/.github#556.