Skip to content

Reuse the loaded module in tests instead of force-importing it 165 times - #133

Merged
juemerson-at-purestorage merged 14 commits into
dmann000:mainfrom
juemerson-at-purestorage:perf/test-module-import-helper
Aug 20, 2026
Merged

juemerson-at-purestorage merged 14 commits into
dmann000:mainfrom
juemerson-at-purestorage:perf/test-module-import-helper

Conversation

@juemerson-at-purestorage

Copy link
Copy Markdown
Collaborator

Reuse the loaded module in tests instead of force-importing it 165 times

The problem

Test containers loaded the module in their BeforeAll like this:

Import-Module $manifest -Force

-Force discards the loaded module and rebuilds it, re-dot-sourcing the 574 .ps1 files
under Public/ and Private/. That costs roughly 4.2 s. Pester runs the whole suite in one
host process, so 165 containers each paid it — and 164 of those rebuilds were of an
already-loaded, byte-identical module.

The change

Tests/PfbTestModule.ps1 provides Import-PfbTestModule, which reuses the loaded instance
and resets the volatile module-scope state instead of rebuilding. Every converted container now
has two lines in its BeforeAll:

. (Join-Path $PSScriptRoot 'PfbTestModule.ps1')
$null = Import-PfbTestModule

The rewrite was applied mechanically by tools/Update-PfbTestModuleImport.ps1, an AST rewriter
that splices the replacement over the Import-Module command node rather than editing lines.
It is idempotent and re-runnable: on this tree -WhatIf reports
Changed=0 Unchanged=195 Excluded=1 Unrecognised=0, which sums to all 196 *.Tests.ps1 files.

-Force equivalence is the whole correctness question, and it is what the helper's reset block
buys. All five volatile module-scope variables are cleared unconditionally on every call:

$script:PfbDefaultArray = $null
$script:PfbArrays = @{}
$script:PfbCachedCredential = $null
$script:PfbCapabilityMap = $null
$script:PfbVersionMap = $null

An earlier revision cleared the two JSON caches only when the module root changed, on the
grounds that they are read-only and safe to reuse. That was reversed. The condition keyed off
the module root, which is one of several ways a stale cache arrives — a test that populates
$script:PfbCapabilityMap directly leaves it warm and visible to the next container, which is
precisely the order-dependent leakage -Force used to make impossible. The reset must not
become conditional again; Tests/PfbTestModule.ps1 says so at the site.

Scope

175 files: 172 under Tests/, two new files under tools/, and one under scripts/.

