Skip to content

test(helpers): add a test-scoped temporaryDirectory helper - #3174

Merged
freeqaz merged 2 commits into
mainfrom
test-dry-1022b-tmp
Oct 22, 2026
Merged

freeqaz merged 2 commits into
mainfrom
test-dry-1022b-tmp

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The most copied test setup is a temporary directory: mkdtemp(join(tmpdir(), prefix)) plus
t.after(() => rm(directory, { recursive: true, force: true })), with its three imports, in about 110 files.
tests/helpers/temporary-directory.mjs only offers a file-scoped directory, so tests with a context keep
writing the pair by hand, and two files carry a private temporaryDirectory(t, ...).

Change

Test-only. tests/helpers/temporary-directory.mjs gains temporaryDirectory(t, prefix): a new directory under
TMPDIR, removed when the test (or suite) that owns t ends.

First batch, one suite: images-packaging (the fewest open PRs among suites with copies). Migrated:
container-publication-workflow, repository-runtime-volume, production-image-qualification (3 sites) and
oidc-ca-trust's private copy. Later batches go suite by suite.

Not migrated: tests/fixtures/repository-credentials/process.mjs keeps its own temporaryDirectory, because the
isolation fixture mounts that directory alone into containers where tests/helpers is absent.

Verification

Run on a remote test VM; "main" is the PR's merge base.

  • Mutants in the code under test, run on main and on the branch, give identical kill sets: node-platform check
    accepting two platforms (production-image-qualification), publication summary dropping the image result
    (container-publication-workflow), CA bundle assembly accepting a private key (oidc-ca-trust); a Harness trust-bundle
    mutant survives on both.
  • The 4 files 20/20 at 2 CPUs via the pin runner (repository-runtime-volume's one test skips there without a runtime
    image, as on main).
  • Wall time: median 23.5 s per pin run on the branch (20 runs), 24 s on main (10 runs).
  • Internal review: approve with nits; the comment nit applied (the helper needs a test context, not a suite one).

Risks

Test-only and mechanical: the same directory prefixes, the same removal on t.after.

…ackaging

tests/helpers/temporary-directory.mjs gains temporaryDirectory(t, prefix): a new
directory under TMPDIR removed when the owning test ends. First batch of the temp-dir
cluster, the images-packaging suite (fewest open PRs among suites with copies):
container-publication-workflow, repository-runtime-volume,
production-image-qualification (3 sites) and oidc-ca-trust (its private copy).
@freeqaz
freeqaz requested a review from a team as a code owner October 22, 2026 03:41
@clawsweeper

clawsweeper Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:feature Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:feature Preliminary submission classification; full source review pending. labels Oct 22, 2026
@clawsweeper

clawsweeper Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

Adds a test-scoped temporary-directory helper and replaces eight equivalent setup-and-cleanup sequences across four integration files.

Example: A publication-summary test needs an oce-publication-summary- temporary directory.

  • Before: The test creates the directory and separately registers recursive removal with t.after.
  • After: temporaryDirectory(t, "oce-publication-summary-") performs the same creation and cleanup registration.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A small, correct extraction supported by source equivalence, passing head checks, and reported harness runs; supplied evidence does not establish a fresh real-runtime volume run.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored test-only extraction is outside the ordinary shipped-behavior proof gate; the body reports repeated runs and unchanged mutation outcomes, explicitly noting the runtime-volume skip.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Contributors repeat temporary-directory acquisition and teardown code across integration tests.
Reason: The maintainer-authored direction consolidates existing behavior in its current owner with a small net reduction in code.

Merge readiness

✅ Ready for maintainer review

Keep open: this maintainer-authored PR is a focused test refactor with no actionable correctness findings.

Priority: P3
Reviewed head: 60821d1741fd4ff5c20b80012e202fbb49638c69

Before merge

None.

Findings

None.

Agent review details

How this fits together

The helper belongs to the test infrastructure: callers supply a Node test context and directory prefix, receive a temporary path, and remove that directory through the owning test's teardown.

flowchart LR
  A[Integration test] --> B[Temporary directory helper]
  B --> C[Directory under TMPDIR]
  C --> D[Test fixture operations]
  B --> E[Test teardown hook]
  E --> F[Recursive directory removal]
Loading

Technical review

Best possible solution:

Keep the focused extraction in the existing helper module while preserving test-scoped and file-scoped lifetimes.

Do we have a high-confidence way to reproduce the issue?

This is a refactor rather than a bug report; source inspection verifies the duplicated sequences and their equivalent replacements.

Is this the best way to solve the issue?

The extraction is appropriately small, uses the existing helper owner, and retains the separate container-isolation fixture whose mounted filesystem excludes shared helpers.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against c8e5e2a9d412.

Provenance checked

  • File-scoped temporary-directory cleanup keeps the original intent (fix(test): remove profile and console browser scratch dirs after runs #2967: Remove leaked profile and browser scratch directories while preserving files until their consumers finish.)
  • OIDC and repository-volume temporary fixtures keeps the original intent (cb63275: Provide isolated scratch material for CA-trust integration scenarios; repository-volume history independently retains per-test fixture ownership.)
  • Publication and image-qualification temporary fixtures keeps the original intent (070ac36: Materialize qualification inputs with test-owned cleanup; later publication and metadata-regression changes use the same lifecycle.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Pinned patch preserves behavior: All eight replacements preserve their prefixes, asynchronous directory creation, and recursive forced removal registered on the same test context; assertions are unchanged. (tests/helpers/temporary-directory.mjs:30, 60821d1741fd)
  • Existing helper ownership and intent: The existing file-scoped helper remains intact; its originating contribution, fix(test): remove profile and console browser scratch dirs after runs #2967, explains why callers without a test context require file-lifetime cleanup. (tests/helpers/temporary-directory.mjs:19, 556aaf49f36b)
  • Current main still has the duplication: Fetched main exposes only fileTemporaryDirectory; its separate listen-helper refactor changes server lifecycle setup without replacing these directory sequences. (tests/helpers/temporary-directory.mjs:21, c8e5e2a9d412)
  • Head validation and author direction: GitHub confirms COLLABORATOR authorship and the reviewed head; Images and Packaging, Static Checks, Suite Audit, and CI Required pass, while First Agent Smoke remains in progress. (60821d1741fd)
  • Policy and inspection limits: Read the complete root policy; no nested tests AGENTS.md, maintainer-notes directory, or pr-simplify skill exists. Some historical object reads fail because lazy fetching is disabled; available path history, blame, base blobs, and originating PR context establish the relevant intent. No tests or review helpers were executed, and the checkout remains clean. (AGENTS.md, 60821d1741fd)

Likely related people:

  • Free Wortley: Raw commit 556aaf4 adds tests/helpers/temporary-directory.mjs:20 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 556aaf49f36b; files: tests/helpers/temporary-directory.mjs)

Review metrics

Metric Value Why it matters
Change footprint Production +0/-0; test infrastructure +25/-27 Five existing files consolidate eight cleanup sequences without adding test cases or changing assertions.

Labels

Label changes:

  • add P3: This is low-risk test maintenance with no changed product behavior.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is low-risk test maintenance with no changed product behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 22, 2026, 12:02 AM ET / 04:02 UTC.

@freeqaz
freeqaz merged commit e62872a into main Oct 22, 2026
69 checks passed
@freeqaz
freeqaz deleted the test-dry-1022b-tmp branch October 22, 2026 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant