Skip to content

Comments

fix: unit test flake#23648

Merged
anikdhabal merged 4 commits intocalcom:mainfrom
anikdhabal:fix-unit-test-flakes
Sep 6, 2025
Merged

fix: unit test flake#23648
anikdhabal merged 4 commits intocalcom:mainfrom
anikdhabal:fix-unit-test-flakes

Conversation

@anikdhabal
Copy link
Contributor

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

  • Show screen recordings of the issue or feature.
  • Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

  • Add side-by-side screenshots of the original and updated change.
  • Highlight any significant change(s).

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@anikdhabal anikdhabal requested a review from a team as a code owner September 6, 2025 16:15
@anikdhabal anikdhabal requested a review from a team September 6, 2025 16:15
@graphite-app graphite-app bot requested a review from a team September 6, 2025 16:15
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 6, 2025

Walkthrough

Updated packages/app-store/googlecalendar/lib/tests/CalendarService.test.ts to replace a fixed performance threshold (speedupRatio > 5) with a CI-aware minimum. Introduced minSpeedup = process.env.CI ? 1.5 : 5 and updated the assertion to speedupRatio > minSpeedup. Added an inline comment explaining the lower threshold for CI. No other changes.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2025

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:

No release type found in pull request title "Fix unit test flake". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@vercel
Copy link

vercel bot commented Sep 6, 2025

@anikdhabal is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@keithwillcode keithwillcode added the core area: core, team members only label Sep 6, 2025
@anikdhabal anikdhabal changed the title Fix unit test flake fix: unit test flake Sep 6, 2025
@dosubot dosubot bot added the automated-tests area: unit tests, e2e tests, playwright label Sep 6, 2025
Comment on lines +1313 to +1314
const minSpeedup = process.env.CI ? 1.5 : 5; // Lower threshold for CI
expect(speedupRatio).toBeGreaterThan(minSpeedup);
Copy link
Contributor Author

@anikdhabal anikdhabal Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE:- Running all tests together in CI makes performance benchmarks unreliable. The 5x threshold becomes impossible to meet when CPU/memory is shared across many concurrent tests. Set a decent threshold in CI and keep it 5 when running this test independently.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts (2)

1313-1314: Harden perf assertion to avoid edge-case flakes (>= and env override).

Good direction. To eliminate equality-edge flakes and allow quick tuning in CI, consider ≥ and an overridable env threshold.

-      const minSpeedup = process.env.CI ? 1.5 : 5; // Lower threshold for CI
-      expect(speedupRatio).toBeGreaterThan(minSpeedup);
+      const envMin = Number(process.env.CI_MIN_SPEEDUP);
+      const minSpeedup = Number.isFinite(envMin) ? envMin : (process.env.CI ? 1.5 : 5); // CI overrideable
+      expect(speedupRatio).toBeGreaterThanOrEqual(minSpeedup);

764-764: Quiet noisy console logs in tests.

These logs add noise in CI and can mask failures.

-    console.log({ calendarCachesAfter });
+    // log.debug({ calendarCachesAfter });
-      console.log({ error });
+      // log.debug({ error });

Also applies to: 783-783

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1792d49 and 0900796.

📒 Files selected for processing (1)
  • packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.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:

  • packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.

Applied to files:

  • packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Tests / Unit
  • GitHub Check: Linters / lint
  • GitHub Check: Type check / check-types

Copy link
Contributor

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should plan to move this and other benchmark/perf tests we have to a different, dedicated perf suite that is more predictable.

@anikdhabal anikdhabal enabled auto-merge (squash) September 6, 2025 16:40
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2025

E2E results are ready!

@anikdhabal anikdhabal merged commit 5375c65 into calcom:main Sep 6, 2025
101 of 111 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated-tests area: unit tests, e2e tests, playwright core area: core, team members only ready-for-e2e size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants