- Notifications
You must be signed in to change notification settings - Fork11.2k
fix: resolve flaky integration tests#25030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
- Fixed isAdminGuard Prisma query to use explicit 'is' filter for organizationSettings- Fixed async describe with top-level awaits in _get.integration-test.ts- Added global setup in setupVitest.ts to prevent race conditions- Removed duplicate setup logic from individual test filesRoot cause: Tests were running in parallel with independent beforeAll setups,causing race conditions where organizationSettings weren't created beforetests executed. The async describe with top-level awaits made this worse byexecuting queries before beforeAll hooks ran.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:
|
The global setup in setupVitest.ts was running for ALL test workspaces(including unit tests), causing ECONNREFUSED errors because unit testsdon't have database access.Changes:- Created setupVitest.integration.ts with org-admin seeding logic- Removed database seeding from setupVitest.ts- Updated vitest.workspace.ts to use integration-only setup file- Added DATABASE_URL guard to prevent errors when DB is unavailableThis fixes the unit test failures while preserving the fix for flakyintegration tests.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
vercelbot commentedNov 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The latest updates on your projects. Learn more aboutVercel for GitHub. |
The previous fix using setupFiles didn't work because setupFiles runAFTER test modules are evaluated. This meant any top-level Prismaqueries in test files would execute before the org-admin seeding.Changes:- Moved org-admin seeding to tests/integration/global-setup.ts- Updated vitest.workspace.ts to use globalSetup for IntegrationTests- globalSetup runs BEFORE any test modules are loaded, ensuring org settings exist before tests execute- Added teardown function to properly disconnect Prisma after testsThis ensures org-admin state is seeded once before all integrationtests run, eliminating the race condition and ensuring tests havethe correct database state.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Udit-takkar left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@anikdhabal i had tried to something similar but it didn't fix the issue.#24990
Added console.log statements throughout the globalSetup to verify:- Whether the globalSetup is running at all- Whether DATABASE_URL is available- Whether the org teams are found in the database- Whether the upserts are executing successfullyThis will help diagnose why the integration tests are still failingwith org-admin not being detected.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Changed from relative path 'tests/integration/global-setup.ts' toabsolute path using new URL().pathname to ensure Vitest can properlylocate and load the globalSetup file.This should fix the issue where the globalSetup wasn't being executedat all (no logs appearing in CI).Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
github-actionsbot commentedNov 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
E2E results are ready! |
Added sequence.concurrent: false to IntegrationTests workspace to eliminateinter-file race conditions while stabilizing org-admin seeding. This ensurestests run one at a time, preventing parallel execution issues that couldcause flaky test failures.This is a temporary stabilizer that can be reverted once the globalSetupseeding is confirmed working.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Refactored globalSetup to use TeamRepository instead of direct Prismaaccess, following the 'No prisma outside of repositories' architecturalrule.Changes:- Created TeamRepository class with methods for finding organizations and upserting organization settings- Updated globalSetup to use TeamRepository.withGlobalPrisma()- Removed direct Prisma imports from globalSetupThis ensures proper separation of concerns and follows the repositorypattern established in the codebase.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Changed from package-scoped import '@calcom/lib/server/repository/team'to relative path import '../../packages/lib/server/repository/team' tofix module resolution issue.Added try/catch with logging around the import to surface any remainingresolution issues in CI logs. This should allow the globalSetup toexecute properly and seed org-admin state before tests run.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Root cause: CI database snapshot doesn't include the owner1-acme OWNER membership that exists in the current seed file, because cache-db action's cache key doesn't include scripts/seed.ts.Solution: Add ensureMembership method to TeamRepository and call it in globalSetup to ensure the owner1-acme user has an accepted OWNER membership in the Acme org before tests run.This fixes the 5 failing org-admin integration tests that depend on this membership.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
Add ensureUser method to TeamRepository to create users if they don't exist.Update ensureMembership to accept MEMBER role in addition to OWNER and ADMIN.Ensure all 10 member{0-9}-acme users are created with MEMBER role and accepted: true in the Acme org.This should fix the remaining 4 failing tests that expect multiple org members to exist.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>…raint violationsThe ensureUser method was using create which could fail if a user with that email already exists.Switch to upsert to make the operation idempotent and avoid P2002 unique constraint errors.Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
…tion- Remove all console.log debug statements from global-setup.ts- Remove serial execution from IntegrationTests workspace (restore parallel execution)- Move TeamRepository to tests/lib/test-team-repository.ts and rename to TestTeamRepository- Keep all actual fixes: isAdmin Prisma query fix, ensureUser/ensureMembership methods, globalSetup seedingCo-Authored-By: anik@cal.com <adhabal2002@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
3 issues found across 8 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.<file name="vitest.workspace.ts"><violation number="1" location="vitest.workspace.ts:30">`new URL(...).pathname` percent-encodes spaces and prepends a slash on Windows, so Vitest cannot resolve `global-setup.ts`. Use `fileURLToPath(new URL(...))` instead so the global setup works on all environments.</violation></file><file name="apps/api/v1/test/lib/bookings/_get.integration-test.ts"><violation number="1" location="apps/api/v1/test/lib/bookings/_get.integration-test.ts:25">Please add a select to this booking lookup so we only retrieve the id we assert against, keeping the query compliant with our Prisma select-only policy.</violation></file><file name="tests/lib/test-team-repository.ts"><violation number="1" location="tests/lib/test-team-repository.ts:110">This lookup should add a select so we only fetch the user id needed by ensureMembership, avoiding unnecessary data exposure.</violation></file>React with 👍 or 👎 to teach cubic. Mention@cubic-dev-ai to give feedback, ask questions, or re-run the review.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| exportdefaultasyncfunctionglobalSetup(){ | ||
| constmodule=awaitimport("../lib/test-team-repository"); | ||
| constTestTeamRepository=module.TestTeamRepository; | ||
| constteamRepo=awaitTestTeamRepository.withGlobalPrisma(); | ||
| constacmeOrg=awaitteamRepo.findOrganizationBySlug("acme"); | ||
| if(acmeOrg){ | ||
| awaitteamRepo.upsertOrganizationSettings({ | ||
| organizationId:acmeOrg.id, | ||
| isAdminAPIEnabled:true, | ||
| orgAutoAcceptEmail:"acme.com", | ||
| }); | ||
| awaitteamRepo.ensureMembership({ | ||
| userEmail:"owner1-acme@example.com", | ||
| organizationId:acmeOrg.id, | ||
| role:"OWNER", | ||
| accepted:true, | ||
| }); | ||
| for(leti=0;i<10;i++){ | ||
| constmemberEmail=`member${i}-acme@example.com`; | ||
| constmemberUsername=`member${i}-acme`; | ||
| constmemberName=`Member${i}`; | ||
| awaitteamRepo.ensureUser({ | ||
| email:memberEmail, | ||
| username:memberUsername, | ||
| name:memberName, | ||
| }); | ||
| awaitteamRepo.ensureMembership({ | ||
| userEmail:memberEmail, | ||
| organizationId:acmeOrg.id, | ||
| role:"MEMBER", | ||
| accepted:true, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why are we running all this? we already seed data for acme org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we only need toupsertOrganizationSettings
hbjORbj left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would be great if you can leave self-review comments so that it's easier to understand why the flaky tests are fixed by the changes in the PR!
emrysal left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK for now.
4e5d4f6 intomainUh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
4 issues found across 8 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.<file name="tests/lib/test-team-repository.ts"><violation number="1" location="tests/lib/test-team-repository.ts:24">Rule violated: **Enforce Singular Naming for Single-Item Functions**Rename this method to use a singular noun so it’s clear the Prisma upsert returns a single settings record, in line with the singular naming rule for single-item functions.</violation><violation number="2" location="tests/lib/test-team-repository.ts:26">The update branch should persist orgAutoAcceptEmail; otherwise existing org settings keep the stale value instead of the domain passed in by global-setup, so the test seeding still fails to produce the expected state.</violation></file><file name="vitest.workspace.ts"><violation number="1" location="vitest.workspace.ts:30">Deriving the globalSetup path with `.pathname` keeps percent-encoding (e.g., spaces become %20), so Vitest cannot load the setup file when the repo lives in a directory containing spaces. Prefer converting the file URL with fileURLToPath instead.</violation></file><file name="tests/integration/global-setup.ts"><violation number="1" location="tests/integration/global-setup.ts:19">`ensureMembership` throws if the user record is missing, but this global setup never creates owner1-acme@example.com before assigning the OWNER role. Please create the owner user (e.g., via `ensureUser`) or otherwise guarantee the user exists before calling `ensureMembership` to avoid setup failures.</violation></file>React with 👍 or 👎 to teach cubic. Mention@cubic-dev-ai to give feedback, ask questions, or re-run the review.
| }); | ||
| } | ||
| asyncupsertOrganizationSettings({ |
cubic-dev-aibotNov 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Rule violated:Enforce Singular Naming for Single-Item Functions
Rename this method to use a singular noun so it’s clear the Prisma upsert returns a single settings record, in line with the singular naming rule for single-item functions.
Prompt for AI agents
Address the following comment on tests/lib/test-team-repository.ts at line 24:<comment>Rename this method to use a singular noun so it’s clear the Prisma upsert returns a single settings record, in line with the singular naming rule for single-item functions.</comment><file context>@@ -0,0 +1,144 @@+ });+ }++ async upsertOrganizationSettings({+ organizationId,+ isAdminAPIEnabled,</file context>
| name:`IntegrationTests`, | ||
| include:["packages/**/*.integration-test.ts","apps/**/*.integration-test.ts"], | ||
| exclude:["**/node_modules/**/*","packages/embeds/**/*"], | ||
| globalSetup:newURL("./tests/integration/global-setup.ts",import.meta.url).pathname, |
cubic-dev-aibotNov 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Deriving the globalSetup path with.pathname keeps percent-encoding (e.g., spaces become %20), so Vitest cannot load the setup file when the repo lives in a directory containing spaces. Prefer converting the file URL with fileURLToPath instead.
Prompt for AI agents
Address the following comment on vitest.workspace.ts at line 30:<comment>Deriving the globalSetup path with `.pathname` keeps percent-encoding (e.g., spaces become %20), so Vitest cannot load the setup file when the repo lives in a directory containing spaces. Prefer converting the file URL with fileURLToPath instead.</comment><file context>@@ -27,6 +27,7 @@ const workspaces = packagedEmbedTestsOnly name: `IntegrationTests`, include: ["packages/**/*.integration-test.ts", "apps/**/*.integration-test.ts"], exclude: ["**/node_modules/**/*", "packages/embeds/**/*"],+ globalSetup: new URL("./tests/integration/global-setup.ts", import.meta.url).pathname, setupFiles: ["setupVitest.ts"], },</file context>| asyncupsertOrganizationSettings({ | ||
| organizationId, | ||
| isAdminAPIEnabled, |
cubic-dev-aibotNov 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The update branch should persist orgAutoAcceptEmail; otherwise existing org settings keep the stale value instead of the domain passed in by global-setup, so the test seeding still fails to produce the expected state.
Prompt for AI agents
Address the following comment on tests/lib/test-team-repository.ts at line 26:<comment>The update branch should persist orgAutoAcceptEmail; otherwise existing org settings keep the stale value instead of the domain passed in by global-setup, so the test seeding still fails to produce the expected state.</comment><file context>@@ -0,0 +1,144 @@++ async upsertOrganizationSettings({+ organizationId,+ isAdminAPIEnabled,+ orgAutoAcceptEmail,+ }: {</file context>| orgAutoAcceptEmail:"acme.com", | ||
| }); | ||
| awaitteamRepo.ensureMembership({ |
cubic-dev-aibotNov 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ensureMembership throws if the user record is missing, but this global setup never createsowner1-acme@example.com before assigning the OWNER role. Please create the owner user (e.g., viaensureUser) or otherwise guarantee the user exists before callingensureMembership to avoid setup failures.
Prompt for AI agents
Address the following comment on tests/integration/global-setup.ts at line 19:<comment>`ensureMembership` throws if the user record is missing, but this global setup never creates owner1-acme@example.com before assigning the OWNER role. Please create the owner user (e.g., via `ensureUser`) or otherwise guarantee the user exists before calling `ensureMembership` to avoid setup failures.</comment><file context>@@ -0,0 +1,58 @@+ orgAutoAcceptEmail: "acme.com",+ });++ await teamRepo.ensureMembership({+ userEmail: "owner1-acme@example.com",+ organizationId: acmeOrg.id,</file context>
Uh oh!
There was an error while loading.Please reload this page.
What does this PR do?
Fixes flaky org-admin integration tests that were failing 9/10 times in CI and blocking PRs from merging.
Root Cause: Three interconnected issues caused the flakiness:
organizationSettingsin its ownbeforeAllwhile tests ran in parallel, causing some tests to execute before the settings were created_get.integration-test.tshad top-level awaits insidedescribe()that executed BEFOREbeforeAll, so Prisma queries ran before org settings existedorganizationSettings: { isAdminAPIEnabled: true }which fails when the relation doesn't existThe Fix:
isAdminGuardPrisma query to use explicitis: {}filter syntax (more robust when relation doesn't exist)asyncand moving top-level awaits intobeforeAllsetupVitest.tsthat runs once before all tests (eliminates race condition)