Nothing under Public/ or Private/ is touched, and neither the manifest nor the root
module is.
That is the basis on which the repo owner waived live-FlashBlade verification for
this change — it cannot alter what goes on the wire. The claim was verified mechanically rather
than asserted: the file list contains no .psm1, no .psd1, and nothing under Data/,
Reports/ or .github/. Both new tools/ files are dot-sourced by exactly three consumers,
no wildcard tools/lib/*.ps1 dot-source picks them up, and none of their eight function names
collides with an existing definition.

The scripts/ file is a deliberate addition to the original scope — see the CI gate below.

What stops this silently regressing

The helper's module-identity check fails closed. If the ModuleBase path spelling it
compares against ever stops matching, every container force-imports anyway, the whole suite
stays green, the coverage gate stays green, and the entire saving evaporates with nothing able
to detect it. A performance change whose failure mode is "still passes, just slow again" needs
an instrument, not a test.

So there are two, and both are new:

Tests/TestModuleImportGuard.Tests.ps1 — pure AST over tracked .ps1 files, so it needs
neither the spec cache nor PowerShell 7 and runs ungated on both editions. Three Its catch a
raw -Force manifest import returning, a test file that uses the module without loading it
through the helper, and a new module-scope $script: variable the helper does not know to
reset. A fourth asserts the guard's allowlist and the rewriter's exclusion set are equal as
sets, so the two cannot drift apart in the direction that hides a real offender.

A force-import ratio gate in scripts/Invoke-PfbCiPester.ps1 — emits both counters
unconditionally into every CI job log, then fails the run if
force-imports >= calls / 10.

The gate is a ratio rather than a small absolute number on purpose. The healthy force count is
an enumeration of legitimate contributors — one cold import per run, about four from
Tests/PfbTestModule.Tests.ps1 (which force-imports deliberately, because the force path is
its subject), and one recovery per selector-harness container that installs an unmarked
instance — so a tight pin would red on ordinary maintenance. The failure it exists to catch is
not a small drift: it drives force-imports up to roughly equal calls. An order-of-magnitude
gate separates those two regimes and still tolerates a new harness file.

The threshold is not marginal, and it is measured rather than estimated — every pwsh leg reports
183 calls / 7 force-imports and the 5.1 leg 182 / 5, against limits of 18.3 and 18.2.
That is 2.6x and 3.6x headroom. Put the other way: at 7 force-imports the call count would have
to fall below 70 for this to false-red, so a leg would need to stop running roughly two-thirds
of its containers first.

The ubuntu and macOS legs matter here specifically, because the fail-closed hazard is a
ModuleBase path-spelling mismatch and those runners could plausibly have differed. They do not
— all three pwsh legs report the same 183 / 7.

Adding this expanded the change into scripts/, which the branch otherwise leaves alone. The
justification is that the original design deferred the assertion on the belief that the
counters could not be read after Invoke-Pester returns. That belief was wrong:
Run.Exit = $false is set deliberately so the coverage gate is not skipped, and
Invoke-Pester, the gate call and exit all run in one process with the globals in scope.
Without the assertion, the entire value of this PR can evaporate and no test can see it.

Verification

CI on this branch, all four matrix legs green
(run 32340782815):

leg passed failed skipped helper calls / force imports
windows-latest, pwsh 3301 0 2 (ceiling 8) 183 / 7
windows-latest, WinPS 5.1 3006 0 297 (ceiling 313) 182 / 5
ubuntu-latest, pwsh 3301 0 2 (ceiling 8) 183 / 7
macos-latest, pwsh 3301 0 2 (ceiling 8) 183 / 7

Every leg passes both the coverage gate and the new force-import ratio gate. The three pwsh legs
report identical counters, and both Windows legs match a local full-suite run exactly (596.38 s
on pwsh7, 226.05 s on 5.1 locally).

On every leg, expected base equalled loaded base(s) on every logged line, so the fail-closed
condition is absent rather than merely unobserved.

A note for anyone reading a full run log: Tests/CiCoverageGate.Tests.ps1 prints several
COVERAGE GATE FAILED (issue #63) blocks as it exercises the gate's own failure paths on
purpose. The real verdicts are the Coverage gate passed lines at the end.

Because the risk this change introduces is order-dependent state leakage between containers,
that was exercised directly rather than inferred from a green suite: a 26-file subset run five
times — control, three seeded permutations, and one hand-built adversarial order placing a
module-root redirector immediately upstream of the cache consumers — on both editions, all
green. The permutations were confirmed to have actually taken effect by checking that the
sequence of force-import reasons differed between them, since five identical green runs would
otherwise be consistent with Pester ignoring the ordering entirely. 21 files were additionally
run standalone, and the grouped totals reconcile exactly to the sum of the isolated totals on
both editions, so grouping neither skipped nor silently added tests.

Disclosures

The recovered-time figure is an estimate, not a controlled measurement. The 596.38 s and
226.05 s above are measured on this tree, but the "before" figure comes from the originating
analysis rather than a run on the same machine. The saving is inferred from the force-import
count — 7 forced imports where there were once 165, which is solid — but this is not a
before/after benchmark on identical hardware.

The 5.1 skip ceiling was raised 273 → 313, and it absorbs +6 of drift that is not from this
branch.
The branch was cut before #112 landed, so its own raise (268 → 308) collided with
#112's (268 → 273) on rebase. The two are additive rather than competing, because they gate
different files, and the total is attributed exactly:

252 the figure that justified 268, from run 31830362870
+5 Build-PfbCapabilityMap.ContextScopeDrift.Tests.ps1, from #112
+34 Update-PfbTestModuleImport.Tests.ps1, added here — PS7-gated because the tool it tests declares #Requires -Version 7.0
+6 Build-PfbDeadKeyReport.Tests.ps1, from neither branch
297 expected, and exactly what CI measured

The +6 is the part worth a maintainer's eye. It landed in #120 two days after the ceiling was
last set; #120 still had slack and passed without touching this file, so 268 was already stale
against main before either branch existed. Whether folding those 6 in here is acceptable or
should be split out is your call. 313 keeps the standing +16 headroom over 297.

That collision is a symptom rather than a one-off, and #132 proposes the fix: a per-file
expected-skip map, so movement reds the PR that caused it and names the file, instead of being
absorbed by headroom and billed to a later innocent PR.

Live-FlashBlade verification was waived by the repo owner for this change, on the basis that
it touches only CI test scaffolding and no cmdlet behaviour. The scope section above is the
evidence for the second half of that.

No version bump and no CHANGELOG.md entry — those are separate maintainer decisions.

juemerson-at-purestorage and others added 14 commits August 19, 2026 23:26
Pin the offset contract Task 3 splices on, and correct four stale or
inaccurate comments in the single-definition library.

- Assert StartOffset/EndOffset/Text in the existing multi-line backtick-
  continuation test: ReadAllText().Substring(Start, End - Start) must equal
  the returned Text, and Text must begin with Import-Module and end with
  -Force. That last pair is what proves the extent spans the whole two-line
  command rather than truncating at the continuation.
- Correct the doc comment: the offsets are CHARACTER offsets into the decoded
  file, not byte-exact. State that a consumer must splice a decoded string
  (ReadAllText + Substring) and never index a byte array.
- Say six known call-site forms, not five, and enumerate them.
- Describe what Get-PfbTestImportAst actually returns (the AST root, not the
  imports and not a CommandAst list). Not renamed: Tasks 3 and 5 are written
  against the current name.
- Assert that a positive Test-PfbTestModuleUsage case names its reason.

Left as accepted by design: NestedJoinPath absorbing any Join-Path spelling
containing the manifest leaf name, and the per-spec silent drops for a
differently-named variable argument and the ipmo alias.

Scoped Pester: 17/17 passed under pwsh 7 and Windows PowerShell 5.1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…de entry

- Log line now names the loaded base(s) and distinguishes 'no instance'
  from a path-spelling mismatch.
- Correct the healthy force-count documentation to an enumeration plus a
  FORCE < CALLS / 10 ratio gate.
- Invalidate the two JSON caches only when the incoming module root is not
  the real module base, so warm caches no longer depend on four other
  files' unasserted AfterEach restores.
- Initialise the global counters defensively for StrictMode callers.
- Tests: save/restore both caches, cover both cache-warmth directions, and
  cover -ManifestPath.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rewriter hardcoded the assignment target of a -PassThru import and moved
the left edge of its splice back to the line's indentation, so
`$myOwnName = Import-Module ... -PassThru` came out renamed to
`$script:module = ...` and any earlier statement on the same line was deleted
silently. It now replaces the enclosing AssignmentStatementAst (target copied
verbatim from its left-hand side) or the single-element PipelineAst, and
reports anything in another syntactic position as Unrecognised.

It also had no exclusion mechanism, so it rewrote the raw -Force import in
Tests/PfbTestModule.Tests.ps1 that exists precisely to install an unmarked
module instance for the helper's rebuild-detection test. Excluded files are
checked before the file is read and reported in their own bucket.

Also: enumerate recursively, sort ordinally rather than by culture, take the
spliced newline from the preceding line so a mixed-terminator file keeps its
mix, and cover all six call-site forms with fixtures.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve-PfbImportSplice guarded the pipeline arity and the assignment
operator but never checked the replaced node's OWN parent, so four nested
shapes were rewritten instead of reported Unrecognised: an assignment in
an if-condition (emitting a file that does not parse), a chained
assignment, and a $( ) / @( ) expression body (all three parsing while
silently leaving the caller's variable empty). Both arms now require the
replaced node to sit in statement position, via one shared predicate.

Separately, an import carrying any parameter beyond Force/PassThru/Name
was rewritten with that parameter silently dropped (-Global,
-ErrorAction Stop). It is now rejected in the rewriter rather than
supported; reproducing extra parameters, or widening the shared
predicate, is a maintainer design call.

Zero sites in the tree match either case: -WhatIf is unchanged at
Changed=165, Unchanged=28, Excluded=1, Unrecognised=0.

Fixtures: one per nested shape and per extra-parameter case, each
asserting Unrecognised plus a byte-identical file, and one pinning a
semicolon-joined FOLLOWING statement surviving a rewrite. 26 -> 33.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured 292 skipped on a local Windows PowerShell 5.1 full-suite run
(2983 passed / 0 failed), against the 268 ceiling. Attributed exactly by a
per-file tally of the run's skip markers:

  +34  Update-PfbTestModuleImport.Tests.ps1, added by this branch. The
       rewriter it tests declares #Requires -Version 7.0, so the PS7 gate is
       correct; the file runs in full on 7, where the leg measured 2 skipped
       against its ceiling of 8.
  +6   Build-PfbDeadKeyReport.Tests.ps1, which is not from this branch. It
       landed in 302f9e4 (dmann000#120) on 2026-08-16, two days after 364a0fe set the
       ceiling to 268. So 268 was already stale against main.

308 restores the standing +16 headroom over measured.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@juemerson-at-purestorage
juemerson-at-purestorage merged commit 2157452 into dmann000:main Aug 20, 2026
6 checks passed
juemerson-at-purestorage added a commit to juemerson-at-purestorage/fb-powershell that referenced this pull request Aug 20, 2026
Adopt the shared module imports in the new regression suites so this branch remains compatible with PR dmann000#133.

Co-Authored-By: Claude <noreply@anthropic.com>
@juemerson-at-purestorage
juemerson-at-purestorage deleted the perf/test-module-import-helper branch August 25, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant