Conversation
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Drizzle migrations, enables Bun as an alternative execution engine for build and test commands, and refactors E2E test execution logic to support Vitest and Bun. Feedback includes suggestions to simplify the migration command splitting logic to avoid redundant array operations and to deduplicate test target logic while ensuring consistent use of the "BUN" placeholder.
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the wb package, including support for Drizzle migrations, the ability to use Bun for running built files, and improved E2E test command generation that supports both Vitest and Bun. Additionally, it refines the default unit test targets to exclude E2E tests when using Vitest. A review comment suggests ensuring the --bail flag is consistently passed to the bun test command when the bail option is enabled, matching the behavior of the Vitest execution path.
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Drizzle migrations and Bun-based test execution, while also refining how Vitest handles end-to-end test exclusion. Key changes include the addition of drizzleScripts to the migration logic in BaseScripts and a refactored testCommand construction in HttpServerScripts to support both Vitest and Bun. Feedback suggests using the literal bun command instead of the BUN placeholder to avoid potential recursion issues during direct file execution. Additionally, it is recommended to refactor the complex testCommand construction logic into a shared utility to reduce redundancy and improve maintainability across the codebase.
| ? [ | ||
| `YARN wb buildIfNeeded ${argv.verbose ? '--verbose' : ''}`.trim(), | ||
| `node dist/index.js ${argv.normalizedArgsText ?? ''}`.trim(), | ||
| `${project.isBunAvailable ? 'BUN' : 'node'} dist/index.js ${argv.normalizedArgsText ?? ''}`.trim(), |
There was a problem hiding this comment.
Using the BUN placeholder here might lead to unexpected behavior if dist/index.js is intended to be executed as a file rather than a package script. As noted in httpServerScripts.ts, the BUN placeholder normalizes to bun --bun run, which could potentially recurse if a script with the same name exists in package.json. For direct file execution in Bun projects, using the literal bun command is often safer.
| `${project.isBunAvailable ? 'BUN' : 'node'} dist/index.js ${argv.normalizedArgsText ?? ''}`.trim(), | |
| `${project.isBunAvailable ? 'bun' : 'node'} dist/index.js ${argv.normalizedArgsText ?? ''}`.trim(), |
| const normalizedTargets = targets && targets.length > 0 ? targets : ['test/e2e/']; | ||
| const testCommand = project.hasVitest | ||
| ? buildShellCommand([ | ||
| 'vitest', | ||
| 'run', | ||
| ...normalizedTargets, | ||
| '--color', | ||
| '--passWithNoTests', | ||
| '--allowOnly', | ||
| ...(argv.bail ? ['--bail=1'] : []), | ||
| ]) | ||
| : project.isBunAvailable | ||
| ? // Use literal `bun test`; the `BUN` placeholder normalizes to `bun --bun run` and may recurse into a `test` package script. | ||
| buildShellCommand(['bun', 'test', ...normalizedTargets, ...(argv.bail ? ['--bail'] : [])]) | ||
| : 'echo "No tests."'; |
There was a problem hiding this comment.
The logic for constructing testCommand is becoming complex and is repeated in spirit across different script classes. Consider refactoring this into a helper method or a shared utility to improve maintainability and ensure consistency across different project types (e.g., handling of --bail vs --bail=1).
References
- Simplify code as much as possible to eliminate redundancy and design each module with high cohesion.
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the wb package, including support for Drizzle migrations, the ability to use Bun for running tests and executing built scripts, and improved test target handling. Specifically, it updates getDefaultUnitTargets to exclude E2E tests when Vitest is used, refactors BaseScripts to handle Drizzle migrations, and updates HttpServerScripts to support Bun as a fallback test runner. A suggestion was made to simplify the logic for normalizing test targets using optional chaining.
| const port = await checkAndKillPortProcess(project.env.PORT, project); | ||
| const suffix = project.packageJson.scripts?.['test/e2e-additional'] ? ' && YARN test/e2e-additional' : ''; | ||
| const targets = argv.targets?.map(String); | ||
| const normalizedTargets = targets && targets.length > 0 ? targets : ['test/e2e/']; |
There was a problem hiding this comment.
The condition targets && targets.length > 0 can be simplified to targets?.length for better readability and consistency with other parts of the codebase (e.g., in baseScripts.ts).
| const normalizedTargets = targets && targets.length > 0 ? targets : ['test/e2e/']; | |
| const normalizedTargets = targets?.length ? targets : ['test/e2e/']; |
References
- Simplify code as much as possible to eliminate redundancy. (link)
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Drizzle migrations and enables the use of Bun for executing scripts and tests when available. It also refactors the E2E test command generation to provide a fallback mechanism. Feedback suggests reusing the existing testUnit logic in HttpServerScripts to eliminate redundancy and ensure consistent behavior with Vitest and watch flags.
| private buildFallbackTestCommand(project: Project, argv: TestArgv, targets: string[]): string { | ||
| if (project.hasVitest) { | ||
| return buildShellCommand([ | ||
| 'vitest', | ||
| 'run', | ||
| ...targets, | ||
| '--color', | ||
| '--passWithNoTests', | ||
| '--allowOnly', | ||
| ...(argv.bail ? ['--bail=1'] : []), | ||
| ]); | ||
| } | ||
| if (project.isBunAvailable) { | ||
| // Use literal `bun test`; the `BUN` placeholder normalizes to `bun --bun run` and may recurse into a `test` package script. | ||
| return buildShellCommand(['bun', 'test', ...targets, ...(argv.bail ? ['--bail'] : [])]); | ||
| } | ||
| return 'echo "No tests."'; | ||
| } |
There was a problem hiding this comment.
This method duplicates logic already present in BaseScripts.testUnit. To maintain consistency and adhere to the principle of reducing redundancy, consider reusing this.testUnit(project, { ...argv, targets }) instead of defining a new fallback command. Note that testUnit also includes the YARN prefix for Vitest and handles the --watch=false flag, which makes it more robust for use within concurrently.
References
- Simplify code as much as possible to eliminate redundancy. (link)
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Drizzle migrations, enables Bun as an alternative runtime for production builds, and refactors E2E test execution to reuse unit test logic. A potential issue was identified regarding the migration logic, which prioritizes Prisma over Drizzle when both are present; this could cause issues for projects transitioning between the two ORMs.
| const migrationCommand = project.hasPrisma | ||
| ? prismaScripts.migrate(project) | ||
| : project.hasDrizzle | ||
| ? drizzleScripts.migrate(project) | ||
| : ''; |
There was a problem hiding this comment.
The current logic prioritizes Prisma migrations over Drizzle if both are present in a project. While it's uncommon to have both, it might be safer to check for both independently or ensure that this prioritization is intended. If a project is transitioning from Prisma to Drizzle, this could lead to unexpected behavior where only the old Prisma migrations are run.
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the test and execution scripts. Key changes include excluding E2E tests from default unit test targets when they reside in the 'test/e2e' directory, adding support for Drizzle migrations, and preferring 'bun' over 'node' for script execution when available. Additionally, the E2E test command generation in 'HttpServerScripts' was refactored for better consistency, and unit tests were updated accordingly. I have no feedback to provide.
1 participant
Summary
test/e2e/**from default Vitest unit runs so Playwright specs are not collected by Vitest whenwb testruns all tests.bun testfor Bun projects that do not have a Playwright config.Why
Cross-repo
wb testdebugging exposed target-project runtime gaps. Bun HTTP-server projects could be started or tested through Node/Vitest, and Drizzle-backed projects could reach E2E before their schema existed. Repos with only E2E tests plus Vitest could also have Playwright tests collected as unit tests.Testing
wb testvia local@willbooster/wbsource across repos listed in/Users/exkazuu/ghq/github.com/WillBooster/self-host-utils/scripts/process-repos.sh.moti-aiafter fixes:yarn workspace @willbooster/wb start --working-dir /Users/exkazuu/ghq/github.com/WillBooster/moti-ai test --silentpassed.understandability-survey; it now reaches Playwright tests instead of failing during Vitest collection. Remaining failures are app/test behavior in Playwright.yarn workspace @willbooster/wb buildyarn workspace @willbooster/wb testyarn check-for-aiNotes
Several repos in the process list were not cloned locally. Other failures from the initial scan were repo-local setup or dependency issues, such as missing installs, missing environment variables, or existing test failures.