chore(ci): run on develop PRs and fail on lint/test failures#36
Conversation
CI was gated to PRs targeting main and won't run for the typical feature -> develop flow used in this repo. Even when CI did run, both steps had continue-on-error: true so failures didn't block merges. Tests on develop have been broken since 601db2e (April 30) without anyone noticing. - Switch pull_request branches from main to develop - Drop continue-on-error from lint and test steps Also wires up two pieces needed for yarn test to actually run: - Root yarn test now runs turbo run test (was bare jest, which failed because there's no root jest config — turbo dispatches to each workspace's own test script and rebuilds shared-types beforehand via dependsOn ^build). - Adds a missing jest mock for utils/pricing in the verifier test setup. utils/pricing transitively pulls p-queue (ESM), the same reason @shapeshiftoss/swapper is already mocked. Drops the unused coverage outputs key from the turbo test task to silence its missing-output warning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR modernizes test infrastructure by migrating to Turbo-based test execution, enforces stricter test failure handling in CI, and updates the CI workflow to trigger on develop branch instead of main. Test setup mocking is added to support verification tests. ChangesTest Infrastructure Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
package.json (1)
21-21: ⚡ Quick winDocument turbo test task requirement for workspace consistency
The root
"test": "turbo run test"will only execute packages that define atestscript. Currently all workspaces with test files (only swap-service) have the script defined, so there's no gap. However, to prevent future issues if tests are added to notifications-service, user-service, shared-types, or shared-utils, document this requirement clearly—either in a contribution guide or add a workspace script template/lint rule ensuring any new tests include a correspondingtestscript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 21, Add documentation or automation to ensure every workspace that contains tests defines a "test" script so the root "test": "turbo run test" runs them all; specifically, update the contribution guide (e.g., CONTRIBUTING.md) to state that any workspace (notifications-service, user-service, shared-types, shared-utils, swap-service) with tests must include a "test" script in its package.json, or add a workspace package.json template or linter/CI rule that enforces presence of the "test" script when test files are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@package.json`:
- Line 21: Add documentation or automation to ensure every workspace that
contains tests defines a "test" script so the root "test": "turbo run test" runs
them all; specifically, update the contribution guide (e.g., CONTRIBUTING.md) to
state that any workspace (notifications-service, user-service, shared-types,
shared-utils, swap-service) with tests must include a "test" script in its
package.json, or add a workspace package.json template or linter/CI rule that
enforces presence of the "test" script when test files are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad4b7d44-f105-4546-bd64-9a63172fa1e5
📒 Files selected for processing (4)
.github/workflows/ci.ymlapps/swap-service/src/verification/__tests__/setup.tspackage.jsonturbo.json
…r tests Use actions/setup-node's built-in yarn cache, generate the prisma client in the shared setup step so lint sees @prisma/client types, and drop the ^build dependency on test (ts-jest transpiles source directly). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ESLint runs prettier on ts/js via eslint-plugin-prettier, so the format script only needs to cover the file types eslint doesn't lint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove entries for tools and artifacts this repo doesn't produce (Pulumi, nyc, lerna, Eclipse, etc.) and ignore Claude Code's local-only files (settings.local.json, worktrees/) while leaving room to track shared settings, commands, and agents. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shared-utils and shared-types declare main/types at ./dist/, so without a build their consumers' types fall back to error and trigger no-unsafe-* in typed lint. Filter the build to packages/* (cheap, turbo-cached) and rely on the build task's existing dep on db:generate to also produce the prisma client. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Description
CI was gated to PRs targeting
mainand won't run for the typical feature → develop flow used in this repo. Even when CI did run, both steps hadcontinue-on-error: trueso failures didn't block merges. Tests on develop have been broken since601db2e(April 30) without anyone noticing.pull_requestbranches frommaintodevelopcontinue-on-errorfrom lint and test stepsAlso wires up two pieces needed for
yarn testto actually run:yarn testnow runsturbo run test(was barejest, which failed because there's no root jest config — turbo dispatches to each workspace's own test script and rebuildsshared-typesbeforehand viadependsOn ^build).utils/pricingin the verifier test setup.utils/pricingtransitively pullsp-queue(ESM), the same reason@shapeshiftoss/swapperis already mocked.Drops the unused coverage
outputskey from the turbotesttask to silence its missing-output warning.Testing
yarn testfrom repo root passes (4 turbo tasks, 22 jest tests)yarn lintfrom repo root passesSummary by CodeRabbit