tests: replace compat validator with bun:test suite using src/data.ts#2
tests: replace compat validator with bun:test suite using src/data.ts#2
Conversation
- Removed static compat.json validator - Added compat.test.ts powered by Bun’s built-in test runner - Tests import real data from src/data.ts for frameworks, DBs, ORMs, hosts, and auth providers - Added host↔engine and ORM↔engine subset checks - Skipped scaffold smoke tests pending e2e setup
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds multiple test suites (compatibility, module/unit tests) and includes tests in TypeScript compilation; also inserts a non-functional placeholder comment in a MongoDB branch of a DB scaffold generator. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 review |
✅ Actions performedReview triggered.
|
![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 (2)
tests/compat.test.ts (2)
4-4: Consider removing unnecessary directives.The
eslint-disable import/no-unused-modulesand the firstexport {}at line 18 may be unnecessary since this is a test file that isn't expected to export modules, and there's a duplicateexport {}at line 119.Also applies to: 18-18
119-119: Remove duplicate export statement.This
export {}duplicates the one at line 18. Only one is needed to make the file a module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/generators/db/scaffoldDocker.ts(1 hunks)tests/compat.test.ts(1 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (4)
tsconfig.json (1)
34-34: LGTM!Adding
"tests/**/*"to the include array correctly enables TypeScript compilation and type-checking for the new test suite.tests/compat.test.ts (3)
31-59: LGTM!The integrity checks effectively validate that the data arrays from
src/data.tsare well-formed and contain expected baseline values.
75-107: LGTM!The ORM dialect tests correctly validate that the dialect arrays are subsets of available engines and that the
isDrizzleDialectandisPrismaDialectguard functions accurately classify engines. This properly tests production code rather than duplicating logic.
109-117: Smoke tests appropriately skipped.The skipped scaffold smoke tests align with the PR description noting they're "pending end-to-end (e2e) setup."
| ); | ||
|
|
||
| if (databaseEngine === 'mongodb') { | ||
| // ...work... |
There was a problem hiding this comment.
Remove or clarify the placeholder comment.
The comment // ...work... doesn't provide useful information. Either remove it if no work is needed, or replace it with a specific TODO/FIXME describing what needs to be implemented.
🤖 Prompt for AI Agents
In src/generators/db/scaffoldDocker.ts around line 45, replace the useless
placeholder comment "// ...work..." by either removing it entirely if no action
is required, or by replacing it with a clear TODO/FIXME that explains exactly
what needs to be implemented (include expected behavior, inputs/outputs, and any
relevant ticket/issue ID). Ensure the comment follows project convention (e.g.,
TODO: implement X — ticket #123) so future readers know the intent.
…ed up eslint errors and added small fixes so all tests run smooth with bun test
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/modules/backend/backend.elysia.spec.ts (1)
1-7: Placeholder test provides no validation.This test suite contains only a trivial assertion (
expect(true).toBeTrue()), which doesn't validate any actual behavior of the Elysia backend module.Consider either:
- Adding meaningful tests that validate Elysia backend functionality, or
- Removing this file until real test cases are ready to be implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/compat.test.ts(1 hunks)tests/modules/auth/auth.spec.ts(1 hunks)tests/modules/backend/backend.elysia.spec.ts(1 hunks)tests/modules/db/db.spec.ts(1 hunks)tests/modules/frontend/frontend.react.spec.ts(1 hunks)
🔇 Additional comments (7)
tests/modules/auth/auth.spec.ts (1)
1-9: LGTM! Validates critical "none" option.The test correctly verifies that "none" is included in the available auth providers, ensuring users can opt out of authentication.
tests/modules/frontend/frontend.react.spec.ts (1)
1-9: LGTM! Validates React availability.The test correctly verifies that React is included in the available frontend frameworks.
tests/modules/db/db.spec.ts (2)
11-15: LGTM! Validates "none" option for database engines.The test correctly verifies that "none" is included in the available database engines.
36-43: LGTM! ORM dialect guard tests are correct.The tests properly validate that
isDrizzleDialectandisPrismaDialecttype guards correctly recognize their respective valid dialects.tests/compat.test.ts (3)
32-59: LGTM! Integrity checks validate core invariants.The tests correctly verify that essential data arrays are properly populated and include required "none" options, ensuring users can opt out of features.
61-84: Circular logic resolved, but consider testing production code.The test now uses explicit expected values (lines 63-79) rather than circular logic, which resolves the issue from the past review. However, the test still validates only the local
hostAllowsEnginehelper rather than the actual production validation logic.For stronger validation, consider importing and testing the actual host↔engine validation from
parseCommandLineOptionsto ensure this test helper stays in sync with production behavior.
86-118: LGTM! Comprehensive ORM dialect subset validation.The tests thoroughly validate both positive and negative cases:
- Drizzle/Prisma dialects are subsets of available engines
- Type guards correctly recognize valid dialects
- Type guards correctly reject invalid dialects
- replace useless test with actual db host/engine checks - rename ok -> isAllowed to make eslint happy - add missing types and remove unused import
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/modules/db/db.spec.ts (1)
16-43: Previous issue resolved—explicit test cases now validate against hardcoded expectations.The tautological assertion has been fixed by using explicit expected boolean values (lines 25-36), which provides real validation of the host-engine compatibility rules.
However, the
hostAllowshelper (lines 17-23) duplicates logic fromtests/compat.test.tswhere a nearly identicalhostAllowsEnginefunction exists (lines 22-29). Consider extracting this shared validation logic into a test utility module (e.g.,tests/utils/hostRules.ts) to eliminate duplication and ensure consistency when rules change.tests/compat.test.ts (2)
130-130: Remove duplicate export statement.Line 18 already contains
export {};, making this duplicate unnecessary.Apply this diff to remove the duplication:
-export {};
22-29: Consider extracting shared host-engine validation logic.The
hostAllowsEnginehelper duplicates thehostAllowsfunction fromtests/modules/db/db.spec.ts(lines 17-23). Both encode identical host-engine compatibility rules. Extract this logic into a shared test utility module (e.g.,tests/utils/hostRules.ts) to eliminate duplication and ensure consistency when compatibility rules evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/compat.test.ts(1 hunks)tests/modules/db/db.spec.ts(1 hunks)
🔇 Additional comments (8)
tests/modules/db/db.spec.ts (3)
1-9: LGTM!Imports are appropriate, and the eslint-disable directive is reasonable for test modules.
10-14: LGTM!The test correctly validates the presence of the "none" option in available database engines.
45-52: LGTM!The dialect guard tests correctly validate that all available Drizzle and Prisma dialects are recognized by their respective type guard functions.
tests/compat.test.ts (5)
1-19: LGTM!The header comments clearly document the test's purpose, and imports are appropriate.
31-59: LGTM!The integrity checks provide good coverage for validating the presence of required options and ensuring data consistency across configuration arrays.
61-84: Previous circular test issue resolved—explicit test cases now validate the helper.The test now uses hardcoded
isAllowedvalues (lines 63-79) instead of computingexpectedby calling the same function, which provides real validation of the host-engine compatibility logic.
86-118: LGTM!The ORM dialect subset tests are comprehensive, covering both positive validation (dialects are recognized) and negative validation (non-dialects are rejected by guards).
120-128: LGTM!The disabled scaffold smoke tests are appropriately skipped with clear rationale, avoiding filesystem and template interactions in unit test runs.
- Move host-engine validation to tests/utils/hostRules.ts - Update both test files to use shared logic - Remove duplicate export statement from compat.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…eScript's inference instead absolute/no-explicit-return-type
1 participant
Summary by CodeRabbit
Tests
Chores