Skip to content

test: raise the Vitest hook timeout to 60s - #227

Merged
metalwarrior665 merged 3 commits into
masterfrom
claude/cli-test-compile-once
Oct 31, 2026
Merged

test: raise the Vitest hook timeout to 60s#227
metalwarrior665 merged 3 commits into
masterfrom
claude/cli-test-compile-once

Conversation

@metalwarrior665

@metalwarrior665 metalwarrior665 commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Problem

affected-actors-cli, build-cli and logging each compile the whole project with tsc in their beforeAll. That's about 5s per compile, and the three run at the same time as the rest of the suite. The repo has no Vitest config, so those hooks run under Vitest's default 10s hookTimeout, and in the pre-commit hook they sometimes went over it.

Change

  • Timeout. A new vitest.config.ts sets hookTimeout: 60_000. Unit tests taking longer than 10s is fine; a flaky run isn't.
  • Lint. vitest.config.ts is added to ESLint's ignores, next to eslint.config.mjs, because it isn't part of the tsconfig project.

Checks

  • npm test: 32 files passed, 1 skipped
  • eslint, prettier --check and type-check pass

🤖 Generated with Claude Code

https://claude.ai/code/session_016XtpQtt5LQb3A1b6nkD4DY

@claude
test: compile the CLI once per run and raise the hook timeout
b095e3c 
Three test files each compiled the whole project with tsc in their
beforeAll. Running in parallel with the rest of the suite, those hooks
took close to Vitest's default 10s hookTimeout and sometimes went over.
A timed-out compileCli() never returned its cleanup, so its .cli-test-*
build stayed in the repo root, and the next run collected the compiled
copies of the tests inside it and failed across the board.

- Compile once in a Vitest globalSetup and hand the path to the tests
  with provide/inject. The build goes under node_modules/.cache, which
  Vitest never collects from, and is removed when the compile fails.
- Raise hookTimeout to 60s in a new vitest.config.ts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016XtpQtt5LQb3A1b6nkD4DY
JuanGalilea
JuanGalilea requested changes Oct 30, 2026
View reviewed changes

@JuanGalilea JuanGalilea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tbh do we even need to optimize this? we create 2 or 3 and its performant enough and afaik the number is not going to grow more, no?

Comment thread test/compile-cli.global-setup.ts Outdated

@JuanGalilea JuanGalilea Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dont like this, why not just create it as part of the file import. meaning create the folder on root level and then using it?

just move the main setup thing out of the function and provide a way to tear it down withoult having issues with each other.

@metalwarrior665 metalwarrior665 Oct 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I haven't really looked at the code, I trimmed the PR just to timeout increase which is what was blocking me

@claude
test: keep only the hook timeout increase
2138095 
Drop the shared globalSetup compile and go back to compiling the CLI in
each test file as on master. The 60s hookTimeout alone stops those hooks
from timing out when the whole suite runs in parallel.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016XtpQtt5LQb3A1b6nkD4DY
@metalwarrior665 metalwarrior665 changed the title test: compile the CLI once per run and raise the hook timeout test: raise the Vitest hook timeout to 60s Oct 30, 2026
@metalwarrior665
metalwarrior665 marked this pull request as ready for review October 31, 2026 08:29
@metalwarrior665
metalwarrior665 merged commit 54084cc into master Oct 31, 2026
10 checks passed
@metalwarrior665 metalwarrior665 mentioned this pull request Nov 2, 2026
Merged
1 task done
metalwarrior665 added a commit that referenced this pull request Nov 4, 2026
@metalwarrior665 @claude
refactor!: remove legacy get-commits, get-latest-commit and get-chang…
d254a6d 
…ed-files (#228)

I think this makes sense, users can use `get-affected-actors` to get
changedFiles too.

## AI

`get-affected-actors` (#204) now computes commits, changed files and
changelog in one place from a single diff path. The three older commands
(`get-commits`, `get-latest-commit`, `get-changed-files`) are documented
as "kept for older workflows", but no workflow in this repo uses them.
They still carry a second, parallel git model:

- `getCommits`, which has its own rerun and base-commit heuristics and
duplicates what `since` does in `get-affected-actors`.
- `getChangedFiles`, which is `getChangedFilesBetween` with a `~` hack.
- `parseBaseCommit`, with the "SHA or Commit JSON" overload that only
existed to pipe `get-latest-commit` output back in.
- The `--source-branch`, `--target-branch` and `--base-commit` flags,
the `gitRangeOptions` group and the `Config` type.

#218 left these out on purpose and said the call belongs to the
maintainers. This PR is that proposal.

## What

- Remove the three commands and everything only they used. About 140
lines of source and about 250 lines of tests go away.
- `resolveReleaseBaseCommit` now validates the SHA inline and reuses
`findCommitSha`, instead of a second copy of the "`rev-parse` exit code
1 means missing" handling.
- Drop the "Legacy git commands" section from `docs/cli.md`.
CONTRIBUTING's CLI example now uses `get-affected-actors --base master`.
- Delete `logging.test.ts`. It compiled the whole project with `tsc`
(the cause of #227's timeout) just to check log levels through
`get-commits`. The same check is now two cases in `cli-bundle.test.ts`
that run `get-affected-actors` against the committed bundle.
- Rebuild the CLI bundle.

## Breaking

Anyone who calls these three commands directly breaks. The
`workflows-v0` reusable workflows don't, so only external users
scripting the CLI are affected. Since we're before v1, this is the
cheapest time to drop them. Whether to do it is the open question for
reviewers.

## Other ideas I considered and left out

Each overlaps an open PR, so it would conflict:

- #218 (build pipeline), #223 (`test` and `notify` commands) and #224
(bundle-based CLI tests) already cover the build and reporting code.
- Filling `lib/` gaps from issues (`fails` modifier #159, standby
without a task #127) are features, not refactors.

## Checks

- [x] `tsc`, eslint, prettier, knip (the unused exports it lists were
already on master) and `npm test` pass: 31 files, 438 tests.

---
_Generated by [Claude
Code](https://claude.ai/code/session_016XtpQvEqmuEhj1cBAaUxML)_

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JuanGalilea JuanGalilea JuanGalilea requested changes
@ruocco-l ruocco-l Awaiting requested review from ruocco-l ruocco-l is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@metalwarrior665 @JuanGalilea @claude