Improve test infrastructure and Jest configuration #1437
Conversation
- Fix Jest moduleNameMapper order and add missing path mappings - Add support for testing apps/workers in Jest node config - Improve setupEnv.test.ts with proper TypeScript types for child_process mocks - Remove duplicate moduleNameMapper entry and organize path mappings logically 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
Caution Review failedThe pull request is closed. 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. WalkthroughUpdates adjust Jest module alias mappings to add specific Changes
Possibly related PRs
Suggested labels
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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
🧹 Nitpick comments (2)
__tests__/lib/utils/setupEnv.test.ts (2)
39-51: Consider defensive handling for the callback parameter.The flexible parameter handling correctly accommodates both exec overload signatures. However, the type assertion
as ExecCallbackon line 45 could be unsafe ifmaybeCallbackis null/undefined whenoptionsOrCallbackis an ExecOptions object.While this is likely safe in the test context (since
setupEnvpresumably always provides a callback), consider adding a null check or runtime assertion for robustness:🔎 Suggested defensive handling
const callback: ExecCallback = typeof optionsOrCallback === "function" ? optionsOrCallback - : (maybeCallback as ExecCallback) + : maybeCallback ?? (() => { + throw new Error("exec mock called without callback") + })
69-87: Same callback handling pattern - consider defensive handling.The flexible parameter extraction logic here mirrors the success test (lines 42-45). The same consideration about the type assertion on line 75 applies here.
🔎 Suggested defensive handling
const callback: ExecCallback = typeof optionsOrCallback === "function" ? optionsOrCallback - : (maybeCallback as ExecCallback) + : maybeCallback ?? (() => { + throw new Error("exec mock called without callback") + })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/config/jest.config.base.ts__tests__/config/jest.config.node.ts__tests__/lib/utils/setupEnv.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
__tests__/config/jest.config.node.ts__tests__/lib/utils/setupEnv.test.ts__tests__/config/jest.config.base.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
📚 Learning: 2025-09-04T14:57:04.612Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/code-structure.mdc:0-0
Timestamp: 2025-09-04T14:57:04.612Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mts,mjs} : Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Applied to files:
__tests__/config/jest.config.base.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: Cursor Bugbot
🔇 Additional comments (3)
__tests__/config/jest.config.node.ts (1)
13-17: LGTM! Test scope expanded appropriately.The addition of apps tests to the testMatch array correctly extends Jest's coverage to include the apps directory, aligning with the PR objective to improve test infrastructure for apps/workers.
__tests__/config/jest.config.base.ts (1)
12-16: LGTM! Module mapper ordering is correct.The reordering ensures that specific path mappings are evaluated before the generic
@/catch-all pattern (now on line 16). This is the correct approach for Jest's moduleNameMapper, which applies patterns in order and uses the first match.The new mappings for
shared/(line 12) and internal shared paths (lines 14-15) appropriately resolve to<rootDir>/shared/src/, while the generic pattern handles all other@/imports.__tests__/lib/utils/setupEnv.test.ts (1)
7-26: LGTM! Type safety improvements enhance maintainability.The addition of proper TypeScript types for child_process imports and the ExecCallback type definition improves code clarity and type safety for the mock implementation.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
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.
## Summary
Implement webhook handling for PR labels to trigger the createDependentPR workflow when PRs are labeled with "I2PR: Update PR".
## Features Implemented
### 🔗 Webhook Handler Foundation
- **New Handler**: `handlePullRequestLabelCreateDependentPR` enqueues createDependentPR jobs
- **Enhanced Schema**: Extended `PullRequestPayloadSchema` with `label`, `number`, and `sender` fields
- **Route Integration**: Added PR labeled event routing in webhook endpoint
### 🎯 How It Works
1. **GitHub Event**: PR gets labeled with "I2PR: Update PR"
2. **Webhook Processing**: Validates payload and extracts job data
3. **Job Queuing**: Enqueues `createDependentPR` job with:
- `repoFullName` (e.g., "owner/repo")
- `pullNumber` (PR number)
- `githubLogin` (who applied the label)
- `githubInstallationId` (for GitHub App auth)
4. **Worker Processing**: (Next PR) Worker will create dependent branch/PR
### 🧪 Test Coverage
- [x] Handler validates required fields and environment
- [x] Handler enqueues jobs with correct payload structure
- [x] Webhook route correctly routes labeled events
- [x] Integration test covers full webhook → handler flow
## Technical Details
### New Files
```
lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts
__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts
__tests__/api/webhook/github.route.test.ts
```
### Modified Files
```
app/api/webhook/github/route.ts - Added labeled event routing
lib/webhook/github/types.ts - Extended PullRequestPayloadSchema
```
### Example Usage
When a PR is labeled "I2PR: Update PR", this triggers:
```typescript
// Enqueues Redis job:
{
name: "createDependentPR",
data: {
repoFullName: "owner/repo",
pullNumber: 42,
githubLogin: "developer",
githubInstallationId: "12345"
}
}
```
## Context & Sequence
This PR is part of breaking down the large PR #1413 into manageable pieces:
1. ✅ **PR #1437**: Test infrastructure fixes
2. ✅ **PR #1438**: Container security enhancements
3. ✅ **This PR**: Webhook handler foundation
4. 🔄 **Next**: Worker infrastructure (Redis/Neo4j setup)
5. 🔄 **Final**: CreateDependentPR core workflow
## Testing
```bash
pnpm test __tests__/lib/webhook/handlers/pullRequest/
pnpm test __tests__/api/webhook/github.route.test.ts
```
Addresses the webhook handling requirements from **Issue #1399**.
🤖 Generated with [Claude Code](https://claude.ai/code)
---
Update: Adjust handler to no-op per review feedback
- Addressed RLC-001 and RLC-002: The worker does not yet recognize a `createDependentPR` job name. To keep this PR scoped to webhook routing and avoid enqueueing unknown jobs, the handler now performs a no-op.
- Changes:
- `handlePullRequestLabelCreateDependentPR` no longer enqueues a job. It validates required fields, logs a clear message indicating the webhook was received, and returns a small `{ status: "noop", ... }` result.
- Updated unit tests to reflect the no-op behavior (removed Redis/job enqueue expectations; assert logging/return value instead).
- Route tests remain the same (they mock the handler and only assert routing/invocation).
Rationale
- This keeps the boundary clean and avoids enqueueing jobs that would fail in the worker until the job schema/handler support is added in a follow-up PR.
Verification
- Jest tests for the updated handler and webhook route pass locally:
- `__tests__/lib/webhook/handlers/pullRequest/label.createDependentPR.handler.test.ts`
- `__tests__/api/webhook/github.route.test.ts`
Next steps
- Follow-up PR to register the `createDependentPR` job type in the worker and wire up the actual enqueue logic.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* Pull requests can now be automatically processed when labeled with "i2pr: update pr"
* Enhanced webhook system to recognize and route pull request label events with proper signature verification
* **Tests**
* Added comprehensive unit tests for webhook routing and signature verification
* Added tests for pull request label event handling, including validation of required payload fields
<sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Fix Jest test infrastructure to support testing new worker and shared functionality added in the createDependentPR feature chain.
What Changed
"^@/(.*)$"to end to prevent it from overriding more specific mappings"^shared/(.*)$"and clean architecture paths foradapters|entities|lib|ports|providers|services|ui|usecases|utilschild_process.execmocks to prevent type errorsWhy These Changes Were Needed
This fixes test infrastructure issues that emerged during the createDependentPR feature development:
shared/modules and worker orchestrators were failing due to unresolved importsTest Plan
shared/and worker import paths properlyThis enables the test infrastructure needed for the remaining PRs in the createDependentPR feature chain.
🤖 Generated with Claude Code
Note
Improves Jest resolution and test coverage, plus tightens types in a CLI test.
__tests__/config/jest.config.base.ts:^shared/(.*)$and grouped clean-architecture alias^@/(adapters|entities|ports|providers|services|ui|usecases|utils)(/.*)?$to map toshared/src@sharedand@workersmappings; move catch‑all^@/(.*)$to the end to avoid shadowing specific aliases__tests__/config/jest.config.node.ts:testMatchto include**/__tests__/apps/**/*.ts?(x)__tests__/lib/utils/setupEnv.test.ts:child_process.execmock (ChildProcess,ExecException,ExecOptions) and a typed callback; adjust mock implementations and casts accordinglyWritten by Cursor Bugbot for commit e42310c. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.