Skip to content

test(repo): prefer test.each over for/forEach loops in tests#303

Merged
rafa-thayto merged 1 commit into
mainfrom
test-each-over-loops
May 21, 2026
Merged

test(repo): prefer test.each over for/forEach loops in tests#303
rafa-thayto merged 1 commit into
mainfrom
test-each-over-loops

Conversation

@rafa-thayto
Copy link
Copy Markdown
Contributor

@rafa-thayto rafa-thayto commented May 21, 2026

Summary

  • Adds a new convention to .claude/rules/testing.md: tests should use test.each (or it.each / describe.each) instead of for/forEach loops inside a single test, so each case becomes its own reported test with pinpointed failure attribution.
  • Converts every existing hand-rolled parameterization loop in the test suite to test.each. Test count grows from 1450 → 1528 (each test.each row becomes a separately reported case); zero new failures.
  • Documents the two carve-outs: loops are fine when the iteration is the behavior under test, or when the data is collected at runtime and can't be expressed as a static module-load table.
  • Documents the Bun test.each readonly-array gotcha: spread as const/readonly arrays to a mutable copy (test.each([...MODES])) so Bun's literal-inferring overload preserves the union in the callback type.

Files refactored

File Change
cli-program.test.ts two outer for loops → cross-product test.each(AGENT_DIR_CASES) + test.each([...EXTRA_REL_PATHS])
bootstrap.test.ts nested registry × package-manager loops → test.each(REGISTRY_PM_PAIRS)
skill/install.test.ts two per-file version assertion loops → test.each(VERSION_FILE_CASES)
lib/listage.test.ts two for (active = 0; <20; ...) property tests → cross-product test.each(SCROLL_CASES)
lib/users.test.ts assertion-loop over invalid JSON inputs → test.each
integration/onboard.test.ts nested for (mode) { test.each(frameworks) } → flattened test.each(ONBOARD_CASES) with explicit FrameworkCase interface
integration/switch-apps.test.ts outer for (mode of MODES)test.each([...MODES])

Loops left alone (per the documented exceptions)

  • Cleanup/teardown loops (env restoration, mock restoration, temp-dir removal).
  • Runtime-data assertions (e.g. http.requests collected during the test, files map captured from a callback).
  • Mathematical iteration that is the property being asserted (pkce rejection-sampling distribution).
  • Setup loops creating fixture files.

Test plan

  • bun run typecheck — passes
  • bun run lint — 0 warnings, 0 errors
  • bun run format:check — clean
  • bun run test on the 7 touched files — 217/217 pass
  • Full bun run test — 1406 pass, 122 fail; the 122 failures are preexisting on main and unrelated to this diff (verified via git stash baseline)
  • bun run test:e2e:op — not run locally; CI will exercise

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 21, 2026

⚠️ No Changeset found

Latest commit: e89546e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b02c8983-30b6-4487-b71b-70a8247faa23

📥 Commits

Reviewing files that changed from the base of the PR and between 04baa3f and e89546e.

📒 Files selected for processing (8)
  • .claude/rules/testing.md
  • packages/cli-core/src/cli-program.test.ts
  • packages/cli-core/src/commands/init/bootstrap.test.ts
  • packages/cli-core/src/commands/skill/install.test.ts
  • packages/cli-core/src/lib/listage.test.ts
  • packages/cli-core/src/lib/users.test.ts
  • packages/cli-core/src/test/integration/onboard.test.ts
  • packages/cli-core/src/test/integration/switch-apps.test.ts
✅ Files skipped from review due to trivial changes (1)
  • .claude/rules/testing.md

📝 Walkthrough

Walkthrough

This PR replaces manual in-test loops with parameterized test.each across documentation, unit tests, and integration tests. It adds a testing convention doc explaining the rule, a typing workaround for readonly/as const arrays, and exceptions where loops are acceptable. The pattern is applied to several unit tests (cli-program, bootstrap, skill/install, listage, users) and two integration tests (onboard, switch-apps), preserving existing assertions and test behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main objective: converting test loops to test.each across the test suite.
Description check ✅ Passed The description comprehensively explains the changes, including the new convention, files refactored, exceptions documented, and test results.
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.

✏️ 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.

@rafa-thayto rafa-thayto enabled auto-merge (squash) May 21, 2026 14:58
Convert hand-rolled parameterization loops in test bodies (and around
test definitions) to test.each so each case becomes its own reported
test with pinpointed failure attribution. Document the convention in
.claude/rules/testing.md, including Bun's readonly-array spread gotcha
and the runtime-data exception.
@rafa-thayto rafa-thayto force-pushed the test-each-over-loops branch from 04baa3f to e89546e Compare May 21, 2026 19:27
@rafa-thayto rafa-thayto merged commit 5f77408 into main May 21, 2026
10 checks passed
@rafa-thayto rafa-thayto deleted the test-each-over-loops branch May 21, 2026 19:38
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.

2 participants