feat: add race condition test with dynamic date selection #22533
Conversation
- Recreate race condition test from PR #22401 for calendar-cache-serve booking flow - Replace hardcoded TEST_DATE with dynamic first available slot selection - Use getDynamicBookingDate function to select first available day dynamically - Prevent test flakiness by avoiding hardcoded tomorrow dates - Test validates concurrent booking prevention and cache functionality Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughA new end-to-end test file has been added to validate the prevention of double-booking race conditions in a calendar booking system. The test establishes a round-robin team environment with multiple hosts, sets up deterministic calendar data, and mocks Google Calendar API responses. It simulates concurrent booking attempts for the same time slot using two browser contexts, then analyzes the outcomes to verify that double bookings are prevented either by database constraints or by assigning different hosts. The test also checks calendar cache functionality and round-robin logic, and logs detailed diagnostic results. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
✅ No security or compliance issues detected. Reviewed everything up to c5f7775. Security Overview
Detected Code Changes
Reply to this PR with |
E2E results are ready! |
…ency Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
…r efficiency" This reverts commit b4b7d35.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/29/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/web/playwright/booking-race-condition.e2e.ts (6)
347-347: Move type alias to the top of the file.For better code organization and readability, move this type alias near the other type imports at the top of the file.
Move this line after line 10:
import type { teamMetadataSchema } from "@calcom/prisma/zod-utils"; export type TeamWithMetadata = Team & { metadata: z.infer<typeof teamMetadataSchema> };
145-155: Consider adding a comment to clarify test credentials.While these are clearly test credentials, adding a comment would make it explicit that these tokens are for testing purposes only and not real credentials.
key: { + // Test credentials only - not real tokens access_token: "test_access_token", refresh_token: "test_refresh_token",
349-349: Remove unnecessary export.This function is only used within this test file and doesn't need to be exported.
-export type TeamWithMetadata = Team & { metadata: z.infer<typeof teamMetadataSchema> }; +type TeamWithMetadata = Team & { metadata: z.infer<typeof teamMetadataSchema> };
366-368: Extract duplicated date calculation logic.This date calculation logic is duplicated from
getDynamicBookingDate. Consider passingselectedDateISOas a parameter instead.Update the function signature and remove the duplicate calculation:
async function performConcurrentBookings( page: Page, browser: Browser, org: TeamWithMetadata, team: TeamWithMetadata, teamEvent: EventType, - selectedDate: string + selectedDate: string, + selectedDateISO: string ) { - const currentDate = new Date(); - const nextMonth = new Date(currentDate.getFullYear(), currentDate.getMonth() + 1, parseInt(selectedDate)); - const selectedDateISO = nextMonth.toISOString();Then update the call site at line 73-80 to pass both values.
408-408: Remove unnecessary export.This function is only used within this test file and doesn't need to be exported.
-export async function analyzeBookingResults( +async function analyzeBookingResults(
413-418: Optimize database query.The query fetches 10 bookings but only uses the first 2. Consider limiting the query to what's actually needed.
const bookings = await prisma.booking.findMany({ where: { eventTypeId }, include: { attendees: true }, orderBy: { createdAt: "desc" }, - take: 10, + take: 2, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/playwright/booking-race-condition.e2e.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/booking-race-condition.e2e.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/playwright/booking-race-condition.e2e.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
apps/web/playwright/booking-race-condition.e2e.ts (3)
Learnt from: CarinaWolli
PR: #22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
⏰ 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). (2)
- GitHub Check: required
- GitHub Check: labeler
🔇 Additional comments (6)
apps/web/playwright/booking-race-condition.e2e.ts (6)
56-93: Well-structured test implementation!The test effectively validates race condition prevention with proper setup, concurrent booking simulation, and result analysis. Good use of dynamic date selection and detailed logging for diagnostics.
95-126: Excellent setup function with proper data selection!The function correctly creates the test environment with round-robin scheduling and follows best practices by using
selectinstead ofincludein Prisma queries.
161-188: Good test data setup with proper relationships!The function creates realistic booking histories with attendees and uses relative timestamps appropriately.
190-227: Excellent implementation of dynamic date selection!The function properly handles navigation, waits for elements, and includes error handling. This successfully addresses the flakiness issue from hardcoded dates mentioned in the PR objectives.
229-284: Well-implemented calendar cache setup!The function correctly uses the calendar cache system with proper date range expansion using
getTimeMin/getTimeMaxutilities. The availability data setup creates a realistic scenario for testing race conditions.
286-303: Efficient feature flag setup!Good use of
createManyfor batch insertion of feature flags.
|
@zomars all checks are green. Can you pls review it and merge? |
feat: add race condition test with dynamic date selection
Summary
This PR recreates the race condition test from PR #22401 with a key improvement: replacing the hardcoded
TEST_DATEwith dynamic first available slot selection to prevent test flakiness.Key Changes:
apps/web/playwright/booking-race-condition.e2e.tswith the same race condition testing logic as the original PRgetDynamicBookingDate()function that selects the first available day dynamicallypage.locator('[data-testid="day"][data-disabled="false"]').nth(0)to find the first available slotDynamic Date Selection Implementation:
getDynamicBookingDate()navigates to the booking page, increments to next month, and extracts the first available daysetupCalendarCache,mockGoogleCalendarAPI,performConcurrentBookings) now acceptselectedDateISOparameter instead of using hardcoded datesReview & Testing Checklist for Human
🔴 CRITICAL - Test was not successfully executed during development due to environment issues
TZ=UTC PLAYWRIGHT_HEADLESS=1 yarn e2e booking-race-condition.e2e.ts(repeat 5 times)Recommended Test Plan:
Diagram
%%{ init : { "theme" : "default" }}%% graph TD Test["apps/web/playwright/<br/>booking-race-condition.e2e.ts"]:::major-edit TestUtils["apps/web/playwright/lib/<br/>testUtils.ts"]:::context OrgFixture["apps/web/playwright/<br/>fixtures/orgs.ts"]:::context UserFixture["apps/web/playwright/<br/>fixtures/users.ts"]:::context Test --> TestUtils Test --> OrgFixture Test --> UserFixture Test --> GetDynamicDate["getDynamicBookingDate()"]:::major-edit Test --> SetupCache["setupCalendarCache()"]:::major-edit Test --> MockAPI["mockGoogleCalendarAPI()"]:::major-edit Test --> ConcurrentBookings["performConcurrentBookings()"]:::major-edit GetDynamicDate --> TestUtils subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
Link to Devin run: https://app.devin.ai/sessions/55c22ac32dfa4345ae51aaa6f3183992