Skip to content

perf(test): switch vitest default env to node, move jsdom to 3 DOM files#3852

Open
KATO-Hiro wants to merge 7 commits into
stagingfrom
#3847
Open

perf(test): switch vitest default env to node, move jsdom to 3 DOM files#3852
KATO-Hiro wants to merge 7 commits into
stagingfrom
#3847

Conversation

@KATO-Hiro

@KATO-Hiro KATO-Hiro commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

close #3847

Summary by CodeRabbit

  • テスト
    • Vitest の既定実行環境を node に変更し、DOM依存テストは各ファイルで jsdom を明示して有効化。
    • ブラウザ/SSRの切替を安定させ、localStorage を実環境(jsdom)に基づいて検証するよう改善。
  • 開発環境
    • テスト関連パッケージを整理しました。
    • コンテナのボリュームマウント設定からキャッシュ指定を削除しました。

KATO-Hiro and others added 3 commits July 13, 2026 10:07
- vite.config.ts: set default environment to `node`
- add `// @vitest-environment jsdom` to 3 files referencing DOM APIs
- remove @testing-library/svelte and @testing-library/jest-dom (0 imports)
- compose.yaml: use named volume for node_modules to avoid virtiofs overhead
- .claude/rules/testing.md: document per-file jsdom opt-in rule
- docs/dev-notes/2026-07-11/build-performance: add survey and TODO with measurements

wall clock 33.3s → 11.3s (2.9x); environment cumulative 161s → 5.7s

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Named volume's initial mount is an empty root-owned directory, which
causes EACCES when the node user runs pnpm install under the
`.:/usr/src/app` bind mount. The layering of bind mount + named
volume doesn't allow Docker image initialization to populate it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Investigation and TODO items are done; findings already folded into
prior commits (compose.yaml revert, dependency prune).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@KATO-Hiro, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: db45f27f-a385-4873-8d75-0a435417a024

📥 Commits

Reviewing files that changed from the base of the PR and between fbd1349 and 1d5f3fe.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml, !pnpm-lock.yaml
📒 Files selected for processing (2)
  • .claude/rules/testing.md
  • package.json
📝 Walkthrough

Walkthrough

Vitestの既定環境をnodeへ変更し、DOM依存の3テストのみjsdomを明示指定しました。ストアテストは実localStorageと動的なbrowserモックで、ブラウザ・SSR分岐を検証します。不要なテスト依存関係とComposeの:cached指定も削除しました。

Changes

Vitest環境とストアテスト

Layer / File(s) Summary
Vitest環境ポリシーと実行設定
vite.config.ts, .claude/rules/testing.md, package.json, compose.yaml
既定環境をnodeへ変更し、DOM依存テストのみjsdomを使用するルールを追加しました。未使用依存関係とボリュームの:cached指定を削除しました。
ストアのブラウザ・SSR分岐テスト
src/features/tasks/stores/active_contest_type.svelte.test.ts, src/features/workbooks/stores/replenishment_workbook.test.ts, src/test/lib/stores/active_problem_list_tab.svelte.test.ts, src/features/workbooks/stores/replenishment_workbook.svelte.ts
実localStorage、vi.hoistedによるbrowser切替、永続化・復元・不正値・SSR分岐の検証を追加・変更しました。
テスト修正計画と検証記録
docs/dev-notes/2026-07-13/store-test-browser-mock-fix/plan.md
モック問題への対応方針、改修内容、検証結果、運用上の知見を記録しました。

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 主要変更であるVitestの既定環境変更と3つのDOMテストへのjsdom移行を端的に表しています。
Linked Issues check ✅ Passed #3847の主目的である既定環境のnode化と、DOM依存3ファイルのみjsdom適用という要件を満たしています。
Out of Scope Changes check ✅ Passed 追加のdocs、依存削除、compose調整も性能改善とテスト修正に関連しており、明確な逸脱は見当たりません。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch #3847

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.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/features/tasks/stores/active_contest_type.svelte.test.ts`:
- Around line 1-2: Remove the `@vitest-environment` jsdom annotation from
src/features/tasks/stores/active_contest_type.svelte.test.ts:1-2,
src/features/workbooks/stores/replenishment_workbook.test.ts:1-2, and
src/test/lib/stores/active_problem_list_tab.svelte.test.ts:1-2. Verify the
associated store tests use only mocked localStorage and have no window or
document dependencies so they run with Vitest’s default node environment.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 71c54fa8-699d-4d16-b81f-a05fdea85cb8

📥 Commits

Reviewing files that changed from the base of the PR and between 3fe6860 and 9dcda03.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml, !pnpm-lock.yaml
📒 Files selected for processing (7)
  • .claude/rules/testing.md
  • compose.yaml
  • package.json
  • src/features/tasks/stores/active_contest_type.svelte.test.ts
  • src/features/workbooks/stores/replenishment_workbook.test.ts
  • src/test/lib/stores/active_problem_list_tab.svelte.test.ts
  • vite.config.ts
💤 Files with no reviewable changes (1)
  • package.json

Comment thread src/features/tasks/stores/active_contest_type.svelte.test.ts
KATO-Hiro and others added 2 commits July 13, 2026 13:14
…se in store tests

Each store test file registered `$app/environment` twice (top-level
`{browser:true}` plus an SSR-describe `{browser:false}`). vi.mock calls
are hoisted and the last registration for a module wins, so every test
in these files silently ran with browser=false — the localStorage
branches in ActiveContestTypeStore, ReplenishmentWorkBooksStore, and
ActiveProblemListTabStore never executed, and the "invalid localStorage
key" tests were false-positives.

Replace the double registration with a single vi.mock using a
vi.hoisted flag toggled per describe, and use jsdom's real Storage
instead of a vi.stubGlobal mock so assertions can check actual
persisted state. Un-skip the replenishment persistence test (its
skip comment blamed jsdom, not the real cause). Export
ReplenishmentWorkBooksStore so tests can construct fresh instances
for the invalid-JSON catch branch. Document the pitfall in
testing.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implementation and verification already landed in fbd1349.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@KATO-Hiro

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@KATO-Hiro Review finished.

✅ Action performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…ssertions

Follow-up to fbd1349's testing.md addition. Two more pitfalls found
while applying the pattern: asserting a browser branch against the
import-time singleton is the same false-positive as the double-mock
issue (the singleton is always built with browser=false), and a
single assertion can't cover both the localStorage read guard and
write guard — they need separate cases.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

[deps] pnpm build / vitest の処理時間を短縮できるようにしましょう

1 participant