Skip to content

refactor(tests): reuse setup runtime fixtures - #139920

Merged
steipete merged 1 commit into
mainfrom
codex/test-setup-runtime-fixtures
Sep 6, 2026
Merged

refactor(tests): reuse setup runtime fixtures#139920
steipete merged 1 commit into
mainfrom
codex/test-setup-runtime-fixtures

Conversation

@steipete

@steipete steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Setup command tests repeat the same three-spy runtime fixture in 17 places, adding maintenance work without distinct scenario coverage.

Why This Change Was Made

Reuse the existing command test runtime factory at each original allocation site. This removes 35 net test lines while preserving every input, assertion, and case. The helper stays unchanged; it creates fresh spies, has no output writers, and keeps exit non-throwing.

User Impact

No production behavior change. Setup tests keep their existing coverage with less repeated fixture code.

Evidence

All 64 setup and CLI setup-registration cases pass before and after. Full changed-file checks and independent review through P2 pass. Exact replacement normalization confirms all source outside the 17 literals and added import is unchanged. The helper's exit arity and explicit default implementation differ from an empty spy, but setup and these tests do not inspect either distinction.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026 •
edited
Loading

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.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: S maintainer Maintainer-authored PR labels Sep 6, 2026
@steipete
steipete marked this pull request as ready for review September 6, 2026 08:04
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 4:10 AM ET / 08:10 UTC.

ClawSweeper review

What this changes

The setup-command tests reuse the existing runtime-spy factory at 17 allocation sites while preserving their scenarios and assertions.

Merge readiness

✅ Ready for maintainer review

Keep open: this is a focused, behavior-preserving cleanup that remains absent from current main. No actionable correctness findings were identified; the maintainer-authored, protected PR is not eligible for automatic closure.

Priority: P3
Reviewed head: 283516bde0f2f87ea00608e07d06b33ebade6641

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A straightforward cleanup with preserved coverage, compatible fixture behavior, and reported passing focused checks.
Proof confidence 🌊 off-meta tidepool Not applicable: The maintainer-authored PR changes only test fixture construction; production setup behavior is unchanged, and the supplied test results are proportional supplemental validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The maintainer-authored PR changes only test fixture construction; production setup behavior is unchanged, and the supplied test results are proportional supplemental validation.
Evidence reviewed 6 items Pinned introduced change: The complete merge-base-to-head patch replaces 17 runtime literals and adds one import; it changes no assertions, scenario inputs, production files, or helper implementation.
Fixture behavior: createTestRuntime allocates fresh log, error, and exit spies on each invocation. Exit returns undefined without throwing, and the returned object has no output writers. The setup tests do not inspect exit arity or its implementation identity.
Production consumer contract: Invalid configuration calls runtime.exit(1) and explicitly returns; structured output falls back to runtime.log when no output writer exists. Both behaviors remain compatible with the shared fixture.
Findings None None.
Security None None.

How this fits together

Baseline setup initializes OpenClaw configuration, workspace, and session directories. These tests supply temporary configuration and runtime spies to check setup results and failure handling.

flowchart LR
  A[Setup test scenarios] --> B[Temporary configuration]
  A --> C[Fresh runtime spies]
  B --> D[Baseline setup command]
  C --> D
  D --> E[Configuration and directory results]
  D --> F[Logs and exit calls]
  E --> G[Existing assertions]
  F --> G
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Tests -35. Total -35 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 18 53 -35
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 18 53 -35

Review metrics

None.

Technical review

Best possible solution:

Keep setup coverage intact while using the existing fresh-spy factory at each original allocation site.

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

Not applicable: this PR consolidates test fixtures rather than fixing a reported runtime defect.

Is this the best way to solve the issue?

Yes: reusing the existing factory removes repetition without adding abstractions, sharing mutable spies between cases, or weakening assertions.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P3: This reduces repeated test setup without changing production behavior or coverage.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored PR changes only test fixture construction; production setup behavior is unchanged, and the supplied test results are proportional supplemental validation.

Label justifications:

  • P3: This reduces repeated test setup without changing production behavior or coverage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored PR changes only test fixture construction; production setup behavior is unchanged, and the supplied test results are proportional supplemental validation.

Evidence

What I checked:

  • Pinned introduced change: The complete merge-base-to-head patch replaces 17 runtime literals and adds one import; it changes no assertions, scenario inputs, production files, or helper implementation. (src/commands/setup.test.ts:11, 283516bde0f2)
  • Fixture behavior: createTestRuntime allocates fresh log, error, and exit spies on each invocation. Exit returns undefined without throwing, and the returned object has no output writers. The setup tests do not inspect exit arity or its implementation identity. (src/commands/test-runtime-config-helpers.ts:57, 283516bde0f2)
  • Production consumer contract: Invalid configuration calls runtime.exit(1) and explicitly returns; structured output falls back to runtime.log when no output writer exists. Both behaviors remain compatible with the shared fixture. (src/commands/setup.ts:164, 283516bde0f2)
  • Current-main necessity: Current main retains the repeated runtime literals. Comparing the three relevant files shows only the proposed setup-test cleanup; the production owner and shared helper match. No implemented-on-main or shipped-fix claim is warranted. (src/commands/setup.test.ts:62, 976fde6ae1d0)
  • Area routing history: Available main history shows repeated helper maintenance by Peter Steinberger, including fixture sharing and helper consolidation. Deeper blame and parent-diff inspection encountered unavailable objects and failed network access, so this establishes routing relevance rather than source-line introduction. (src/commands/test-runtime-config-helpers.ts, 976fde6ae1d0)
  • Captured validation and discussion: The complete supplied PR body reports 64 setup and setup-registration cases passing before and after, plus changed-file checks. Captured checks include successful test-type and changed-node checks. No substantive review comments or related fixing PR were supplied. Tests were not executed during this read-only review. (283516bde0f2)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit f896071 into main Sep 6, 2026
167 checks passed
@steipete
steipete deleted the codex/test-setup-runtime-fixtures branch September 6, 2026 08:14
@steipete

steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@steipete steipete mentioned this pull request Sep 6, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 7, 2026
@steipete
refactor(tests): reuse setup runtime fixtures (openclaw#139920)
6928c0b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

commands Command implementations maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete