Improve unit test workflow performance #2096
Conversation
|
WalkthroughThe changes introduce a modular and sharded approach to running unit tests in the repository's continuous integration setup. Three new GitHub Actions workflows are added: one each for webapp, packages, and internal unit tests, each supporting sharded execution and report merging. The main unit test workflow is refactored to delegate to these reusable workflows. In addition, the root Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (25)
🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
package.json (1)
57-57: Centralize and pin Vitest dependencies at the root.
Adding@vitest/coverage-v8and updatingvitestto3.1.4in the rootpackage.jsonappropriately centralizes testing tooling. Verify compatibility with existing workflows and consider using caret ranges (e.g.,^3.1.4) if you’d like to automatically pick up future patch releases.Also applies to: 68-68
.github/workflows/unit-tests-packages.yml (1)
14-16: Consider future sharding expansion for packages.Currently using a single shard for package tests. Consider expanding to multiple shards if package test execution time becomes a bottleneck in the future.
.github/workflows/unit-tests-webapp.yml (2)
84-88: Consider using GitHub secrets for test database credentials.While these appear to be test credentials for local PostgreSQL, hardcoding them in the workflow file could set a precedent for less secure practices. Consider moving these to GitHub secrets or documenting that these are intentionally non-sensitive test values.
- env: - DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres - DIRECT_URL: postgresql://postgres:postgres@localhost:5432/postgres - SESSION_SECRET: "secret" - MAGIC_LINK_SECRET: "secret" - ENCRYPTION_KEY: "secret" + env: + DATABASE_URL: ${{ secrets.TEST_DATABASE_URL || 'postgresql://postgres:postgres@localhost:5432/postgres' }} + DIRECT_URL: ${{ secrets.TEST_DIRECT_URL || 'postgresql://postgres:postgres@localhost:5432/postgres' }} + SESSION_SECRET: ${{ secrets.TEST_SESSION_SECRET || 'secret' }} + MAGIC_LINK_SECRET: ${{ secrets.TEST_MAGIC_LINK_SECRET || 'secret' }} + ENCRYPTION_KEY: ${{ secrets.TEST_ENCRYPTION_KEY || 'secret' }}🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 84-85: Basic Auth Credentials
(CKV_SECRET_4)
14-16: Consider making the shard configuration dynamic.The shard count is hardcoded to 10. For better flexibility and reusability, consider accepting these as workflow inputs.
Add workflow inputs:
on: workflow_call: inputs: shard-total: description: 'Total number of shards' type: number default: 10Then update the matrix:
- shardIndex: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] - shardTotal: [10] + shardIndex: ${{ fromJson(format('[{0}]', join(range(1, inputs.shard-total + 1), ','))) }} + shardTotal: [${{ inputs.shard-total }}]apps/webapp/test/runsReplicationService.part2.test.ts (1)
225-226: Consider dynamic wait strategies instead of fixed timeouts.The fixed 5-second wait might be insufficient under heavy CI load or excessive in optimal conditions. Consider implementing a polling mechanism that checks for replication completion.
// Helper function to wait for replication async function waitForReplication(clickhouse: ClickHouse, expectedCount: number, maxWaitMs = 10000) { const pollInterval = 500; const startTime = Date.now(); while (Date.now() - startTime < maxWaitMs) { const [err, result] = await clickhouse.reader.query({ query: "SELECT count(*) as count FROM trigger_dev.task_runs_v2 FINAL", schema: z.object({ count: z.number() }) })({}); if (!err && result?.[0]?.count >= expectedCount) { return; } await setTimeout(pollInterval); } throw new Error(`Replication did not complete within ${maxWaitMs}ms`); }Also applies to: 344-345
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/unit-tests-internal.yml(1 hunks).github/workflows/unit-tests-packages.yml(1 hunks).github/workflows/unit-tests-webapp.yml(1 hunks).github/workflows/unit-tests.yml(1 hunks)apps/supervisor/package.json(1 hunks)apps/webapp/package.json(1 hunks)apps/webapp/test/runsReplicationService.part1.test.ts(1 hunks)apps/webapp/test/runsReplicationService.part2.test.ts(1 hunks)internal-packages/clickhouse/package.json(1 hunks)internal-packages/redis/package.json(0 hunks)internal-packages/replication/package.json(0 hunks)internal-packages/run-engine/package.json(1 hunks)internal-packages/testcontainers/package.json(1 hunks)internal-packages/tracing/package.json(0 hunks)internal-packages/zod-worker/package.json(1 hunks)package.json(2 hunks)packages/cli-v3/package.json(1 hunks)packages/core/package.json(1 hunks)packages/redis-worker/package.json(1 hunks)
💤 Files with no reviewable changes (3)
- internal-packages/redis/package.json
- internal-packages/tracing/package.json
- internal-packages/replication/package.json
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/unit-tests-webapp.yml
[MEDIUM] 84-85: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (21)
internal-packages/testcontainers/package.json (1)
19-19: Verify necessity of newtinyexecdependency.
Thetinyexecpackage was added to devDependencies. Please confirm that it’s required for testcontainer operations and doesn’t overlap with existing utilities in this package.packages/redis-worker/package.json (1)
40-40: Confirm the addition oftsxin devDependencies.
Thetsxdependency was introduced but no scripts reference it in this package. Ensure it’s intentional—remove it if it’s not being used.apps/supervisor/package.json (1)
25-25: Ensure test scripts resolvevitestfrom the root workspace.
vitestwas removed from local devDependencies, yet scripts still callvitest. Confirm that the root-levelvitestis hoisted and accessible for package-local test runs.internal-packages/clickhouse/package.json (1)
17-17: Verify centralized testing strategy after local removal of Vitest.
With onlyrimrafleft in devDependencies and test scripts still invokingvitest, ensure that the intended centralized Vitest installation is picked up and that tests execute correctly in CI and locally.packages/core/package.json (1)
215-215: Minor reordering of devDependencies entry
Thetsxdependency remains unchanged but has shifted position due to the removal of local Vitest. No functional impact.apps/webapp/package.json (1)
257-257: Verify updated vite-tsconfig-paths version
The version ofvite-tsconfig-pathshas been updated to^4.0.5. Please confirm that this upgrade is fully compatible with the project's Vite and Remix setup and does not introduce any build issues.packages/cli-v3/package.json (1)
68-68: Ensure tsx support for update-version script
Thetsxdependency has been added to support theupdate-versionscript invocation. Verify that this aligns with the root-level devDependencies and that the script runs correctly in all development environments.internal-packages/zod-worker/package.json (1)
17-17: Confirm necessity of@types/pg
A@types/pgentry was introduced in devDependencies. Please verify that this package is actually used byzod-workeror its tests. If it's not required, consider removing it to keep dependencies lean.internal-packages/run-engine/package.json (1)
38-38: Verify upgrade to majorrimrafversion
TherimrafdevDependency has been bumped to6.0.1. Since this is a major version upgrade, please confirm that the clean script (rimraf dist) and any programmatic use of the API remain compatible.apps/webapp/test/runsReplicationService.part1.test.ts (2)
13-13: Excellent test organization for performance optimization.The split of integration tests into part 1/2 is a smart approach that aligns perfectly with the PR's goal of improving unit test workflow performance. This allows for better parallelization and sharding in the CI pipeline.
11-11: Good timeout configuration for integration tests.The 60-second timeout is appropriate for container-based integration tests that involve database replication and multiple service interactions.
.github/workflows/unit-tests.yml (1)
10-18: Excellent modular architecture for improved performance.The refactoring from a monolithic job to three separate reusable workflows is a significant improvement that enables:
- Parallel execution: Each test category (webapp, packages, internal) can run independently
- Targeted sharding: Each workflow can implement its own optimal sharding strategy
- Resource optimization: Different test types can use appropriate runner configurations
- Maintainability: Separation of concerns makes the CI configuration easier to manage
This modular approach directly supports the PR's goal of reducing test time from 21+ minutes to ~6.5 minutes.
.github/workflows/unit-tests-packages.yml (3)
22-46: Excellent Docker configuration for CI stability.The IPv6 disabling and Docker address pool configuration are excellent practices that prevent common networking issues in CI environments. The daemon restart ensures configuration takes effect.
65-73: Smart conditional DockerHub login implementation.The conditional login based on secret availability is a good practice that:
- Avoids rate limits when credentials are available
- Gracefully handles environments without secrets
- Provides clear logging for debugging
84-98: Robust artifact collection and retention strategy.The blob report collection logic properly:
- Handles test cancellation scenarios with
if: ${{ !cancelled() }}- Flattens nested report structures for easier merging
- Uses appropriate 1-day retention for temporary CI artifacts
- Includes hidden files which may contain important test metadata
.github/workflows/unit-tests-internal.yml (3)
14-16: Appropriate sharding strategy for internal tests.Using 8 shards for internal tests shows good understanding of the test execution time distribution. This higher shard count compared to packages (1 shard) suggests internal tests require more parallelization, which is well-reasoned.
84-90: Consistent artifact collection pattern.The blob report collection logic is identical to the packages workflow, demonstrating good consistency and reusability of patterns across the CI configuration.
100-130: Efficient report merging strategy.The merge-reports job efficiently:
- Runs only when needed (
if: ${{ !cancelled() }})- Skips dependency caching since it doesn't install packages
- Uses pattern matching to collect all shard reports
- Leverages Vitest's built-in report merging capabilities
.github/workflows/unit-tests-webapp.yml (2)
22-46: LGTM! Docker configuration addresses CI environment constraints.The IPv6 disabling and custom address pool configuration effectively prevent network conflicts in the CI environment, which is essential for running containerized integration tests reliably.
90-105: Excellent test report collection and artifact management.The implementation properly:
- Collects Vitest blob reports from all subdirectories
- Preserves package context in filenames for better organization
- Uses appropriate retention period for CI artifacts
- Handles cancellation scenarios gracefully
apps/webapp/test/runsReplicationService.part2.test.ts (1)
432-507: Well-designed transaction replication test.This test effectively validates:
- Multiple operations within a single transaction
- Proper replication of both inserts and updates
- Correct use of parameterized queries for ClickHouse
- Appropriate assertions for data integrity
This decreases unit test duration from >21m to ~6m30s.
vitestand@vitest/coverage-v8have been updated and locked to the latest version. They are now only present on the root package.json just like typescript, tsx, etc.