fix: resolve ESLint error and speed up gateway recovery tests#1223
Conversation
- snapshot.test.ts: replace forbidden import() type annotation in importOriginal generic with a top-level type-only import - onboard.js: make health-poll count and interval configurable via NEMOCLAW_HEALTH_POLL_COUNT and NEMOCLAW_HEALTH_POLL_INTERVAL env vars (defaults unchanged for production) - cli.test.js: set poll count=1 and interval=0 in test helper to eliminate ~40s of unnecessary sleep in gateway recovery tests - cli.test.js: reduce test timeouts from 25s to 10s accordingly - Apply Prettier formatting fixes from pre-commit hooks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGateway health-check polling loops were made configurable via environment variables; Vitest fs mocks were re-typed to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nemoclaw/src/blueprint/state.test.ts (1)
9-10: Consider using the same pattern as snapshot.test.ts for consistency.This file uses type inference for
importOriginal(), whilesnapshot.test.tsuses an explicitimport type fs from "node:fs"followed byimportOriginal<typeof fs>(). Using the same pattern across all three files would improve consistency and align with the approach described in the PR summary.♻️ Align with snapshot.test.ts pattern
Add the type import near the top (after line 4):
import { describe, it, expect, beforeEach, vi } from "vitest"; import { loadState, saveState, clearState, type NemoClawState } from "./state.js"; +import type fs from "node:fs";Then update the mock factory:
vi.mock("node:fs", async (importOriginal) => { - const original = await importOriginal(); + const original = await importOriginal<typeof fs>(); return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/state.test.ts` around lines 9 - 10, The test's mock currently uses type inference for importOriginal(); update it to match snapshot.test.ts by adding an explicit type import (import type fs from "node:fs") and then call importOriginal<typeof fs>() inside the vi.mock factory used in the vi.mock("node:fs", async (importOriginal) => { ... }) block so the mocked "original" variable is strongly typed; adjust any related variable typings in the mock factory (e.g., the `original` constant) to use the imported fs type.nemoclaw/src/blueprint/runner.test.ts (1)
34-35: Consider using the same pattern as snapshot.test.ts for consistency.While relying on type inference works,
snapshot.test.tsuses an explicit top-level type import (import type fs from "node:fs") followed byimportOriginal<typeof fs>(). This approach aligns with the PR summary's stated fix and provides better consistency across files addressing the same ESLint issue.♻️ Align with snapshot.test.ts pattern
Add the type import at the top of the file (after line 5):
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import YAML from "yaml"; +import type fs from "node:fs";Then update the mock factory:
vi.mock("node:fs", async (importOriginal) => { - const original = await importOriginal(); + const original = await importOriginal<typeof fs>(); return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.test.ts` around lines 34 - 35, Add a top-level type import for the Node fs module (import type fs from "node:fs") and update the vi.mock call to use the explicit generic when calling importOriginal by changing importOriginal() to importOriginal<typeof fs>() so the mock factory matches the pattern used in snapshot.test.ts and resolves the same ESLint/type inference fix; keep the existing vi.mock("node:fs", async (importOriginal) => { const original = await importOriginal<typeof fs>(); ... }) structure with the new top-level type import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/blueprint/runner.test.ts`:
- Around line 34-35: Add a top-level type import for the Node fs module (import
type fs from "node:fs") and update the vi.mock call to use the explicit generic
when calling importOriginal by changing importOriginal() to
importOriginal<typeof fs>() so the mock factory matches the pattern used in
snapshot.test.ts and resolves the same ESLint/type inference fix; keep the
existing vi.mock("node:fs", async (importOriginal) => { const original = await
importOriginal<typeof fs>(); ... }) structure with the new top-level type
import.
In `@nemoclaw/src/blueprint/state.test.ts`:
- Around line 9-10: The test's mock currently uses type inference for
importOriginal(); update it to match snapshot.test.ts by adding an explicit type
import (import type fs from "node:fs") and then call importOriginal<typeof fs>()
inside the vi.mock factory used in the vi.mock("node:fs", async (importOriginal)
=> { ... }) block so the mocked "original" variable is strongly typed; adjust
any related variable typings in the mock factory (e.g., the `original` constant)
to use the imported fs type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 981469ee-c84f-41db-8bf0-78b9c95bd491
📒 Files selected for processing (6)
bin/lib/onboard.jsnemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/state.test.tstest/cli.test.jstest/uninstall.test.js
Apply the same import type fs + importOriginal<typeof fs>() pattern from snapshot.test.ts to runner.test.ts and state.test.ts for consistency.
Replace Number(env) || default with a dedicated envInt() helper that
handles 0 correctly (Number('0') || 5 would silently fall back to 5),
rejects non-finite values, and clamps to non-negative integers.
The recovery loop in recoverGatewayRuntime slept unconditionally after every iteration, including the last one. Guard with the same i < count - 1 check used in startGatewayWithOptions.
ericksoa
left a comment
There was a problem hiding this comment.
Clean and low risk. envInt() is well-written (NaN guard, floor at 0, rounding). Defaults preserve existing behavior. Test speedup is a nice win — no reason to sleep 20s in unit tests. ESLint fixes are the correct TS pattern.
Merge origin/main into feat/jetson-orin-nano-support to resolve conflicts from recent changes (NVIDIA#1208, NVIDIA#1200, NVIDIA#836, NVIDIA#1221, NVIDIA#1223). Jetson detection now leverages main's UNIFIED_MEMORY_GPU_TAGS with added jetson flag and /proc/device-tree fallback. All 116 tests pass.
## Problem
After pulling latest main, `prek run -a` fails with:
1. **ESLint error** in `snapshot.test.ts` —
`@typescript-eslint/consistent-type-imports` forbids `import()` type
annotations in generic parameters
2. **Slow tests** — gateway recovery tests sleep 8–19s each waiting for
health polls that will never succeed against stub `openshell` scripts,
adding ~40s to the suite
## Changes
### ESLint fix
- `snapshot.test.ts`: replace `importOriginal<typeof
import("node:fs")>()` with a top-level `import type fs from "node:fs"`
and use `importOriginal<typeof fs>()`
### Test performance (70s → 28s)
- `onboard.js`: make health-poll count and interval configurable via
`NEMOCLAW_HEALTH_POLL_COUNT` and `NEMOCLAW_HEALTH_POLL_INTERVAL` env
vars (production defaults unchanged)
- `cli.test.js`: set `POLL_COUNT=1`, `POLL_INTERVAL=0` in the
`runWithEnv` test helper so gateway recovery tests skip unnecessary
sleep loops
- `cli.test.js`: reduce test timeouts from 25s → 10s accordingly
### Formatting (pre-commit hooks)
- Prettier reformatting in `runner.test.ts`, `state.test.ts`,
`uninstall.test.js`
## Verification
- All 740 tests pass
- All pre-commit hooks pass (ESLint, Prettier, TypeScript, tests)
- No production behavior change (env vars default to existing values)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Chores**
* Made gateway health polling configurable via environment variables
(poll count and interval).
* **Tests**
* Reduced CLI test timeouts and injected health-check env vars to speed
CI runs.
* Improved test typing/mocking and cleaned up test formatting for
stability and readability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
…#1223) ## Problem After pulling latest main, `prek run -a` fails with: 1. **ESLint error** in `snapshot.test.ts` — `@typescript-eslint/consistent-type-imports` forbids `import()` type annotations in generic parameters 2. **Slow tests** — gateway recovery tests sleep 8–19s each waiting for health polls that will never succeed against stub `openshell` scripts, adding ~40s to the suite ## Changes ### ESLint fix - `snapshot.test.ts`: replace `importOriginal<typeof import("node:fs")>()` with a top-level `import type fs from "node:fs"` and use `importOriginal<typeof fs>()` ### Test performance (70s → 28s) - `onboard.js`: make health-poll count and interval configurable via `NEMOCLAW_HEALTH_POLL_COUNT` and `NEMOCLAW_HEALTH_POLL_INTERVAL` env vars (production defaults unchanged) - `cli.test.js`: set `POLL_COUNT=1`, `POLL_INTERVAL=0` in the `runWithEnv` test helper so gateway recovery tests skip unnecessary sleep loops - `cli.test.js`: reduce test timeouts from 25s → 10s accordingly ### Formatting (pre-commit hooks) - Prettier reformatting in `runner.test.ts`, `state.test.ts`, `uninstall.test.js` ## Verification - All 740 tests pass - All pre-commit hooks pass (ESLint, Prettier, TypeScript, tests) - No production behavior change (env vars default to existing values) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Made gateway health polling configurable via environment variables (poll count and interval). * **Tests** * Reduced CLI test timeouts and injected health-check env vars to speed CI runs. * Improved test typing/mocking and cleaned up test formatting for stability and readability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…#1223) ## Problem After pulling latest main, `prek run -a` fails with: 1. **ESLint error** in `snapshot.test.ts` — `@typescript-eslint/consistent-type-imports` forbids `import()` type annotations in generic parameters 2. **Slow tests** — gateway recovery tests sleep 8–19s each waiting for health polls that will never succeed against stub `openshell` scripts, adding ~40s to the suite ## Changes ### ESLint fix - `snapshot.test.ts`: replace `importOriginal<typeof import("node:fs")>()` with a top-level `import type fs from "node:fs"` and use `importOriginal<typeof fs>()` ### Test performance (70s → 28s) - `onboard.js`: make health-poll count and interval configurable via `NEMOCLAW_HEALTH_POLL_COUNT` and `NEMOCLAW_HEALTH_POLL_INTERVAL` env vars (production defaults unchanged) - `cli.test.js`: set `POLL_COUNT=1`, `POLL_INTERVAL=0` in the `runWithEnv` test helper so gateway recovery tests skip unnecessary sleep loops - `cli.test.js`: reduce test timeouts from 25s → 10s accordingly ### Formatting (pre-commit hooks) - Prettier reformatting in `runner.test.ts`, `state.test.ts`, `uninstall.test.js` ## Verification - All 740 tests pass - All pre-commit hooks pass (ESLint, Prettier, TypeScript, tests) - No production behavior change (env vars default to existing values) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Made gateway health polling configurable via environment variables (poll count and interval). * **Tests** * Reduced CLI test timeouts and injected health-check env vars to speed CI runs. * Improved test typing/mocking and cleaned up test formatting for stability and readability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Problem
After pulling latest main,
prek run -afails with:snapshot.test.ts—@typescript-eslint/consistent-type-importsforbidsimport()type annotations in generic parametersopenshellscripts, adding ~40s to the suiteChanges
ESLint fix
snapshot.test.ts: replaceimportOriginal<typeof import("node:fs")>()with a top-levelimport type fs from "node:fs"and useimportOriginal<typeof fs>()Test performance (70s → 28s)
onboard.js: make health-poll count and interval configurable viaNEMOCLAW_HEALTH_POLL_COUNTandNEMOCLAW_HEALTH_POLL_INTERVALenv vars (production defaults unchanged)cli.test.js: setPOLL_COUNT=1,POLL_INTERVAL=0in therunWithEnvtest helper so gateway recovery tests skip unnecessary sleep loopscli.test.js: reduce test timeouts from 25s → 10s accordinglyFormatting (pre-commit hooks)
runner.test.ts,state.test.ts,uninstall.test.jsVerification
Summary by CodeRabbit
Chores
Tests