migrate to vitest 4

[kibertoad]

Summary by CodeRabbit

• Bug Fixes

  • Improved cleanup error handling to gracefully tolerate already-closed Redis connections.

• Tests

  • Added tests to ensure unknown action IDs are ignored.
  • Added explicit resource teardown across notification publisher/consumer tests.

• Chores

  • Updated test runner dev-dependencies and adjusted runner configuration to reduce parallelism.
  • Replaced coverage tooling annotations and trimmed CI Node matrix.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds try/catch guards around Redis consumer unsubscribe calls, updates Vitest and its config, tightens test teardown in Redis notification tests, replaces c8 coverage pragmas with v8 preserve pragmas across several files, and removes Node 18.x from CI matrix.

Changes

Cohort / File(s)	Summary
Redis Consumer Error Handling lib/redis/RedisGroupNotificationConsumer.ts, lib/redis/RedisNotificationConsumer.ts	Wrapped unsubscribe inside close() with try/catch to tolerate already-closed connections; retained subsequent quit() behavior. No public API changes.
Test Dependencies (manifest) package.json	Bumped devDependencies: @vitest/coverage-v8 ^3.2.0 → ^4.0.15, vitest ^3.2.4 → ^4.0.15.
Vitest Configuration vitest.config.mts	Removed poolOptions/pool settings; added top-level maxWorkers: 1, isolate: false, fileParallelism: false, exclude patterns; relaxed coverage thresholds for functions and branches.
Test Resource Cleanup & New Tests test/redis/RedisGroupNotificationPublisher.spec.ts, test/redis/RedisNotificationPublisher.spec.ts	Added new "Ignores unknown action IDs" tests; expanded teardown to close operation, notificationConsumer, and notificationPublisher in several tests to ensure proper cleanup.
Coverage / Preserve Pragmas lib/AbstractCache.ts, lib/AbstractFlatCache.ts, lib/Loader.ts, lib/notifications/AbstractNotificationConsumer.ts, lib/redis/RedisExpirationTimeDataSource.ts, lib/redis/RedisExpirationTimeGroupDataSource.ts	Replaced c8 ignore comments with v8 preserve pragmas (e.g., /* v8 ignore next -- @preserve */) around catch handlers and specific methods; no behavioral changes.
CI Matrix .github/workflows/ci.yml	Removed Node.js 18.x from the tested Node version matrix, leaving 20.x, 22.x, 24.x.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify the new try/catch around unsubscribe doesn't swallow unexpected errors or hide connection issues.
• Confirm quit() behavior is unchanged and resolves/cleans up as before.
• Review added tests for flakiness and ensure teardown order is correct.
• Check vitest config changes map to intended runner behavior and coverage expectations.

Suggested reviewers

• andrewi-wd

Poem

🐰 I nibble try/catch round unsubscribe,
tidy tests hop off, no surprise.
Vitest trims the workforce small,
CI waves eighteen farewell call.
A rabbit smiles — the logs look wise.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'migrate to vitest 4' directly and clearly describes the main objective of the pull request, which is to upgrade the project from vitest 3 to vitest 4.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch chore/vitest-4

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

test/redis/RedisNotificationPublisher.spec.ts (1)

411-439: Explicit teardown after error-path test is a good fit with the new close semantics

Adding await operation.close(), then closing notificationConsumer and notificationPublisher, gives this test a clear, explicit teardown and should help avoid open-handle noise with Vitest 4, especially after error conditions.

If Loader.close() is (or later becomes) responsible for closing the notification consumer/publisher internally, it might be worth centralizing the cleanup there to avoid double-closing, but with the new tolerant close() implementations this is safe for now.

test/redis/RedisGroupNotificationPublisher.spec.ts (2)

371-399: Improved teardown for default error handling test

The added operation.close(), notificationConsumer.close(), and notificationPublisher.close() calls make this test’s lifecycle explicit and align it with the non-group notification tests, which is helpful under Vitest 4’s stricter open-handle detection.

Longer term, consider extracting a small helper for this triple-close pattern to keep the Redis tests consistent and reduce duplication.

---

401-433: Closing resources after simulated connection error complements the consumer changes

Explicitly closing the GroupLoader, consumer, and publisher after the connection-error scenario matches the new tolerant close() logic and should prevent lingering Redis handles after the test completes.

If more tests need the same pattern, a shared teardown helper (or afterEach hook for this suite) could keep things DRY while preserving the current behavior.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bcd356 and 3ab54bc.

📒 Files selected for processing (6)

• lib/redis/RedisGroupNotificationConsumer.ts (1 hunks)
• lib/redis/RedisNotificationConsumer.ts (1 hunks)
• package.json (1 hunks)
• test/redis/RedisGroupNotificationPublisher.spec.ts (2 hunks)
• test/redis/RedisNotificationPublisher.spec.ts (1 hunks)
• vitest.config.mts (1 hunks)

🔇 Additional comments (4)

package.json (1)

56-63: Vitest devDependency bump looks consistent with config

