Conversation
…ugh strategic consolidation Originally by: Udit-takkar _PR description is being written. Please check back in a minute._ Devin Session: https://app.devin.ai/sessions/43e00bc6cc03442484a35e383ccac06d <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Consolidated and cleaned up E2E test files to reduce redundancy, improve execution speed, and lower CI resource usage. Added timing measurement tools and reduced sharding to further optimize test runs. - **Refactors** - Merged booking, authentication, and organization tests into fewer, more focused files. - Removed 8 redundant or obsolete E2E test files. - Reduced E2E shards from 4 to 3 and lowered timeout from 20 to 15 minutes. - **New Features** - Added a workflow and script for E2E timing measurement and analysis. <!-- End of auto-generated description by cubic. -->
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
cursor review |
1 similar comment
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: E2E Tests Fail Due to Missing User Initialization
Multiple new Playwright E2E tests attempt to retrieve a user from the users fixture using users.get() without first creating any users. This causes the tests to fail as no users exist in the fixture. The tests should use await users.create() to provision a user before attempting to retrieve one.
apps/web/playwright/authentication.e2e.ts#L7-L10
cal.com/apps/web/playwright/authentication.e2e.ts
Lines 7 to 10 in d6a255d
apps/web/playwright/organization-consolidated.e2e.ts#L8-L11
cal.com/apps/web/playwright/organization-consolidated.e2e.ts
Lines 8 to 11 in d6a255d
apps/web/playwright/booking-advanced.e2e.ts#L8-L11
cal.com/apps/web/playwright/booking-advanced.e2e.ts
Lines 8 to 11 in d6a255d
apps/web/playwright/booking-core.e2e.ts#L8-L11
cal.com/apps/web/playwright/booking-core.e2e.ts
Lines 8 to 11 in d6a255d
Bug: Booking Limits Test Coverage Lost
The test.fixme block for Per ${limitUnit} booking limits was accidentally removed, deleting approximately 130 lines of test logic. This eliminated test coverage for booking limits, including reschedule functionality. The forEach loop iterating BOOKING_LIMITS_SINGLE now contains no test logic and is effectively dead code, resulting in a significant loss of test coverage.
apps/web/playwright/booking-limits.e2e.ts#L60-L63
cal.com/apps/web/playwright/booking-limits.e2e.ts
Lines 60 to 63 in d6a255d
Was this report helpful? Give feedback by reacting with 👍 or 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a significant optimization of the E2E test suite through strategic consolidation and improved test organization. The changes focus on reducing redundancy and improving execution speed while maintaining comprehensive test coverage.
Key changes include:
- Consolidation of related tests into focused files (booking-core.e2e.ts, booking-advanced.e2e.ts, authentication.e2e.ts, organization-consolidated.e2e.ts)
- Removal of 8 redundant test files with functionality merged into consolidated files
- Infrastructure improvements including reduction from 4 to 3 test shards and timeout reduction from 20 to 15 minutes
- Addition of timing measurement tools and analysis scripts to guide future optimizations
The consolidation maintains test coverage while reducing CI resource usage and execution time. The new consolidated test files are well-structured with parallel test execution configuration.
Confidence score: 4/5
- This PR is safe to merge with careful validation of the consolidated test coverage
- Score based on thorough consolidation strategy, maintained test coverage, and addition of timing measurement tools
- Files needing attention:
- booking-core.e2e.ts and booking-advanced.e2e.ts: Verify all critical booking scenarios are covered
- authentication.e2e.ts: Check 2FA test implementation
- organization-consolidated.e2e.ts: Ensure all org-specific edge cases are preserved
17 files reviewed, 13 comments
Edit PR Review Bot Settings | Greptile
| test("Should handle organization booking flow", async ({ page, users }) => { | ||
| const [user] = users.get(); | ||
| await user.apiLogin(); | ||
|
|
||
| await page.goto("/settings/organizations/new"); | ||
| await page.locator('[name="name"]').fill("Test Organization"); | ||
| await page.locator('[name="slug"]').fill("test-org"); | ||
| await page.locator('[data-testid="create-organization"]').click(); | ||
|
|
||
| await page.goto("/event-types"); | ||
| await page.locator('[data-testid="new-event-type"]').click(); | ||
| await page.locator("[name=title]").fill("Org Event"); | ||
| await page.locator("[name=slug]").fill("org-event"); | ||
| await page.locator("[data-testid=update-eventtype]").click(); | ||
|
|
||
| await page.goto("/org/test-org/org-event"); | ||
| await selectFirstAvailableTimeSlotNextMonth(page); | ||
| await page.locator('[name="name"]').fill("John Doe"); | ||
| await page.locator('[name="email"]').fill("john@example.com"); | ||
| await page.locator('[data-testid="confirm-book-button"]').click(); | ||
|
|
||
| await expect(page.locator('[data-testid="success-page"]')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
style: Organization booking flow test creates a new org for each test. Consider using beforeAll() to create a single org for all booking-related tests to reduce test execution time.
| test("Should require 2FA code when enabled", async ({ page, users }) => { | ||
| const [user] = users.get(); | ||
| await user.apiLogin(); | ||
|
|
||
| await page.goto("/settings/security/two-factor-auth"); | ||
| await page.locator('[data-testid="enable-2fa-button"]').click(); | ||
|
|
||
| await page.goto("/auth/logout"); | ||
| await page.goto("/auth/login"); | ||
|
|
||
| await page.locator('[name="email"]').fill(user.email); | ||
| await page.locator('[name="password"]').fill(user.username || "password"); | ||
| await page.locator('[type="submit"]').click(); | ||
|
|
||
| await expect(page.locator('[data-testid="2fa-input"]')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
logic: Test needs cleanup after 2FA enablement - enabled 2FA could affect other tests. Consider adding afterEach hook to disable 2FA
| await page.locator("[name=slug]").fill("event-with-questions"); | ||
| await page.locator("[data-testid=update-eventtype]").click(); | ||
|
|
||
| const eventTypeId = await page.locator("[data-testid=update-eventtype]").getAttribute("data-id"); |
There was a problem hiding this comment.
logic: getAttribute('data-id') could return null. Add null check before using eventTypeId.
| const eventTypeId = await page.locator("[data-testid=update-eventtype]").getAttribute("data-id"); | |
| const eventTypeId = await page.locator("[data-testid=update-eventtype]").getAttribute("data-id"); | |
| if (!eventTypeId) { | |
| throw new Error("Event type ID not found"); | |
| } |
| entries(BOOKING_LIMITS_SINGLE).forEach(([limitKey, bookingLimit]) => { | ||
| const limitUnit = intervalLimitKeyToUnit(limitKey); | ||
|
|
||
| // test one limit at a time | ||
| test.fixme(`Per ${limitUnit}`, async ({ page, users }) => { | ||
| const slug = `booking-limit-${limitUnit}`; | ||
| const singleLimit = { [limitKey]: bookingLimit }; | ||
|
|
||
| const user = await createUserWithLimits({ | ||
| users, | ||
| slug, | ||
| length: EVENT_LENGTH, | ||
| bookingLimits: singleLimit, | ||
| }); | ||
|
|
||
| let slotUrl = ""; | ||
|
|
||
| const monthUrl = getLastEventUrlWithMonth(user, firstMondayInBookingMonth); | ||
| await page.goto(monthUrl); | ||
|
|
||
| const availableDays = page.locator('[data-testid="day"][data-disabled="false"]'); | ||
| const bookingDay = availableDays.getByText(firstMondayInBookingMonth.date().toString(), { | ||
| exact: true, | ||
| }); | ||
|
|
||
| // finish rendering days before counting | ||
| await expect(bookingDay).toBeVisible({ timeout: 10_000 }); | ||
| const availableDaysBefore = await availableDays.count(); | ||
|
|
||
| let latestRescheduleUrl: string | null = null; | ||
| await test.step("can book up to limit", async () => { | ||
| for (let i = 0; i < bookingLimit; i++) { | ||
| await bookingDay.click(); | ||
|
|
||
| await page.getByTestId("time").nth(0).click(); | ||
| await bookTimeSlot(page); | ||
|
|
||
| slotUrl = page.url(); | ||
|
|
||
| await expect(page.getByTestId("success-page")).toBeVisible(); | ||
| latestRescheduleUrl = await page | ||
| .locator('span[data-testid="reschedule-link"] > a') | ||
| .getAttribute("href"); | ||
|
|
||
| await page.goto(monthUrl); | ||
| } | ||
| }); | ||
|
|
||
| const expectedAvailableDays = { | ||
| day: -1, | ||
| week: -5, | ||
| month: 0, | ||
| year: 0, | ||
| }; | ||
|
|
||
| await test.step("but not over", async () => { | ||
| // should already have navigated to monthUrl - just ensure days are rendered | ||
| await expect(page.getByTestId("day").nth(0)).toBeVisible(); | ||
|
|
||
| // ensure the day we just booked is now blocked | ||
| await expect(bookingDay).toBeHidden({ timeout: 10_000 }); | ||
|
|
||
| const availableDaysAfter = await availableDays.count(); | ||
|
|
||
| // equals 0 if no available days, otherwise signed difference | ||
| expect(availableDaysAfter && availableDaysAfter - availableDaysBefore).toBe( | ||
| expectedAvailableDays[limitUnit] | ||
| ); | ||
|
|
||
| // try to book directly via form page | ||
| await page.goto(slotUrl); | ||
| await expectSlotNotAllowedToBook(page); | ||
| }); | ||
|
|
||
| await test.step("but can reschedule", async () => { | ||
| const bookingId = latestRescheduleUrl?.split("/").pop(); | ||
| const rescheduledBooking = await prisma.booking.findFirstOrThrow({ where: { uid: bookingId } }); | ||
|
|
||
| const year = rescheduledBooking.startTime.getFullYear(); | ||
| const month = String(rescheduledBooking.startTime.getMonth() + 1).padStart(2, "0"); | ||
| const day = String(rescheduledBooking.startTime.getDate()).padStart(2, "0"); | ||
|
|
||
| await page.goto( | ||
| `/${user.username}/${ | ||
| user.eventTypes.at(-1)?.slug | ||
| }?rescheduleUid=${bookingId}&date=${year}-${month}-${day}&month=${year}-${month}` | ||
| ); | ||
|
|
||
| const formerDay = availableDays.getByText(rescheduledBooking.startTime.getDate().toString(), { | ||
| exact: true, | ||
| }); | ||
| await expect(formerDay).toBeVisible(); | ||
|
|
||
| const formerTimeElement = page.locator('[data-testid="former_time_p"]'); | ||
| await expect(formerTimeElement).toBeVisible(); | ||
|
|
||
| await page.locator('[data-testid="time"]').nth(0).click(); | ||
|
|
||
| await expect(page.locator('[name="name"]')).toBeDisabled(); | ||
| await expect(page.locator('[name="email"]')).toBeDisabled(); | ||
|
|
||
| await confirmReschedule(page); | ||
|
|
||
| await expect(page.locator("[data-testid=success-page]")).toBeVisible(); | ||
|
|
||
| const newBooking = await prisma.booking.findFirstOrThrow({ where: { fromReschedule: bookingId } }); | ||
| expect(newBooking).not.toBeNull(); | ||
|
|
||
| const updatedRescheduledBooking = await prisma.booking.findFirstOrThrow({ | ||
| where: { uid: bookingId }, | ||
| }); | ||
| expect(updatedRescheduledBooking.status).toBe(BookingStatus.CANCELLED); | ||
|
|
||
| await prisma.booking.deleteMany({ | ||
| where: { | ||
| id: { | ||
| in: [newBooking.id, rescheduledBooking.id], | ||
| }, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| await test.step(`month after booking`, async () => { | ||
| await page.goto(getLastEventUrlWithMonth(user, firstMondayInBookingMonth.add(1, "month"))); | ||
|
|
||
| // finish rendering days before counting | ||
| await expect(page.getByTestId("day").nth(0)).toBeVisible({ timeout: 10_000 }); | ||
|
|
||
| // the month after we made bookings should have availability unless we hit a yearly limit | ||
| await expect((await availableDays.count()) === 0).toBe(limitUnit === "year"); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
style: Empty test block should be removed completely rather than leaving placeholder. This forEach loop now does nothing.
| await page.locator('[name="email"]').fill(user.email); | ||
| await page.locator('[name="password"]').fill(user.username || "password"); |
There was a problem hiding this comment.
logic: Using username as password fallback is unsafe. Should use a dedicated test password
| await selectFirstAvailableTimeSlotNextMonth(page); | ||
| await page.locator('[name="name"]').fill("John Doe"); | ||
| await page.locator('[name="email"]').fill("john@example.com"); | ||
| await page.locator('[name="responses.Select your preference"]').selectOption("Option 1"); |
There was a problem hiding this comment.
style: This selector may be fragile - use a more specific data-testid for form responses instead of relying on the question text in the selector
| const eventTypeId = await page.locator("[data-testid=update-eventtype]").getAttribute("data-id"); | ||
| await page.goto(`/event-types/${eventTypeId}?tabName=limits`); | ||
|
|
||
| await page.locator('input[name="durationLimits.PER_DAY"]').fill("60"); |
There was a problem hiding this comment.
logic: Consider adding a validation check for the duration limit being respected - currently only verifies the success page
| if: always() | ||
| run: | | ||
| END_TIME=$(date +%s) | ||
| DURATION=$((END_TIME - E2E_START_TIME)) | ||
| echo "E2E execution time for shard ${{ matrix.shard }}: ${DURATION} seconds" | ||
| echo "SHARD_DURATION=${DURATION}" >> $GITHUB_ENV |
There was a problem hiding this comment.
style: SHARD_DURATION is set but never used. Remove if not needed.
| if: always() | |
| run: | | |
| END_TIME=$(date +%s) | |
| DURATION=$((END_TIME - E2E_START_TIME)) | |
| echo "E2E execution time for shard ${{ matrix.shard }}: ${DURATION} seconds" | |
| echo "SHARD_DURATION=${DURATION}" >> $GITHUB_ENV | |
| if: always() | |
| run: | | |
| END_TIME=$(date +%s) | |
| DURATION=$((END_TIME - E2E_START_TIME)) | |
| echo "E2E execution time for shard ${{ matrix.shard }}: ${DURATION} seconds" |
| async loadTimingData() { | ||
| try { | ||
| const files = fs.readdirSync(this.timingDataPath); | ||
|
|
||
| for (const file of files) { | ||
| if (file.endsWith('-timing.json')) { | ||
| const filePath = path.join(this.timingDataPath, file); | ||
| const data = JSON.parse(fs.readFileSync(filePath, 'utf8')); | ||
| this.shardData.push(data); | ||
| } | ||
| } | ||
|
|
||
| console.log(`Loaded timing data for ${this.shardData.length} shards`); | ||
| } catch (error) { | ||
| console.error('Error loading timing data:', error.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: Missing proper error propagation - should throw or set error state
| console.log(` Estimated time saving: ${rec.estimatedTimeSaving}`); | ||
| }); | ||
|
|
||
| fs.writeFileSync('e2e-optimization-report.json', JSON.stringify(report, null, 2)); |
There was a problem hiding this comment.
style: Add try/catch around file write operation
| fs.writeFileSync('e2e-optimization-report.json', JSON.stringify(report, null, 2)); | |
| try { | |
| fs.writeFileSync('e2e-optimization-report.json', JSON.stringify(report, null, 2)); | |
| console.log('\n📄 Detailed report saved to: e2e-optimization-report.json'); | |
| } catch (error) { | |
| console.error('Error saving report:', error.message); | |
| } |
1 participant
This is a copy of calcom#22484
Originally by: Udit-takkar
PR description is being written. Please check back in a minute.
Devin Session: https://app.devin.ai/sessions/43e00bc6cc03442484a35e383ccac06d
Summary by cubic
Consolidated and cleaned up E2E test files to reduce redundancy, improve execution speed, and lower CI resource usage. Added timing measurement tools and reduced sharding to further optimize test runs.
Refactors
New Features