Skip to content

test(helpers): move runtime-image and ci-logging temp dirs onto temporaryDirectory - #3204

Merged
freeqaz merged 1 commit into
mainfrom
test-dry-1022f
Oct 22, 2026
Merged

freeqaz merged 1 commit into
mainfrom
test-dry-1022f

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The runtime image test files, two of their helpers and the ci-logging fixture each create a per-test directory with mkdtemp(join(tmpdir(), prefix)) plus their own t.after(() => rm(...)). temporaryDirectory(t, prefix) in tests/helpers/temporary-directory.mjs does exactly that (test DRY survey, cluster 1). The ci-logging helper also kept a private 30 s poll loop that waitUntil already covers.

Change

  • Nine sites call temporaryDirectory(t, prefix): runtime-image-gateway-workspace (1), runtime-image-gateway-peer (2), runtime-image-native-listener (2), runtime-image-startup.test (2, through the identical helper it already imports), and the helpers temporaryGatewayConfiguration (runtime-image-startup.mjs), createProbeMaterial (runtime-image-model-probe.mjs) and createStartupProbeMaterial (runtime-image-startup-probe.mjs).
  • tests/helpers/ci-logging.mjs: fixture(t) uses temporaryDirectory; the private waitFor export is removed and its one caller (ci-logging-collector) uses waitUntil with the same 30 s timeout, 250 ms interval and message. The file's five unused imports go too, with their eslint-suppressions.json entry.
  • Each hook is registered at the same point as before, so cleanup order against the container hooks is unchanged. Prefixes are unchanged.

Verification

All local runs compare this branch with a main worktree at 5227270, using a runtime image built from 5227270 with podman. "routes sandboxed Git through stock Codex and the repository broker" was skipped: podman networking breaks it here, on main too.

  • Mutants (whole set of 12 affected files, test concurrency 3, the same cases fail on main and on the branch):
    • workspace-node startup phase renamed: the hot-load case fails on both;
    • requiredProfile changed: "validates the configuration dedicated native OpenClaw renders" fails on both;
    • inherited OPENCLAW_LOG_LEVEL kept: "publishes dedicated assets with the Kubernetes gateway entrypoint" fails on both;
    • the ci-logging output directory mode loosened to 0755: "prepareLogging registers an owned Collector backend" fails on both.
    • Under that load, three timing-sensitive cases failed once on one side only (startup-termination SIGTERM, a model-probe outcome, a startup-probe credential case). They flake on main the same way (see pins) and none of them touches a changed line.
    • ci-logging-collector's real smoke cannot bind 172.17.0.1 on this host, on both sides; CI's Logging Collector lane passed on this head.
  • Pins at 2 CPUs (node and each container limited to 2 CPUs):
    • runtime-image-native-listener 20/20; ci-logging + runtime-image-managed-files + runtime-image-state-migration 30/30; runtime-image-startup 19/20 (one podman docker run --rm exit after correct output).
    • runtime-image-gateway-workspace + -gateway-peer: 18/21 on the branch, 14/17 on main. The failures on both sides are the same two peer-outage cases, which see the container stopped instead of exited.
    • runtime-image-startup-probe fails often at 2 CPUs on both sides (5/11 branch, 3/11 main), always in timing cases; runtime-image-startup-termination + -model-probe-outcomes 9/10 on the branch.
    • Median walls: native-listener 112 s, startup 230 s, gateway files 300-350 s (main 290-310 s), startup-probe 264 s (main 256 s).
  • Leak check: completed runs left no directory in the test TMPDIR; the only leftovers came from runs I stopped mid-way.
  • Open PRs: no new conflict for feat(agents): claude code agent runtime for embedded openclaw #3027, feat(runtime): apply Gateway tool policy to dedicated OCE deployments #2633, feat: assign configured OpenClaw roles to Agent users #906 or Fix the OpenShell QA placement timeout and verify auth with the actual model request #3148 (git merge-tree against main and against this branch give the same conflicted files and hunks).
  • Review: internal review approved, no nits. Prettier and ESLint report nothing on the changed files.

Risks

Test-only. Timeout and cleanup behaviour are unchanged. A file that aborts before its after hooks run leaves the directory behind, as before.

…raryDirectory

