Skip to content

refactor(tests): extract setupTempWorkspace helper (closes #862) - #865

Merged
sonichi merged 2 commits into
mainfrom
refactor/temp-workspace-helper
May 19, 2026
Merged

sonichi merged 2 commits into
mainfrom
refactor/temp-workspace-helper

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Summary

Extract Mini's per-process mkdtempSync test isolation pattern (established by #849) into a shared helper, eliminating the boilerplate duplication flagged by sonichi-bot on #858 LGTM follow-up.

Before — each test file:

const TEMP_WORKSPACE = mkdtempSync(join(tmpdir(), 'sutando-test-foo-'));
process.env.SUTANDO_WORKSPACE = TEMP_WORKSPACE;
// ...
after(() => rmSync(TEMP_WORKSPACE, { recursive: true, force: true }));

After:

import { setupTempWorkspace } from './_helpers/temp-workspace.js';
const { workspace, cleanup } = setupTempWorkspace('foo');
// ...
after(cleanup);

Changes

  • tests/_helpers/temp-workspace.ts — new helper with setupTempWorkspace(name) returning { workspace, cleanup }.
  • tests/agent-state-endpoint.test.ts — uses helper, ditches inline mkdtempSync/rmSync boilerplate.
  • tests/get-core-status-tool.test.ts — same.
  • .gitignore — allowlist tests/_helpers/ (the existing tests/* rule shadowed the new dir).

Test plan

  • npx tsc --noEmit — clean.
  • npx tsx --test tests/get-core-status-tool.test.ts — all 6 cases pass.
  • CI green on tests/agent-state-endpoint.test.ts (spawn-based, slow locally).

Closes #862.

Mini's #849 + #855 per-process workspace pattern was inline in two
test files. Extracting to a shared helper so future workspace-touching
tests adopt it by default rather than reinventing.

## The helper

`tests/_helpers/temp-workspace.ts` exports:

```ts
export function setupTempWorkspace(name: string): {
    workspace: string;
    cleanup: () => void;
}
```

Module-level call. Sets `process.env.SUTANDO_WORKSPACE` to a fresh
`mkdtempSync(... 'sutando-test-<name>-')` so:

1. The code under test's `resolveWorkspace()` at module init resolves
   to the temp dir.
2. Any spawned subprocess can be passed `SUTANDO_WORKSPACE: workspace`
   in its env so its own `resolveWorkspace()` resolves identically.

`cleanup()` is idempotent — safe to call in `after()` without
guarding.

## Refactor

- `tests/agent-state-endpoint.test.ts` — replaces inline
  `mkdtempSync` + `rmSync` + `process.env.SUTANDO_WORKSPACE = ...`
  with `setupTempWorkspace('agent-state')`. Spawn env passes
  `SUTANDO_WORKSPACE: TEMP_WORKSPACE` unchanged.
- `tests/get-core-status-tool.test.ts` — same refactor with name
  `'getcore'`.

Net: 22+/22- (per-test files shrink, helper file adds the
abstraction).

## Verified

- `npx tsc --noEmit` ✓ clean.
- `npx tsx --test tests/get-core-status-tool.test.ts` — all 6 cases
  pass. (Note: `node --test` directly doesn't work for `.ts` test
  files; tests run via the existing `npm test` script which uses
  `tsx --test`.)

## Closes

#862

## What I'm NOT doing

- Doc note in `docs/workspace-design.md` test-isolation section —
  deferred to the same PR that ratifies the RFC (#858), so the doc's
  test-isolation section can reference the helper that's now in
  main. Cleaner than landing the doc reference before the helper
  itself is in main.
…ce helper

Issue #862 follow-through. Previous commit cf26b53 was silently incomplete:
.gitignore line 42 (`tests/*`) shadowed the new helper file, so the commit
contained only the test refactors and the branch was broken at HEAD.

- Add `!tests/_helpers/` + `!tests/_helpers/*.ts` exceptions (matching the
  existing `!tests/*.test.ts` pattern for canonical test sources).
- Add the helper itself (`setupTempWorkspace(name)`) that two tests already
  imported from in cf26b53.

Closes #862.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM. Clean factoring of the per-process temp-workspace pattern that #849 established. Verified locally on commit head:

  • tests/_helpers/temp-workspace.ts doc-comment is excellent — explains the module-init vs env-read timing subtlety that bites people (and is the reason setupTempWorkspace writes process.env.SUTANDO_WORKSPACE unconditionally, not just returns the path).
  • .gitignore allowlist for tests/_helpers/ is necessary — the existing tests/* rule was shadowing the new dir; without these two lines the helper wouldn't get committed.
  • grep -rln "mkdtempSync.*sutando-test" tests/ confirms the 2 migrated files are the only ones using this pattern. No other test file silently kept the boilerplate.
  • npx tsc --noEmit clean, npx tsx --test tests/get-core-status-tool.test.ts → 6/6 pass.
  • CI green.

Nit (non-blocking, future PR): the helper's spawn-child guidance ("pass SUTANDO_WORKSPACE: workspace in the child's env") could be a tiny convenience overload — setupTempWorkspace('foo', { forChildSpawn: true }) returning { ..., childEnv: { SUTANDO_WORKSPACE: workspace } } — but only worth adding when a third spawn-based test file shows up. Not now.

Ready to merge.

@sonichi
sonichi merged commit cb4a7f4 into main May 19, 2026
2 checks passed
@sonichi
sonichi deleted the refactor/temp-workspace-helper branch May 19, 2026 01:59
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
… (sonichi#865)

* test: extract setupTempWorkspace() helper (closes sonichi#862)

Mini's sonichi#849 + sonichi#855 per-process workspace pattern was inline in two
test files. Extracting to a shared helper so future workspace-touching
tests adopt it by default rather than reinventing.

## The helper

`tests/_helpers/temp-workspace.ts` exports:

```ts
export function setupTempWorkspace(name: string): {
    workspace: string;
    cleanup: () => void;
}
```

Module-level call. Sets `process.env.SUTANDO_WORKSPACE` to a fresh
`mkdtempSync(... 'sutando-test-<name>-')` so:

1. The code under test's `resolveWorkspace()` at module init resolves
   to the temp dir.
2. Any spawned subprocess can be passed `SUTANDO_WORKSPACE: workspace`
   in its env so its own `resolveWorkspace()` resolves identically.

`cleanup()` is idempotent — safe to call in `after()` without
guarding.

## Refactor

- `tests/agent-state-endpoint.test.ts` — replaces inline
  `mkdtempSync` + `rmSync` + `process.env.SUTANDO_WORKSPACE = ...`
  with `setupTempWorkspace('agent-state')`. Spawn env passes
  `SUTANDO_WORKSPACE: TEMP_WORKSPACE` unchanged.
- `tests/get-core-status-tool.test.ts` — same refactor with name
  `'getcore'`.

Net: 22+/22- (per-test files shrink, helper file adds the
abstraction).

## Verified

- `npx tsc --noEmit` ✓ clean.
- `npx tsx --test tests/get-core-status-tool.test.ts` — all 6 cases
  pass. (Note: `node --test` directly doesn't work for `.ts` test
  files; tests run via the existing `npm test` script which uses
  `tsx --test`.)

## Closes

sonichi#862

## What I'm NOT doing

- Doc note in `docs/workspace-design.md` test-isolation section —
  deferred to the same PR that ratifies the RFC (sonichi#858), so the doc's
  test-isolation section can reference the helper that's now in
  main. Cleaner than landing the doc reference before the helper
  itself is in main.

* fix(tests): allowlist tests/_helpers/ in .gitignore, add temp-workspace helper

Issue sonichi#862 follow-through. Previous commit cf26b53 was silently incomplete:
.gitignore line 42 (`tests/*`) shadowed the new helper file, so the commit
contained only the test refactors and the branch was broken at HEAD.

- Add `!tests/_helpers/` + `!tests/_helpers/*.ts` exceptions (matching the
  existing `!tests/*.test.ts` pattern for canonical test sources).
- Add the helper itself (`setupTempWorkspace(name)`) that two tests already
  imported from in cf26b53.

Closes sonichi#862.
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.

test: extract per-process workspace setup to tests/_helpers/temp-workspace.ts

2 participants