Skip to content

test(scripts): run OpenClaw formatter tests without vitest#3063

Merged
senamakel merged 2 commits into
tinyhumansai:mainfrom
alexzhu0:codex/OH-2611-tools-generator-node-test
Jun 2, 2026
Merged

test(scripts): run OpenClaw formatter tests without vitest#3063
senamakel merged 2 commits into
tinyhumansai:mainfrom
alexzhu0:codex/OH-2611-tools-generator-node-test

Conversation

@alexzhu0
Copy link
Copy Markdown
Contributor

@alexzhu0 alexzhu0 commented May 31, 2026

Summary

  • Replaces the tools-generator formatter test's vitest import with Node's built-in node:test runner.
  • Adds a tiny local expect compatibility helper so the existing assertions stay unchanged.
  • Keeps the change scoped to the root script test without adding a new root dev dependency.

Problem

  • scripts/tools-generator/__tests__/openClaw-formatter.test.js imports from vitest.
  • vitest is installed under the app workspace, not the repository root where this script test lives.
  • Running the test directly from the repository root fails with ERR_MODULE_NOT_FOUND before the formatter assertions can execute.

Solution

  • Use node:test and node:assert/strict, matching the no-dependency pattern used by other root script tests.
  • Preserve the existing test cases and assertion call sites with a local helper that covers the matchers already used in this file.
  • Avoid adding or moving dependencies for a single script-only test.

Submission Checklist

If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • N/A: Diff coverage gate was not run because this only updates a root Node script test and local Rust tooling is unavailable.
  • N/A: Coverage matrix was not updated because this does not add, remove, or rename a product feature.
  • N/A: No affected feature IDs from the coverage matrix.
  • No new external network dependencies introduced (mock backend used per Testing Strategy)
  • N/A: Manual smoke checklist not affected; this does not touch release-cut surfaces.
  • N/A: Related to [Automated] Weekly code-review report — 2026-05-25 #2611 but does not close it by itself.

Impact

  • Runtime/platform impact: none.
  • Test/dev impact: the OpenClaw formatter test can run from the repository root without relying on app workspace test dependencies.
  • Dependency impact: no new dependency.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: codex/OH-2611-tools-generator-node-test
  • Commit SHA: dfe5faf

Validation Run

  • pnpm --filter openhuman-app format:check (blocked after app Prettier passed; see below)
  • pnpm typecheck
  • Focused tests: node --test scripts/tools-generator/__tests__/openClaw-formatter.test.js
  • Rust fmt/check (if changed): N/A, no Rust files changed
  • Tauri fmt/check (if changed): N/A, no Tauri files changed
  • node scripts/codex-pr-preflight.mjs --lightweight
  • git diff --check

Validation Blocked

  • command: env COREPACK_HOME=/Users/alex/PR/.corepack PNPM_HOME=/Users/alex/PR/.pnpm-home pnpm --filter openhuman-app format:check
  • error: sh: cargo: command not found
  • impact: The app Prettier phase passed, but Rust formatting and the pre-push hook cannot complete in this local environment until Cargo is installed.

Behavior Changes

  • Intended behavior change: none.
  • User-visible effect: none.

Parity Contract

  • Legacy behavior preserved: formatter assertions and fixtures remain unchanged.
  • Guard/fallback/dispatch parity checks: N/A, no runtime dispatch path changed.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none found during triage
  • Canonical PR: this PR
  • Resolution (closed/superseded/updated): N/A

Constraint: Root script tests should run with Node's built-in test runner and avoid app-only devDependencies.

Rejected: Add vitest to the repo root | broadens dependency surface for one script test.

Confidence: high

Scope-risk: narrow

Directive: Keep script-only tests executable from the repository root without app workspace packages.

Tested: node --test scripts/tools-generator/__tests__/openClaw-formatter.test.js; node scripts/codex-pr-preflight.mjs --lightweight; pnpm typecheck; git diff --check