Cluster 1 of the test-dry survey for the Docker suites: nine per-test
directories in the runtime image startup and model probe files and the
ci-logging fixture now come from temporaryDirectory(t, prefix). The
ci-logging helper's private 30 s waitFor goes too; its one caller uses
waitUntil with the same timeout and interval. The helper's unused imports
and their lint suppression are removed.
@freeqaz
freeqaz requested a review from a team as a code owner October 22, 2026 12:07
@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 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. 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

This PR replaces repeated temporary-directory setup and a private Collector polling loop with existing test helpers.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch is focused and source-verified with passing affected CI lanes; available execution evidence is test-harness evidence.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored, test-only consolidation is outside the ordinary contributor proof gate; affected CI lanes passed, while the body describes local verification as still in progress.
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 maintain repeated temporary-directory cleanup and polling implementations across runtime tests.
Reason: The consolidation reduces maintenance duplication without introducing new abstractions or platform behavior, and the collaborator author has prior merged ownership of these helpers.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it is a useful, focused test cleanup with no concrete correctness finding, and collaborator-authored work remains subject to maintainer triage.

Priority: P3
Reviewed head: 51fdcfab91583e38f150bc5c4ef031eec2f52911

Before merge

None.

Findings

None.

Agent review details

How this fits together

Runtime-image tests and the CI logging fixture create temporary configuration files, exercise containers, and remove their files through test cleanup hooks.

flowchart LR
  A[Integration tests] --> B[Shared test helpers]
  B --> C[Temporary configuration files]
  C --> D[Runtime and Collector containers]
  A --> E[Test cleanup hooks]
  E --> C
Loading

Technical review

Best possible solution:

Retain this focused consolidation around the existing cleanup and polling helpers.

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

No product bug is claimed; source inspection confirms the repeated setup and private polling loop being consolidated.

Is this the best way to solve the issue?

The patch uses existing owners, removes duplicate code, and preserves the relevant cleanup and polling contracts.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Temporary-directory lifecycle keeps the original intent (test(helpers): add a test-scoped temporaryDirectory helper #3174: Provide test-scoped cleanup through a shared helper while retaining self-contained fixtures where required.)
  • Collector polling keeps the original intent (2268b0b: Wait for real Collector JSONL output within a bounded interval; the replacement keeps the predicate and configured timing while adopting the shared waiter's final-read convention.)
  • Lint suppression keeps the original intent (046e12b: Record existing lint findings and prune entries when their causes are removed.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Pinned introduced change: The introduced patch changes only test helpers, existing integration tests, and removal of an unused-import suppression; no production implementation changes. (51fdcfab9158)
  • Existing cleanup owner: temporaryDirectory creates the same prefixed directory and registers recursive forced removal with the supplied test context; migrated callers retain their position relative to container cleanup registration. (tests/helpers/temporary-directory.mjs:30, 51fdcfab9158)
  • Collector polling contract: The Collector caller retains its truthy canary predicate, 30-second timeout, 250-millisecond interval, and timeout description; the shared waiter performs a final read at or after its deadline. (tests/integration/ci-logging-collector.test.mjs:63, 51fdcfab9158)
  • Established consolidation direction: Merged test(helpers): add a test-scoped temporaryDirectory helper #3174 introduced the shared directory helper and explicitly retained the self-contained repository-credentials variant; this PR reuses that existing variant where already imported. (tests/fixtures/repository-credentials/process.mjs:135, e62872a3cfc4)
  • Reviewed-head CI: CI Required, Logging Collector, Image Model Probes, and all three Image Runtime Startup lanes passed for the reviewed head; First Agent Smoke remained in progress. (51fdcfab9158)
  • Policy and discussion: Read the complete root AGENTS.md; no nested test instructions or maintainer-notes directory were present. The discussion contains only review-status comments. The requested single-reviewer restriction overrides the bundled autoreview-helper instruction. (AGENTS.md, 51fdcfab9158)

Likely related people:

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

Labels

Label changes:

  • add P3: This is low-risk test-maintenance work with no production behavior change.
  • 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 work with no production behavior change.
  • 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, 8:19 AM ET / 12:19 UTC.

@freeqaz
freeqaz merged commit 03170cc into main Oct 22, 2026
63 checks passed
@freeqaz
freeqaz deleted the test-dry-1022f branch October 22, 2026 14:09
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