Conversation
- Updated bunfig.toml to include preload for test setup. - Changed test scripts in package.json to use Bun instead of Vitest. - Removed unused env.d.ts file and adjusted imports in test files to use the new setup. - Implemented a new D1DatabaseAdapter for in-memory SQLite testing. - Removed vitest.config.ts and replaced it with Bun-compatible test configurations.
…oad step fix: change database execution method in test setup from exec to run
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…imports in client.ts fix: update worker-configuration.d.ts for workerd version and modify WorkerLoader interface delete: remove tsconfig.build.json as it's no longer needed
|
github-actions
bot
added
ci-cd
dependencies
configuration
middleware
routes
tests
client
utils
labels
|
Warning Rate limit exceeded@11gather11 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR updates CI/CD workflows to execute tests with coverage reporting, replaces async glob with Bun's synchronous globbing API, migrates D1 adapter exec to use Bun-native batch execution, reorders client route imports, removes a TypeScript build config file, and updates worker-configuration.d.ts type signatures to accept nullable names and optional tags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/__tests__/setup.ts (1)
15-24: Add error handling for migration failures.The migration loading and application lacks error handling. If a migration file is malformed or contains invalid SQL, the test setup will fail silently or with unclear errors.
🔎 Suggested improvement with error handling
// Apply migrations const drizzleDir = path.resolve(__dirname, '../../drizzle') const migrationFiles = readdirSync(drizzleDir) .filter((f) => f.endsWith('.sql')) .sort() for (const file of migrationFiles) { - const sql = readFileSync(path.join(drizzleDir, file), 'utf-8') - db.run(sql) + try { + const sql = readFileSync(path.join(drizzleDir, file), 'utf-8') + db.run(sql) + } catch (error) { + throw new Error(`Failed to apply migration ${file}: ${error}`) + } }src/__tests__/d1-adapter.ts (2)
41-54: Hardcoded metadata inall()differs fromrun()behavior.The
all()method hardcodeschanges: 0andlast_row_id: 0, while therun()method (lines 56-68) correctly retrieves these values from the statement result. This inconsistency could cause issues if test code relies on accurate metadata fromall()operations.For D1 compatibility and consistency, consider capturing actual metadata:
🔎 Suggested fix to populate metadata
async all<T = unknown>(): Promise<D1Result<T>> { const start = performance.now() const stmt = this.db.prepare(this.query) - const results = stmt.all(...this.params) as T[] + const result = stmt.run(...this.params) + const results = this.db.prepare(this.query).all(...this.params) as T[] return { results, success: true, meta: { - changes: 0, - last_row_id: 0, + changes: result.changes, + last_row_id: Number(result.lastInsertRowid), duration: performance.now() - start, }, } }Note: This executes the statement twice (once for metadata, once for results). If this is a concern, you may need to verify whether Bun's SQLite provides metadata from
.all()directly.
56-68: BigInt to Number conversion could lose precision.Line 64 converts
lastInsertRowid(BigInt) to Number. While unlikely in test scenarios with small datasets, this could theoretically lose precision for row IDs exceedingNumber.MAX_SAFE_INTEGER(2^53 - 1).For completeness, consider documenting this limitation or preserving the BigInt:
async run(): Promise<D1Result> { const start = performance.now() const stmt = this.db.prepare(this.query) const result = stmt.run(...this.params) return { success: true, meta: { changes: result.changes, - last_row_id: Number(result.lastInsertRowid), + last_row_id: Number(result.lastInsertRowid), // Note: precision loss possible for very large IDs duration: performance.now() - start, }, } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.changeset/migrate-to-bun-test.md.github/workflows/ci.ymlbunfig.tomlpackage.jsonscripts/generate-client.tssrc/__tests__/d1-adapter.tssrc/__tests__/env.d.tssrc/__tests__/setup.tssrc/__tests__/test-utils.tssrc/client.tssrc/middlewares/apiKeyMiddleware.test.tssrc/middlewares/corsMiddleware.test.tssrc/routes/v1/guilds/matches/confirm.test.tssrc/routes/v1/guilds/matches/create.test.tssrc/routes/v1/guilds/matches/get.test.tssrc/routes/v1/guilds/matches/remove.test.tssrc/routes/v1/guilds/matches/vote.test.tssrc/routes/v1/guilds/rankings/get.test.tssrc/routes/v1/guilds/ratings/get.test.tssrc/routes/v1/guilds/ratings/upsert.test.tssrc/routes/v1/guilds/users/get.test.tssrc/routes/v1/queues/create.test.tssrc/routes/v1/queues/get.test.tssrc/routes/v1/queues/players/create.test.tssrc/routes/v1/queues/players/remove.test.tssrc/routes/v1/queues/players/update.test.tssrc/routes/v1/queues/remove.test.tssrc/routes/v1/ranks/get.test.tssrc/routes/v1/ranks/upsert.test.tssrc/utils/elo.test.tstsconfig.build.jsontsconfig.jsonvitest.config.tsworker-configuration.d.ts
💤 Files with no reviewable changes (3)
- vitest.config.ts
- tsconfig.build.json
- src/tests/env.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/setup.ts (1)
src/__tests__/d1-adapter.ts (1)
D1DatabaseAdapter(76-120)
🔇 Additional comments (35)
src/client.ts (1)
1-45: File is correctly auto-generated.This file is properly marked as auto-generated and the generation script (
bun run generate:client) exists in package.json. All route imports are valid, and the public API (AppType and hcWithType) follows standard Hono RPC client patterns. No issues detected..changeset/migrate-to-bun-test.md (1)
1-5: LGTM!The changeset correctly documents the migration from Vitest to Bun test runner as a patch release with appropriate refactor categorization.
.github/workflows/ci.yml (1)
21-21: LGTM!The Bun version bump to 1.3.5 aligns with the
@types/bunversion specified in package.json, ensuring consistency across the project.bunfig.toml (1)
4-7: LGTM!The test configuration is well-structured:
- The preload ensures test setup runs before test execution
- Coverage exclusions appropriately skip test files themselves from coverage metrics
- Configuration aligns with the Bun-based testing infrastructure described in the PR
package.json (2)
30-32: LGTM!The test scripts are correctly updated to use Bun's test runner with appropriate flags for each use case (standard run, watch mode, and coverage reporting). This aligns with the CI workflow configuration.
47-47: LGTM!The
@types/bunversion 1.3.5 correctly matches the Bun runtime version specified in the CI workflow, ensuring type consistency.scripts/generate-client.ts (1)
4-7: Test the glob pattern before deploying. The pattern!(*index|*test|*schemas)uses extended glob negation syntax that is not explicitly documented as supported by Bun.Glob. Standard!negation works only at the start of a complete pattern. Consider verifying the pattern matches expected files, or switch to post-scan filtering if the pattern doesn't work as intended.The
{ posix: true }removal is safe—Bun.Glob handles cross-platform paths natively. Removing theasynckeyword from line 4 is optional but recommended sincescanSyncis synchronous.worker-configuration.d.ts (3)
3-3: workerd version header bump looks fineHeader comment reflects a newer workerd build; assuming this file was regenerated via Wrangler for that version, there’s nothing to change here. Just ensure the deployed Workers runtime and your local
wrangler typesgeneration are in sync so these declarations stay accurate.
3282-3284:WorkerLoader.getacceptingstring | nullname matches broader API usageAllowing
nameto bestring | nullmakes the type compatible with callers that intentionally passnullfor “no entrypoint name” and mirrors other APIs that treat the name as optional. This is source-compatible for normal call sites, but if you ever passWorkerLoader["get"]as a callback typed with(name: string, ...) => ..., you may need to widen that callback’s parameter type tostring | nullas well.
8447-8470: MakingAiOptions.tagsoptional removes an unnecessary requirementChanging
tagstotags?: string[]fixes the previous typing where everyAiOptionsconsumer was forced to supply tags even when only using flags likequeueRequestorgateway. The JSDoc constraints on tag content/length still apply at runtime; this type change just makes the option ergonomic and backwards-compatible for existing callers that already passtags.src/utils/elo.test.ts (1)
1-1: LGTM! Clean migration to Bun test runner.The test imports have been correctly updated to use Bun's native test runner while preserving all test logic and assertions.
src/__tests__/test-utils.ts (1)
3-8: LGTM! Test utilities updated to use centralized setup.The env import has been correctly updated to use the local setup module, which centralizes test environment configuration across all test files.
src/routes/v1/queues/get.test.ts (1)
1-4: LGTM! Consistent migration to Bun test framework.Test imports follow the same migration pattern as other test files, maintaining consistency across the test suite.
src/routes/v1/guilds/matches/confirm.test.ts (1)
1-5: LGTM! Complex test suite migrated successfully.The comprehensive match confirmation test suite has been migrated to Bun while preserving all test scenarios and assertions. This is critical functionality and the careful migration approach is appreciated.
src/routes/v1/queues/players/create.test.ts (1)
1-5: LGTM! Queue player tests migrated correctly.Test imports updated consistently with the Bun migration pattern while maintaining full test coverage for queue player functionality.
src/routes/v1/guilds/users/get.test.ts (1)
1-4: LGTM! User history tests migrated successfully.The migration follows the established pattern, maintaining comprehensive test coverage for user match history functionality.
src/routes/v1/guilds/rankings/get.test.ts (1)
1-4: LGTM! Test imports migrated to Bun with centralized setup.The test correctly switches to Bun's test runner and imports env from the centralized setup module. The setup module properly exports env with a D1Database instance initialized with in-memory SQLite and migrations applied.
tsconfig.json (1)
16-16: LGTM! Types configuration updated for Bun migration.@types/bun (version 1.3.5) is correctly included in devDependencies, and the types array now contains only
./worker-configuration.d.tsand@types/bun. No references to Vitest remain in the codebase.src/routes/v1/ranks/upsert.test.ts (1)
1-5: Clean migration to Bun test runner.The import changes correctly migrate from Vitest/Cloudflare test environment to Bun's test runner. The test logic remains unchanged, reducing migration risk.
src/routes/v1/guilds/ratings/upsert.test.ts (1)
1-4: LGTM!Import changes correctly align with the Bun test migration. Test implementation remains intact.
src/routes/v1/queues/remove.test.ts (1)
1-5: LGTM!Test framework migration properly applied with no changes to test logic.
src/routes/v1/queues/players/remove.test.ts (1)
1-4: LGTM!Import updates correctly implement the Bun test runner migration.
src/routes/v1/guilds/matches/get.test.ts (1)
1-4: LGTM!Test framework imports properly updated for Bun migration.
src/routes/v1/guilds/ratings/get.test.ts (1)
1-4: LGTM!Import changes correctly align with the Bun test runner migration.
src/middlewares/corsMiddleware.test.ts (1)
1-3: LGTM!Test framework migration properly applied. The comprehensive CORS test coverage remains intact.
src/routes/v1/guilds/matches/remove.test.ts (1)
1-5: Imports correctly follow Bun test migration pattern. The use of imports frombun:testincludingbeforeEach,describe,expect,it, and other test utilities is standard and properly supported. All other imports (drizzle-orm, hono/testing, and the env module) follow correct patterns for this stack.To complete the review, verify:
src/__tests__/setup.tsexists and properly exportsenv- All tests pass:
bun test- Type checking passes:
bun run typechecksrc/routes/v1/guilds/matches/create.test.ts (1)
1-1: LGTM! Test framework migration looks good.The import changes correctly migrate from Vitest/Cloudflare test environment to Bun's test runner with centralized setup.
Also applies to: 5-5
src/routes/v1/guilds/matches/vote.test.ts (1)
1-1: LGTM! Test framework migration is consistent.Import changes align with the Bun test migration strategy.
Also applies to: 4-4
src/routes/v1/queues/create.test.ts (1)
1-1: LGTM! Migration follows established pattern.Test framework imports correctly updated.
Also applies to: 5-5
src/routes/v1/ranks/get.test.ts (1)
1-1: LGTM! Test migration is correct.Import sources properly updated for Bun test runner.
Also applies to: 4-4
src/routes/v1/queues/players/update.test.ts (1)
1-1: LGTM! Consistent test framework migration.Import changes are correct and align with the PR's migration strategy.
Also applies to: 4-4
src/middlewares/apiKeyMiddleware.test.ts (1)
1-1: LGTM! Test migration completed correctly.Import changes properly migrate to Bun test infrastructure.
Also applies to: 3-3
src/__tests__/setup.ts (1)
27-30: LGTM! Clean test environment setup.The in-memory database approach with D1 adapter is appropriate for testing. The type cast on Line 28 is necessary given the adapter pattern.
src/__tests__/d1-adapter.ts (2)
87-100: LGTM! Transaction handling is correct.The
batch()method properly wraps statements in a transaction with rollback on error, ensuring atomicity.
116-119: LGTM! Database serialization is correctly implemented.The
dump()method properly slices the buffer to create a clean ArrayBuffer for database serialization.
github-actions
bot
removed
ci-cd
dependencies
configuration
middleware
routes
tests
utils
labels
…er into feature/migrate-to-bun-test
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pull_request.yml (1)
24-24: Align Bun version with ci.yml to ensure consistency.This workflow uses Bun
1.3.4whileci.ymluses1.3.5. Version mismatches between PR and main CI workflows can lead to inconsistent test results and unexpected failures after merge.🔎 Proposed fix
- bun-version: 1.3.4 + bun-version: 1.3.5
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/pull_request.ymlsrc/__tests__/d1-adapter.ts
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
38-38: LGTM! Good practice to fail CI on coverage upload errors.Adding
fail_ci_if_error: trueensures that if the coverage upload fails, the CI pipeline will fail, making coverage reporting issues visible immediately..github/workflows/pull_request.yml (1)
35-42: LGTM! Test and coverage steps properly configured.The addition of test execution with coverage reporting and Codecov upload aligns with the PR's goal of migrating to Bun test. The configuration matches
ci.yml(includingfail_ci_if_error: true), ensuring consistent behavior across workflows.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
1 participant
fix: update worker-configuration.d.ts for workerd version and modify WorkerLoader interface
delete: remove tsconfig.build.json as it's no longer needed
Summary
Changes
Test Plan
bun run typecheckRelated Issues
Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.