add CI workflow #683
add CI workflow #683
Conversation
|
@moreorover is attempting to deploy a commit to the Better T Stack Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds suite-level Vitest lifecycle hooks that run a smoke-directory cleanup before and after many CLI tests, adjusts several test scenarios (runtimes, deployments, and error expectations), enables test coverage in bunfig.toml, updates .gitignore, adds a Git user setup step to the CI test workflow, removes several helper functions from test-utils and deletes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Workflow
participant Repo as Repository
participant TestRunner as Vitest (bun test)
participant Utils as test-utils
rect rgb(219,234,254)
note right of CI: CI job "test"
CI->>Repo: checkout
CI->>CI: setup Bun & pnpm
CI->>CI: install deps (bun install --frozen-lockfile)
CI->>CI: Setup Git user (git config user.name/email)
CI->>TestRunner: run tests (bun test apps/cli)
end
TestRunner->>Utils: beforeAll -> cleanupSmokeDirectory()
alt run suites
TestRunner->>TestRunner: execute test suites...
end
TestRunner->>Utils: afterAll -> cleanupSmokeDirectory()
TestRunner-->>CI: return results
sequenceDiagram
autonumber
participant Test as Test Logic
participant Config as Runtime Decision
Test->>Config: provide serverDeploy, backend
alt serverDeploy == "wrangler" or serverDeploy == "alchemy"
Config->>Test: runtime = "workers"
else if backend == "hono"
Config->>Test: serverDeploy = "wrangler"\nruntime = "workers"
else
Config->>Test: runtime = "bun"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/test/database-orm.test.ts (1)
167-186: Fix contradictory test configuration and assertion.The test is configured with
expectError: true(line 182) but then callsexpectSuccess(result)(line 185). This is logically inconsistent and will cause the test to fail.Based on the test name "should work with auth but no database", it appears the intent is to verify this configuration succeeds. Therefore, remove the
expectError: trueflag.Apply this diff:
it("should work with auth but no database (non-convex backend)", async () => { const result = await runTRPCTest({ projectName: "auth-no-db", database: "none", orm: "none", auth: "better-auth", backend: "hono", runtime: "bun", frontend: ["tanstack-router"], api: "trpc", addons: ["none"], examples: ["none"], dbSetup: "none", webDeploy: "none", serverDeploy: "none", - expectError: true, }); expectSuccess(result); });
🧹 Nitpick comments (2)
.github/workflows/test.yaml (1)
15-18: Consider pinning Bun version for CI determinism.Using
bun-version: latestcould lead to unpredictable CI behavior if a new Bun release introduces breaking changes or unexpected behavior. Consider pinning to a specific version (e.g.,1.1.0) and updating it deliberately.CLAUDE.md (1)
117-126: Consider adding language identifiers to code blocks.The code blocks at lines 117 and 172 are missing language specifiers, which helps with syntax highlighting and is a markdown best practice.
Apply this diff:
-``` +```text User Input (CLI flags or prompts) → Flag Processing (validation.ts) → Compatibility Validation (utils/compatibility-rules.ts)And at line 172:
-``` +```text templates/ ├── frontend/ # React, Next, Nuxt, Svelte, Solid, React Native
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/test.yaml(1 hunks).idea/.gitignore(1 hunks).idea/biome.xml(1 hunks).idea/create-better-t-stack.iml(1 hunks).idea/git_toolbox_prj.xml(1 hunks).idea/inspectionProfiles/Project_Default.xml(1 hunks).idea/jsLibraryMappings.xml(1 hunks).idea/misc.xml(1 hunks).idea/modules.xml(1 hunks).idea/vcs.xml(1 hunks)CLAUDE.md(1 hunks)apps/cli/test/addons.test.ts(1 hunks)apps/cli/test/api.test.ts(2 hunks)apps/cli/test/auth.test.ts(2 hunks)apps/cli/test/backend-runtime.test.ts(2 hunks)apps/cli/test/basic-configurations.test.ts(1 hunks)apps/cli/test/database-orm.test.ts(3 hunks)apps/cli/test/database-setup.test.ts(2 hunks)apps/cli/test/deployment.test.ts(7 hunks)apps/cli/test/examples.test.ts(2 hunks)apps/cli/test/frontend.test.ts(1 hunks)apps/cli/test/index.test.ts(1 hunks)apps/cli/test/integration.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
**/*.{ts,tsx}: Use Id from './_generated/dataModel' to type document ids (e.g., Id<'users'>)
Ensure Record key/value types align with validators (e.g., v.record(v.id('users'), v.string()) => Record<Id<'users'>, string>)
Be strict with types for document ids; prefer Id<'table'> over string
Use 'as const' for string literals in discriminated unions
When using Array and Record types, declare with explicit generic types (e.g., const arr: Array = ...)
**/*.{ts,tsx}: Use TypeScript type aliases instead of interface declarations.
Do not use explicit return types in TypeScript.
Files:
apps/cli/test/index.test.tsapps/cli/test/frontend.test.tsapps/cli/test/auth.test.tsapps/cli/test/basic-configurations.test.tsapps/cli/test/backend-runtime.test.tsapps/cli/test/integration.test.tsapps/cli/test/deployment.test.tsapps/cli/test/examples.test.tsapps/cli/test/database-setup.test.tsapps/cli/test/api.test.tsapps/cli/test/database-orm.test.tsapps/cli/test/addons.test.ts
**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx,mjs,cjs}: Do not use dotenv; Bun auto-loads .env
UseBun.serve()for HTTP/WebSockets; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use built-inWebSocket; do not usews
PreferBun.fileovernode:fsreadFile/writeFile
UseBun.$instead ofexecafor shelling out
Files:
apps/cli/test/index.test.tsapps/cli/test/frontend.test.tsapps/cli/test/auth.test.tsapps/cli/test/basic-configurations.test.tsapps/cli/test/backend-runtime.test.tsapps/cli/test/integration.test.tsapps/cli/test/deployment.test.tsapps/cli/test/examples.test.tsapps/cli/test/database-setup.test.tsapps/cli/test/api.test.tsapps/cli/test/database-orm.test.tsapps/cli/test/addons.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/better-t-stack-repo.mdc)
Define functions using the standard function declaration syntax, not arrow functions.
Files:
apps/cli/test/index.test.tsapps/cli/test/frontend.test.tsapps/cli/test/auth.test.tsapps/cli/test/basic-configurations.test.tsapps/cli/test/backend-runtime.test.tsapps/cli/test/integration.test.tsapps/cli/test/deployment.test.tsapps/cli/test/examples.test.tsapps/cli/test/database-setup.test.tsapps/cli/test/api.test.tsapps/cli/test/database-orm.test.tsapps/cli/test/addons.test.ts
**/.github/workflows/**/*.@(yml|yaml)
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun run <script>instead ofnpm run,yarn run, orpnpm runin CI workflows
Files:
.github/workflows/test.yaml
🧠 Learnings (4)
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` to run tests
Applied to files:
apps/cli/test/index.test.tsapps/cli/test/frontend.test.tsapps/cli/test/auth.test.tsapps/cli/test/basic-configurations.test.tsapps/cli/test/backend-runtime.test.tsapps/cli/test/integration.test.tsapps/cli/test/deployment.test.tsapps/cli/test/examples.test.tsapps/cli/test/database-setup.test.tsapps/cli/test/api.test.tsapps/cli/test/addons.test.ts.github/workflows/test.yaml
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` instead of `jest` or `vitest`
Applied to files:
apps/cli/test/index.test.tsapps/cli/test/frontend.test.tsapps/cli/test/auth.test.tsapps/cli/test/basic-configurations.test.tsapps/cli/test/backend-runtime.test.tsapps/cli/test/integration.test.tsapps/cli/test/deployment.test.tsapps/cli/test/examples.test.tsapps/cli/test/database-setup.test.tsapps/cli/test/api.test.tsapps/cli/test/addons.test.ts
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to **/package.json : In package.json scripts, prefer running files with `bun <file>` instead of `node <file>` or `ts-node <file>`
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Default to using Bun instead of Node.js
Applied to files:
apps/cli/test/deployment.test.ts
🧬 Code graph analysis (12)
apps/cli/test/index.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/frontend.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/auth.test.ts (1)
apps/cli/test/test-utils.ts (2)
cleanupSmokeDirectory(31-38)expectError(174-179)
apps/cli/test/basic-configurations.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/backend-runtime.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/integration.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/deployment.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/examples.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/database-setup.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/api.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/database-orm.test.ts (1)
apps/cli/test/test-utils.ts (2)
cleanupSmokeDirectory(31-38)expectSuccess(162-172)
apps/cli/test/addons.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
🪛 LanguageTool
CLAUDE.md
[style] ~212-~212: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...that: - Returns structured InitResult with success status, project paths, and timing - Alw...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
117-117: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (26)
.idea/.gitignore (1)
1-5: Standard IDE configuration..idea/vcs.xml (1)
1-6: Standard IDE configuration..idea/misc.xml (1)
1-4: Standard IDE configuration..idea/jsLibraryMappings.xml (1)
1-6: Standard IDE configuration..idea/inspectionProfiles/Project_Default.xml (1)
1-6: Standard IDE configuration..idea/biome.xml (1)
1-9: Standard IDE configuration..idea/git_toolbox_prj.xml (1)
1-15: Standard IDE configuration..github/workflows/test.yaml (1)
1-29: GitHub Actions workflow follows guidelines and implements PR objectives.The workflow correctly:
- Triggers on pull requests to main
- Uses
bun installandbun testcommands (per coding guidelines and learnings)- Names the job "test" as required for branch protection rules
- Properly configures environment variables for telemetry and PostHog integration
- Runs tests in the correct working directory (apps/cli)
.idea/modules.xml (1)
1-8: LGTM!Standard IntelliJ IDEA module configuration file with correct structure.
.idea/create-better-t-stack.iml (1)
1-12: LGTM!Standard IntelliJ IDEA module configuration with appropriate folder exclusions.
apps/cli/test/api.test.ts (1)
21-27: LGTM!Cleanup lifecycle hooks are correctly implemented with proper async/await handling.
apps/cli/test/frontend.test.ts (1)
11-17: LGTM!Cleanup lifecycle hooks are correctly implemented with proper async/await handling.
apps/cli/test/basic-configurations.test.ts (1)
11-17: LGTM!Cleanup lifecycle hooks are correctly implemented with proper async/await handling.
apps/cli/test/index.test.ts (1)
9-18: LGTM!Cleanup lifecycle hooks are correctly implemented with proper async/await handling, consistent with the pattern across all test files.
apps/cli/test/database-setup.test.ts (1)
12-18: No race condition risk—tests execute sequentially.Verification confirms that tests run sequentially with
vitest run(no parallel flag set). Each project gets its own subdirectory within.smoke/based on projectName, so duplicate project names across test files (ai-solid-fail, trpc-nuxt-fail, web-deploy-no-frontend-fail) do not cause conflicts. The cleanup pattern is correctly implemented and safe.apps/cli/test/database-orm.test.ts (1)
12-18: LGTM! Proper test isolation with cleanup hooks.The addition of
beforeAllandafterAllhooks to clean up the smoke directory ensures tests run in a clean state and don't leave artifacts behind.apps/cli/test/examples.test.ts (1)
12-18: LGTM! Consistent test cleanup pattern.The lifecycle hooks ensure proper cleanup of the smoke directory before and after test execution.
apps/cli/test/backend-runtime.test.ts (2)
12-18: LGTM! Test isolation with cleanup hooks.Consistent with the cleanup pattern applied across the test suite.
474-477: LGTM! More informative error message.The updated error message now explicitly mentions TanStack Start as a supported frontend option alongside Next.js, improving developer experience.
apps/cli/test/auth.test.ts (2)
13-19: LGTM! Proper test cleanup.The lifecycle hooks ensure a clean test environment.
107-110: LGTM! More specific error message.The updated error message provides better context by explaining that the 'todo' example requires a database when a backend is present, rather than the generic "Authentication requires a database" message.
apps/cli/test/integration.test.ts (2)
12-18: LGTM! Test isolation established.Consistent cleanup pattern applied across the test suite.
202-203: LGTM! Deployment configuration updates.The changes to deployment settings and runtime configuration align with the test scenarios being validated.
Also applies to: 235-235
apps/cli/test/addons.test.ts (1)
12-18: LGTM! Clean test environment setup.The lifecycle hooks properly manage the smoke directory cleanup.
apps/cli/test/deployment.test.ts (2)
13-19: LGTM! Proper test cleanup.Consistent with the cleanup pattern applied throughout the test suite.
164-167: LGTM! Conditional runtime configuration for deployment targets.The runtime is now correctly set based on the deployment target:
workersruntime forwrangleroralchemyserver deploymentsbunruntime for other scenariosThis aligns with the deployment constraints where Cloudflare Workers and Alchemy require the workers runtime.
Also applies to: 251-253, 519-521
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yaml (1)
20-23: Consider removing the pnpm setup step if not needed.The workflow uses
bunexclusively for dependency installation and test execution. The pnpm setup step may be redundant unless it's required for workspace resolution or other toolchain requirements in your monorepo. Verify and remove if unnecessary.To verify, check:
- Does
bun installhandle your workspace/monorepo structure without pnpm setup?- Is pnpm needed elsewhere in the workflow or for lockfile compatibility?
If both answers are "no", remove lines 20-23.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/.github/workflows/**/*.@(yml|yaml)
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun run <script>instead ofnpm run,yarn run, orpnpm runin CI workflows
Files:
.github/workflows/test.yaml
🧠 Learnings (2)
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to **/.github/workflows/**/*.@(yml|yaml) : Use `bun run <script>` instead of `npm run`, `yarn run`, or `pnpm run` in CI workflows
Applied to files:
.github/workflows/test.yaml
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` to run tests
Applied to files:
.github/workflows/test.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
.github/workflows/test.yaml (1)
1-34: Workflow structure and tooling are correct.The workflow correctly uses
bun install --frozen-lockfile(line 26) andbun test(line 33) as specified in the coding guidelines. Checkout, bun setup, dependency installation, and test execution are all configured appropriately. Environment variables for telemetry and analytics are properly sourced from GitHub Secrets.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/.github/workflows/**/*.@(yml|yaml)
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun run <script>instead ofnpm run,yarn run, orpnpm runin CI workflows
Files:
.github/workflows/test.yaml
🧠 Learnings (1)
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to **/.github/workflows/**/*.@(yml|yaml) : Use `bun run <script>` instead of `npm run`, `yarn run`, or `pnpm run` in CI workflows
Applied to files:
.github/workflows/test.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
.github/workflows/test.yaml (1)
1-40: LGTM! Workflow structure is sound.The CI pipeline appropriately checks out code, configures git (needed for test operations), sets up Bun and pnpm, installs dependencies with frozen lockfile for reproducibility, and runs tests in the target directory. Environment variables are properly sourced from secrets.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bunfig.toml (1)
4-6: Configuration is valid but consider using array format for better CI reporting.The
coverageReporteroption in bunfig.toml can enable persistent code coverage reports, and the"lcov"format is correct. However, the coverageReporter accepts both string and array formats, with array format allowing multiple reporters like ["text", "lcov"].For CI environments, the array format is more valuable: you get both terminal output (for immediate feedback) and an lcov.info file (for integration with coverage tools and dashboards).
Apply this diff to enable both console and file-based coverage reporting:
[test] coverage = true -coverageReporter = "lcov" +coverageReporter = ["text", "lcov"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yaml(1 hunks).gitignore(1 hunks)bunfig.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bunfig.toml (1)
4-6: Consider adding coverage thresholds (optional).For future iterations, consider adding coverage threshold configurations (e.g., lines, statements, functions, branches) to enforce minimum coverage requirements and prevent regressions. This would help maintain test quality as the codebase grows.
[test] coverage = true coverageReporter = ["text", "lcov"] # Optional: add in a future update # coverageThreshold = { lines = 80, statements = 80, functions = 80, branches = 75 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bunfig.toml(1 hunks)
🔇 Additional comments (1)
bunfig.toml (1)
4-6: Coverage reporting configuration looks good.The new
[test]section enables coverage reporting with both "text" and "lcov" reporters, which is well-suited for CI: "text" provides readable output in logs, and "lcov" integrates with coverage tools. This aligns well with the enhanced test infrastructure and CI workflow being added.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cli/test/addons.test.ts(1 hunks)apps/cli/test/api.test.ts(2 hunks)apps/cli/test/auth.test.ts(2 hunks)apps/cli/test/deployment.test.ts(7 hunks)apps/cli/test/frontend.test.ts(1 hunks)apps/cli/test/integration.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/cli/test/frontend.test.ts
- apps/cli/test/addons.test.ts
- apps/cli/test/api.test.ts
- apps/cli/test/auth.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
**/*.{ts,tsx}: Use Id from './_generated/dataModel' to type document ids (e.g., Id<'users'>)
Ensure Record key/value types align with validators (e.g., v.record(v.id('users'), v.string()) => Record<Id<'users'>, string>)
Be strict with types for document ids; prefer Id<'table'> over string
Use 'as const' for string literals in discriminated unions
When using Array and Record types, declare with explicit generic types (e.g., const arr: Array = ...)
Files:
apps/cli/test/deployment.test.tsapps/cli/test/integration.test.ts
**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx,mjs,cjs}: Do not use dotenv; Bun auto-loads .env
UseBun.serve()for HTTP/WebSockets; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use built-inWebSocket; do not usews
PreferBun.fileovernode:fsreadFile/writeFile
UseBun.$instead ofexecafor shelling out
Files:
apps/cli/test/deployment.test.tsapps/cli/test/integration.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` instead of `jest` or `vitest`
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` to run tests
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` to run tests
Applied to files:
apps/cli/test/deployment.test.tsapps/cli/test/integration.test.ts
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` instead of `jest` or `vitest`
Applied to files:
apps/cli/test/deployment.test.tsapps/cli/test/integration.test.ts
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Default to using Bun instead of Node.js
Applied to files:
apps/cli/test/deployment.test.ts
🧬 Code graph analysis (2)
apps/cli/test/deployment.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
apps/cli/test/integration.test.ts (1)
apps/cli/test/test-utils.ts (1)
cleanupSmokeDirectory(31-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
apps/cli/test/integration.test.ts (4)
1-1: LGTM! Cleanup hooks improve test isolation.The suite-level cleanup hooks prevent test artifact buildup and ensure a clean state before and after test runs.
Also applies to: 4-4, 12-18
480-480: Runtime change consistent with Wrangler deployment.Similar to line 235, the runtime changed from
buntoworkers, aligning with thewranglerdeployment targets (lines 489-490). This ensures the maximum complexity test uses a realistic configuration.
235-235: Runtime change from "bun" to "workers" is correct and required.Validation rules confirm that Wrangler server deployment requires
"workers"runtime. The test configuration is valid: it uses "hono" backend (the only supported backend for workers), SQLite database, "drizzle" ORM, and Wrangler deployment—all validated combinations in the CLI.
202-203: Deployment configuration is correct: "none" is the appropriate value for Express backend.The Express backend does not have alchemy deployment setup support. Alchemy deployment is configured only for frontend frameworks (Next.js, Nuxt, React Router, Solid, Svelte, TanStack Router, TanStack Start), as evidenced by the lack of an
alchemy-express-setup.tsfile inapps/cli/src/helpers/deployment/alchemy/. Setting bothwebDeployandserverDeployto"none"for the Express + Node + MongoDB + Mongoose stack correctly reflects the CLI's actual constraints.
| beforeAll(async () => { | ||
| await cleanupSmokeDirectory(); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await cleanupSmokeDirectory(); | ||
| }); |
There was a problem hiding this comment.
Avoid per-suite cleanup of shared .smoke scratch space.
Vitest runs test files in parallel by default (and bun test keeps that behavior), so other CLI suites may still be generating projects under .smoke when this suite’s beforeAll/afterAll executes. Calling cleanupSmokeDirectory() here wipes the whole directory mid-run, which will intermittently take the other suites down with ENOENTs. Please move this cleanup into a global setup/teardown that runs once per test worker (or scope each suite to its own subdirectory and only delete that). As written, this introduces a cross-suite data race on the shared .smoke directory.
🤖 Prompt for AI Agents
In apps/cli/test/deployment.test.ts around lines 13 to 19, the suite-level
beforeAll/afterAll calls wipe the shared `.smoke` directory which races with
other parallel test suites; change this by removing the per-suite cleanup and
either (A) move cleanupSmokeDirectory() into a global setup/teardown that runs
once per test worker (e.g., Vitest globalSetup/globalTeardown or worker-scoped
hooks) so each worker owns its cleanup, or (B) scope this test suite to its own
unique subdirectory under `.smoke` (create a per-suite temp subfolder and only
delete that subfolder in beforeAll/afterAll) so no cross-suite deletion occurs.
|
So this fixes all the tests?? 👀 @moreorover |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 7
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
|
||
| if (serverDeploy !== "none" || config.backend === "none") { | ||
| config.runtime = "workers"; // Ensure runtime | ||
| } |
There was a problem hiding this comment.
Bug: Logic Error: 'None' Backend Runtime
The condition uses OR (||) when it should use AND (&&). When backend === "none", the runtime should be "none", not "workers". The current logic incorrectly sets runtime = "workers" for cases where backend === "none", which violates the constraint that backend "none" requires runtime "none".
| config.runtime = "bun"; | ||
| } else if (backend === "hono") { | ||
| config.runtime = "workers"; | ||
| config.serverDeploy = "wrangler"; |
There was a problem hiding this comment.
Bug: Incomplete Server Deployment Test Coverage
The test claims to verify server deployment with all compatible backends, but serverDeploy is only set for hono. For express, fastify, and elysia backends, serverDeploy remains undefined, so these backends aren't actually testing server deployment functionality despite the test name indicating they should.
|
Do note that i plan to remove wrangler from web deploy and server deploy so we dont need to write tests for it |
Thats good. For now these test cases can stay, once these options are removed we will remove tests together. This does not only fix the tests but also run a test pipeline on every PR to make sure that the changes do not break anything going forward. I have noticed that you tend to commit straight into main branch, with this PR going forward it is greatly suggested to open a new PR making any changes into main branch this will run tests and at least somewhat make sure that stuff is working. It will also show coverage on source code after the tests are executed, suggest registering for https://github.com/marketplace/actions/report-lcov so that the coverage report data is more readable and gives more insights compared to just pipeline report. There is still a lot to improve with tests, but really need to think of how can we best test generated projects by CLI. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cli/test/test-utils.ts (1)
320-395: PreferBun.$overexecafor shell commands.The function uses
execato run package manager and turbo commands, but the project coding guidelines specify usingBun.$instead ofexecafor shelling out.As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cli/test/test-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
**/*.{ts,tsx}: Use Id from './_generated/dataModel' to type document ids (e.g., Id<'users'>)
Ensure Record key/value types align with validators (e.g., v.record(v.id('users'), v.string()) => Record<Id<'users'>, string>)
Be strict with types for document ids; prefer Id<'table'> over string
Use 'as const' for string literals in discriminated unions
When using Array and Record types, declare with explicit generic types (e.g., const arr: Array = ...)
Files:
apps/cli/test/test-utils.ts
**/*.{js,jsx,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx,mjs,cjs}: Do not use dotenv; Bun auto-loads .env
UseBun.serve()for HTTP/WebSockets; do not useexpress
Usebun:sqlitefor SQLite; do not usebetter-sqlite3
UseBun.redisfor Redis; do not useioredis
UseBun.sqlfor Postgres; do not usepgorpostgres.js
Use built-inWebSocket; do not usews
PreferBun.fileovernode:fsreadFile/writeFile
UseBun.$instead ofexecafor shelling out
Files:
apps/cli/test/test-utils.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` instead of `jest` or `vitest`
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` to run tests
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` instead of `jest` or `vitest`
Applied to files:
apps/cli/test/test-utils.ts
📚 Learning: 2025-08-24T18:01:11.544Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-08-24T18:01:11.544Z
Learning: Applies to {**/package.json,**/@(jest|vitest).config.@(js|ts|mjs|cjs)} : Use `bun test` to run tests
Applied to files:
apps/cli/test/test-utils.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
|
@AmanVarshney01 something is wrong the the config package tests, so removed those, I do not have this week or next to have a look at it. But the rest is good to be merged in. |
2 participants
closes #684
Changes
CI/CD
.github/workflows/test.yaml) that runs on every PR tomainbun testinapps/clito ensure all tests pass before mergingImportant: Branch Protection
To maintain code quality and prevent broken builds from reaching
main, please:main. Create a PR for every change instead.mainbranch:maintest(the CI job name)This ensures that the test suite runs successfully before any code is merged into
main, catching issues early and maintaining a stable codebase.Testing
Run tests locally: