Skip to content

test(plugin-contract): prove reload after unload (#34) - #35

Merged
ss-o merged 2 commits into
mainfrom
feature-34
Sep 30, 2026
Merged

ss-o merged 2 commits into
mainfrom
feature-34

Conversation

@ss-o

@ss-o ss-o commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

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: a reload scenario 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 with missing fixture.
  • tests/_support/plugin-contract/fixture.plugin.zsh: the callback now has an observable effect, so the reload case has something to observe.
  • Two negative fixtures, each tried only through the reload scenario:
    • reload-broken.plugin.zsh leaves 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, naming function:_contract_fixture_callback.
    • reload-stale.plugin.zsh restores 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 in zsh -f and zsh -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 ordinary scenario's job and is not repeated in reload.

Verification

All runs are local; this is the evidence until the hosted checks report on this head.

  • Failing first: before the fixture change, the two new positive cases failed with exit 24 (the stock callback had no effect to observe).
  • Full suite ./zunit --tap tests on zsh 5.9.2: 130 of 130 ok.
  • tests/plugin-contract.zunit in local images on zsh 5.5.1 (the floor in zsh-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 -n over the zsh-n.yml file set, and zsh -f -n plus zcompile -U -z on every changed test source: clean.
  • Hand mutants, each in a scratch copy: removing the surface comparison, the effect check, the fixture's effect, the second load, or the effect reset, and making the first compare a no-op. Also removing or inverting the missing-fixture check. Eight of eight were killed by at least one case.
  • Trunk pre-commit: no issues.

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 main confirms. 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.

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
@ss-o ss-o added the type:feature A request for new behavior or capability. label Sep 30, 2026
@ss-o

ss-o commented Sep 30, 2026

Copy link
Copy Markdown
Member Author

Review request record for head 8b074c5.

  • Copilot review is configured for main (the ruleset carries a copilot_code_review rule).
  • At 2026-09-30T13:05:25Z, POST /repos/z-shell/zunit/pulls/35/requested_reviewers with copilot-pull-request-reviewer[bot] returned success, with an empty requested_reviewers list.
  • 90 seconds later the timeline had no review_requested event for Copilot, and the pull request had no review.

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 ss-o moved this to In Progress in Z-shell Delivery Sep 30, 2026

@ss-o ss-o left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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 restored precmd handler has lost its effect passes the reload scenario (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.
  2. Nit, tests/plugin-contract.zunit:163. The reload-stale case asserts only the exit status. Remedy: also assert empty output, as the positive cases do.
  3. 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.
  4. Nit, tests/_support/plugin-contract/scenario.zsh:14. A fixture name that passes validation but has no file ends with exit 127 and a raw source error, 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.zunit 15 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-broken exits 1 and names function:_contract_fixture_callback. reload-stale exits 24 under reload and passes ordinary. 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.zsh clears _contract_fixture_effect, calls the callback and requires ran. The native suite passes, and the README shows reload after unload. Closes #34 is 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-broken is also rejected by ordinary, 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_glob is set before the unquoted validation pattern, and ../fixture is refused with exit 2. The fixtures use an anonymous function with emulate -L zsh, explicit typeset -g globals 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 B dispatches to _zunit_assert_plugin_restored; run sets $state and $output; same_as exists.
  • 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.

Comment thread README.md Outdated
Comment thread tests/plugin-contract.zunit
Comment thread tests/_support/plugin-contract/reload-stale.plugin.zsh
Comment thread tests/_support/plugin-contract/scenario.zsh
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 ss-o left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

  1. Minor, tests/_support/plugin-contract/scenario.zsh:68. The reload scenario never unloads after the second load, and ordinary never 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 its precmd hook only the first time, keyed on a flag in an observer-excluded name. It passes both reload and ordinary (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 assert plugin_restored against that first snapshot. Alternatively, record the gap in a follow-up issue.
  2. Nit, tests/_support/plugin-contract/reload-broken.plugin.zsh:1. ordinary also rejects this fixture (rc 1, function:_contract_fixture_callback), because its first unload leaves the stub. The comment is accurate, but only reload-stale is caught by reload alone. Remedy: note that the reload case pins the z-shell/z-a-meta-plugins#54 shape as a regression rather than adding unique coverage.
  3. Nit, tests/_support/plugin-contract/scenario.zsh:11. The invalid-name branch works (../fixture exits 2 with invalid 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.zunit 16 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-broken exits 1 and reload-stale exits 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 with missing fixture, and ../fixture exits 2 with invalid 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-stale is 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 #34 is justified.
  • Reload correctness: sound for load, unload, load. plugin_restored loaded reloaded compares every snapshot family. The effect variable is created after the reloaded snapshot, 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_glob is set before the validation pattern; output uses print -u2 -r --; $fixture is quoted; the fixtures use anonymous functions with emulate -L zsh. No construct needs a Zsh newer than the 5.5.1 floor.
  • Tests: one behavior each, exact assertions, none vacuous. The same_as literal 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 as missing 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.

Comment thread tests/_support/plugin-contract/scenario.zsh
Comment thread tests/_support/plugin-contract/reload-broken.plugin.zsh
Comment thread tests/_support/plugin-contract/scenario.zsh
@ss-o
ss-o merged commit 6349b24 into main Sep 30, 2026
18 checks passed
@ss-o
ss-o deleted the feature-34 branch September 30, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature A request for new behavior or capability.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

docs(standard): lifecycle verification omits reload after unload

1 participant