Updating vitest and @vitest/coverage-v8 to ^4.0.15 matches the new v4-style options in vitest.config.mts; from the code perspective this is coherent with the rest of the PR.

Please double-check against your CI matrix that all supported Node versions work with Vitest 4 and that npm test passes across environments.

lib/redis/RedisGroupNotificationConsumer.ts (1)

24-35: Making close() tolerant of already-closed/unsubscribed connections

Wrapping unsubscribe(this.channel) in a try/catch while still calling quit() makes close() effectively idempotent and avoids teardown-time errors when the connection is already closed, which is useful given the new explicit test cleanups.

lib/redis/RedisNotificationConsumer.ts (1)

29-40: Symmetric, resilient close() implementation

The added try/catch around unsubscribe(this.channel) plus the existing quit-based resolution gives close() predictable behavior even after connection issues, and keeps this consumer in sync with RedisGroupNotificationConsumer.

vitest.config.mts (1)

5-16: Vitest v4 config aligns with stateful Redis tests

Using maxWorkers: 1, fileParallelism: false, and isolate: false keeps execution effectively single-threaded/non-isolated, which matches the Redis-backed tests and mirrors the previous pool configuration; test.exclude for node_modules/dist also looks fine.

Please confirm these options match the current Vitest 4 configuration keys/semantics you target (in case of any breaking changes between minor versions).

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/redis/RedisNotificationPublisher.spec.ts (1)

473-476: Enhanced teardown improves resource cleanup.

The explicit closure of operation, notificationConsumer, and notificationPublisher is good practice. However, several other tests in this file (e.g., lines 282-338, 340-360, 362-382, 384-409) don't close these resources, creating inconsistency.

Consider applying similar teardown patterns to other tests in this file for consistency and to prevent potential resource leaks in test isolation scenarios.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between be6244e and 7d33f7b.

📒 Files selected for processing (9)

• lib/AbstractCache.ts (4 hunks)
• lib/AbstractFlatCache.ts (2 hunks)
• lib/Loader.ts (2 hunks)
• lib/notifications/AbstractNotificationConsumer.ts (1 hunks)
• lib/redis/RedisExpirationTimeDataSource.ts (1 hunks)
• lib/redis/RedisExpirationTimeGroupDataSource.ts (1 hunks)
• test/redis/RedisGroupNotificationPublisher.spec.ts (3 hunks)
• test/redis/RedisNotificationPublisher.spec.ts (2 hunks)
• vitest.config.mts (2 hunks)

✅ Files skipped from review due to trivial changes (2)

• lib/Loader.ts
• lib/redis/RedisExpirationTimeDataSource.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• test/redis/RedisGroupNotificationPublisher.spec.ts

🧰 Additional context used

🧬 Code graph analysis (1)

test/redis/RedisNotificationPublisher.spec.ts (4)

index.ts (2)

• createNotificationPair (8-8)
• Loader (1-1)

lib/redis/RedisNotificationFactory.ts (1)

• createNotificationPair (18-44)

lib/Loader.ts (1)

• Loader (35-240)

test/fakes/DummyCache.ts (1)

• DummyCache (6-65)

🔇 Additional comments (8)

lib/redis/RedisExpirationTimeGroupDataSource.ts (1)

15-15: LGTM! Coverage pragma updated for Vitest 4.

The migration from c8 to v8 coverage pragma is correct and aligns with the Vitest 4 upgrade.

lib/notifications/AbstractNotificationConsumer.ts (1)

6-6: LGTM! Coverage pragma updated for Vitest 4.

The migration from c8 to v8 coverage pragma is correct and consistent with similar updates across the codebase.

lib/AbstractCache.ts (1)

141-141: LGTM! Coverage pragmas updated for Vitest 4.

All c8 to v8 pragma migrations are correct and maintain the existing coverage exclusions for error handlers in notification consumer/publisher paths.

Also applies to: 151-151, 187-187, 196-196

lib/AbstractFlatCache.ts (1)

152-152: LGTM! Coverage pragmas updated for Vitest 4.

The migration from c8 to v8 coverage pragmas is correct and consistent with similar updates in related files.

Also applies to: 164-164

test/redis/RedisNotificationPublisher.spec.ts (1)

411-447: LGTM! Good test coverage for unknown action IDs.

The new test properly validates that unknown action IDs are ignored and don't affect the cache state. Resource cleanup is correctly implemented.

vitest.config.mts (3)

8-8: Serial test execution configuration is appropriate for Redis integration tests.

The combination of maxWorkers: 1 and fileParallelism: false ensures tests run sequentially, which is reasonable for integration tests that interact with a shared Redis instance.

Also applies to: 11-11

---

9-9: The isolate: false setting is intentional for this Redis integration test suite and is properly supported by robust cleanup patterns in all test files.

---

28-29: Coverage thresholds lowered without explanation.

Coverage thresholds have been reduced:

• functions: 100 → 97
• branches: 95 → 100

Verify that these reductions are justified. If they reflect intentional uncoverable code paths (already marked with v8 ignore pragmas), document this rationale in a comment. If they represent actual loss of test coverage, the thresholds should not be lowered without addressing the coverage gaps first.