Not-tested: Full format:check/pre-push hook; cargo is not installed in this environment.
@alexzhu0 alexzhu0 requested a review from a team May 31, 2026 05:10
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 31, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The test file migrates from vitest assertion utilities to Node's built-in node:assert/strict and node:test modules. A local expect wrapper function is added to provide the familiar assertion API (toBe, toContain, toHaveLength, toHaveProperty) by delegating to Node's strict assert module.

Changes

Vitest to Node Built-in Test Framework Migration

Layer / File(s) Summary
Import vitest and add expect wrapper
scripts/tools-generator/__tests__/openClaw-formatter.test.js
Switches describe/it imports and assertion source from vitest to node:test and node:assert/strict; adds a local expect wrapper function that implements toBe, toContain, toHaveLength, and toHaveProperty by delegating to assert methods.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A humble bunny hops with glee,
The tests now run on Node, you see!
No vitest needed, built-in and bright,
With tiny assertions, all feels right. 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: replacing vitest with Node's built-in test runner for the OpenClaw formatter tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@alexzhu0
Copy link
Copy Markdown
Contributor Author

Thanks for the checks. I rechecked this PR on current state:

  • The only true blocker is Rust Core Coverage (cargo-llvm-cov) failing.
  • Other core checks in this PR pass; remaining failures are likely rerun / shared-suite related.
  • This is likely a shared upstream/core coverage issue across the fix(codex): compare strict preflight paths by realpath #3060-3073 batch, not a regression from this PR's diff.
  • PR Submission Checklist has historical failed/cancelled runs due to workflow re-triggers.

@alexzhu0
Copy link
Copy Markdown
Contributor Author

I reviewed all open PRs from alexzhu0 in this batch (#3060-3073).

Clarification on PR Submission Checklist

Most PR Submission Checklist failure/cancellation entries are historical/retry artifacts rather than new functional regressions:

  • Multiple checklist runs are expected for these PRs because re-runs happen while soft checks are triggered.
  • In the same PRs, CodeRabbit, lint/format, and most pipeline checks are passing.
  • The remaining hard blockers are concentrated in Rust Core Coverage (cargo-llvm-cov) and, for a subset, Frontend Coverage (Vitest) / Playwright web lane 1.

Suggested action

Please treat PR Submission Checklist failures for this batch as noise unless accompanied by a fresh diff-scope failure in core quality gates.

This should help avoid blocking PRs that are not functionally regressed by their own changes.

@sanil-23 sanil-23 assigned sanil-23 and unassigned sanil-23 May 31, 2026
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

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

@alexzhu0 the code looks good — clean, focused fix for a real problem (vitest not available at the repo root). the local expect compat wrapper is the right call: it covers exactly the matchers the existing tests use, and node:assert/strict gives proper strict-equal semantics for toBe.

one thing to note: Rust Core Coverage is failing in CI, but this PR only touches a JS script test with zero Rust changes, so that failure looks pre-existing rather than caused by this change. if you can confirm it was already failing on main, i'll come back and approve once the CI picture is clear. let me know if you need help tracking it down.

Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

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

@alexzhu0 the code still looks clean — no new findings from the merge commit. CI checks are re-running after the upstream/main merge. I'll come back and approve once all checks are green. Nothing to change on your end.

Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

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

@alexzhu0 quick CI update — the checks have now completed after your upstream merge:

  • Rust Core Coverage: failing on connectivity_raw_coverage_e2e and tools_channels_raw_coverage_e2e. These are Rust integration tests with no connection to your JS-only change. Both were already red before this PR and are pre-existing failures in the codebase.
  • E2E Playwright lane 1/4: one of four shards failed. The other three passed. This looks like an infra flake given the sharded setup and the fact that the other three lanes are green.

The code itself remains clean — no issues introduced by this PR. Once the pre-existing Rust coverage failures are resolved (or confirmed as OK to ignore by the team) and CI is fully green, I'll come back and approve.

@senamakel senamakel merged commit 51a1d1b into tinyhumansai:main Jun 2, 2026
17 of 19 checks passed
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.

3 participants