perf(test): cache admin sign-in tokens in event.test.ts#5261
perf(test): cache admin sign-in tokens in event.test.ts#5261h30s wants to merge 20 commits intoPalisadoesFoundation:developfrom
Conversation
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces per-test admin sign-ins with a suite-scoped cached admin auth ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5261 +/- ##
===========================================
+ Coverage 93.69% 95.37% +1.67%
===========================================
Files 861 906 +45
Lines 71332 78321 +6989
Branches 9893 9144 -749
===========================================
+ Hits 66838 74695 +7857
+ Misses 4494 3626 -868
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
test/graphql/types/Query/event.test.ts (5)
344-356:⚠️ Potential issue | 🟠 MajorLeftover inline sign-in defeats the PR's stated optimization goal
This test still performs a raw
Query_signInmutation instead of callinggetAdminToken(). The PR claims to reduce sign-ins from "13+ per test run to a single sign-in," but this test is one of the sign-ins that was supposed to be eliminated.🛠️ Proposed fix
- test("provided event ID is not a valid ULID.", async () => { - const signInResult = await mercuriusClient.query(Query_signIn, { - variables: { - input: { - emailAddress: - server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, - password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, - }, - }, - }); - - const authToken = signInResult.data?.signIn?.authenticationToken; - assertToBeNonNullish(authToken); + test("provided event ID is not a valid ULID.", async () => { + const authToken = await getAdminToken();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 344 - 356, This test is still performing a raw Query_signIn call (signInResult / authToken) instead of using the shared helper; replace the inline mercuriusClient.query(Query_signIn, ...) sign-in logic with a call to getAdminToken() and use its returned token for authentication; update any variables (signInResult, authToken) to use the token returned by getAdminToken and remove the Query_signIn dependency so the test uses the central single-sign-in optimization.
444-475:⚠️ Potential issue | 🔴 CriticalMissing cleanup — organization and event are never deleted
This top-level test creates an organization (line 450) and an event (line 456) with no cleanup. The outer suite has no
afterEach, so both entities accumulate in the database across runs.Consider wrapping the setup/teardown in a try/finally block, or adding an
afterEachat the outer suite level that deletes known test-owned organizations/events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 444 - 475, The test "admin user can access event from any organization" creates resources (via createTestOrganization and createTestEvent) but never cleans them up; wrap the setup and assertions in a try/finally (or add an outer afterEach) and in the finally block delete the created event and organization using your teardown helpers or API calls (use the returned event.id and organization.id), ensuring cleanup runs even on assertion failures; keep the mercuriusClient.query/Query_event/assertToBeNonNullish logic intact and only add the finally cleanup that calls the appropriate delete functions for the event and organization.
62-64:⚠️ Potential issue | 🟠 Major
beforeAllfor admin auth token caching conflicts with maintainer policyUsing
beforeAllto cache the admin auth token is the crux of this PR's optimization, but it directly contradicts an established repo policy. If the cached token becomes invalid mid-run (e.g., due to a session expiry or a preceding test that modifies auth state in ways not anticipated), every subsequent test that depends on it fails—and the failure is silent and hard to attribute. More critically, this pattern is explicitly discouraged for sharded CI environments where shard ordering and resource state can vary.The guideline is clear:
beforeAllis acceptable only for "truly immutable shared setup (e.g., test utilities, constants)"; auth tokens must be created per-test.Based on learnings: "In talawa-api tests, creating shared resources like authentication tokens in
beforeAllis discouraged. Auth tokens should be created per-test inbeforeEachor within individual tests to prevent cascading failures and improve test isolation in sharded environments." (palisadoes, 2026-02-07). As per coding guidelines fortest/**: "Auth tokens should be created per-test to avoid cascading failures."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 62 - 64, The test suite currently calls ensureAdminAuth() in a beforeAll (the cached admin token), which violates the repo policy requiring per-test auth creation; change the hook to beforeEach(async () => { await ensureAdminAuth(); }) or call ensureAdminAuth() inside each affected test so each test gets a fresh token, remove any global/cached token usage, and ensure tests that reference the admin token (via the existing helper used by these tests) use the freshly-created token rather than a shared one.
479-592:⚠️ Potential issue | 🔴 Critical
testCleanupFunctionsis never populated in three tests — cleanup hook does nothingThe "Additional event tests" suite has an
afterEachthat drainstestCleanupFunctions, but the following tests never push anything into it:
Test Resources created without cleanup "handles events with past dates" (line 491) createTestOrganizationresult + directeventsTableinsert"handles multi-day events" (line 539) createTestOrganizationresult +Mutation_createEventresult"creates a never-ending recurring event" (line 593) createTestOrganizationresult + recurring event template + instancesAdditionally, the invite-only tests (lines 659–1195) push cleanup for the regular user but not for the organization or event created in each test.
Each test should push cleanup functions for every resource it creates:
// Example for "handles events with past dates" testCleanupFunctions.push(async () => { await server.drizzleClient.delete(eventsTable).where(eq(eventsTable.id, pastEventId)); // organization deletion (if a delete mutation exists) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 479 - 592, The afterEach drains testCleanupFunctions but many tests never register cleanup; for each test that calls createTestOrganization, server.drizzleClient.insert(eventsTable) or mercuriusClient.mutate(Mutation_createEvent) (and the recurring event/template flow used by the "creates a never-ending recurring event" tests), push corresponding cleanup lambdas onto testCleanupFunctions that delete the created event rows (use eventsTable/eq on id via server.drizzleClient.delete), remove any recurring templates/instances, and delete the created organization (and user if created) so resources are removed; also update the invite-only tests that currently only push cleanup for the regular user to also push cleanup for the organization and event created in those tests.
381-442:⚠️ Potential issue | 🔴 CriticalMissing user cleanup — created user is never deleted
This top-level test creates a regular user via
Mutation_createUser(line 391) but never callsMutation_deleteUser. There is noafterEachat the outersuite("Query field event", ...)level, so the user leaks into the database after every run. In sharded CI, this accumulates across shards.🧹 Proposed fix
+ const userId = user.user?.id; + assertToBeNonNullish(userId); // Try to access event as regular user const queryResult = await mercuriusClient.query(Query_event, { ... }); expect(queryResult.data.event).toBeNull(); expect(queryResult.errors).toEqual(...); + + // Cleanup: delete the created user + await mercuriusClient.mutate(Mutation_deleteUser, { + headers: { authorization: `bearer ${adminAuthToken}` }, + variables: { input: { id: userId } }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 381 - 442, The test creates a regular user via Mutation_createUser (stored in variable user) but never deletes it; fix by recording created user.id (e.g., push to a local createdUserIds array) and ensure you call Mutation_deleteUser for each id after the test (or add an afterEach in the surrounding suite) using the adminAuthToken in the headers; reference Mutation_createUser, user.id, and Mutation_deleteUser (and adminAuthToken) so deletion runs even on failures (use finally/afterEach) to prevent leaked users across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 344-356: This test is still performing a raw Query_signIn call
(signInResult / authToken) instead of using the shared helper; replace the
inline mercuriusClient.query(Query_signIn, ...) sign-in logic with a call to
getAdminToken() and use its returned token for authentication; update any
variables (signInResult, authToken) to use the token returned by getAdminToken
and remove the Query_signIn dependency so the test uses the central
single-sign-in optimization.
- Around line 444-475: The test "admin user can access event from any
organization" creates resources (via createTestOrganization and createTestEvent)
but never cleans them up; wrap the setup and assertions in a try/finally (or add
an outer afterEach) and in the finally block delete the created event and
organization using your teardown helpers or API calls (use the returned event.id
and organization.id), ensuring cleanup runs even on assertion failures; keep the
mercuriusClient.query/Query_event/assertToBeNonNullish logic intact and only add
the finally cleanup that calls the appropriate delete functions for the event
and organization.
- Around line 62-64: The test suite currently calls ensureAdminAuth() in a
beforeAll (the cached admin token), which violates the repo policy requiring
per-test auth creation; change the hook to beforeEach(async () => { await
ensureAdminAuth(); }) or call ensureAdminAuth() inside each affected test so
each test gets a fresh token, remove any global/cached token usage, and ensure
tests that reference the admin token (via the existing helper used by these
tests) use the freshly-created token rather than a shared one.
- Around line 479-592: The afterEach drains testCleanupFunctions but many tests
never register cleanup; for each test that calls createTestOrganization,
server.drizzleClient.insert(eventsTable) or
mercuriusClient.mutate(Mutation_createEvent) (and the recurring event/template
flow used by the "creates a never-ending recurring event" tests), push
corresponding cleanup lambdas onto testCleanupFunctions that delete the created
event rows (use eventsTable/eq on id via server.drizzleClient.delete), remove
any recurring templates/instances, and delete the created organization (and user
if created) so resources are removed; also update the invite-only tests that
currently only push cleanup for the regular user to also push cleanup for the
organization and event created in those tests.
- Around line 381-442: The test creates a regular user via Mutation_createUser
(stored in variable user) but never deletes it; fix by recording created user.id
(e.g., push to a local createdUserIds array) and ensure you call
Mutation_deleteUser for each id after the test (or add an afterEach in the
surrounding suite) using the adminAuthToken in the headers; reference
Mutation_createUser, user.id, and Mutation_deleteUser (and adminAuthToken) so
deletion runs even on failures (use finally/afterEach) to prevent leaked users
across tests.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
test/graphql/types/Query/event.test.ts (9)
349-425:⚠️ Potential issue | 🔴 Critical
finallyblock only deletes the user; org and event are permanently leaked
createTestOrganizationandcreateTestEventat lines 352–356 create an organization, an admin membership, and an event. Thefinallyblock (lines 412–424) only issuesMutation_deleteUser. Neither the event nor the organization is ever deleted. The outersuite("Query field event")has noafterEach, so there is no secondary safety net.🛡️ Proposed fix
} finally { + // Delete event before organization (dependency order) + await server.drizzleClient.delete(eventsTable).where(eq(eventsTable.id, event.id)); + // Delete organization (add Mutation_deleteOrganization if available) await mercuriusClient.mutate(Mutation_deleteUser, { headers: { authorization: `bearer ${adminAuthToken}` }, variables: { input: { id: user.user.id } }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 349 - 425, The test leaks resources because createTestOrganization and createTestEvent (creating organization.id and event.id) are not cleaned up in the finally block which only calls Mutation_deleteUser; update the finally block to also delete the created event and organization using the adminAuthToken (call the corresponding mutations for deleting the event and the organization or use existing teardown helpers), perform deletions in safe order (delete event first, then organization, then user), and guard each deletion against null ids/errors so cleanup always attempts all removals.
40-48:⚠️ Potential issue | 🟡 MinorRemove redundant
assertToBeNonNullishcallsThe
if (!authToken || !userId)guard at lines 40–46 already throws when either value is falsy, soassertToBeNonNullish(authToken)andassertToBeNonNullish(userId)at lines 47–48 are unreachable dead assertions. Same redundancy exists at line 83 insidecreateTestOrganization.🧹 Proposed fix
if (!authToken || !userId) { throw new Error( `Failed to sign in as admin. Errors: ${JSON.stringify( signInResult.errors, )}`, ); } - assertToBeNonNullish(authToken); - assertToBeNonNullish(userId); return { authToken, userId };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 40 - 48, Remove the unreachable redundant null-check assertions: delete the two assertToBeNonNullish(authToken) and assertToBeNonNullish(userId) calls immediately after the if-block that throws when (!authToken || !userId) in the test setup, and likewise remove the similar redundant assertToBeNonNullish call inside createTestOrganization; the thrown Error guard already guarantees non-null values so only keep the initial guard/throw logic (refer to variables authToken, userId and the createTestOrganization helper to locate the assertions).
1003-1064:⚠️ Potential issue | 🔴 Critical"Event creator" test creates org+event with absolutely no cleanup
Unlike the other invite-only tests that at least push a user-deletion callback (albeit after assertions), this test registers nothing in
testCleanupFunctions. The organization, admin membership, and invite-only event created at lines 1008–1034 are leaked on every test run.🛡️ Proposed fix
test("event creator can access invite-only event", async () => { const { authToken: adminAuthToken, userId: adminUserId } = await getAdminTokenAndUserId(); const organization = await createTestOrganization(adminAuthToken, adminUserId); + testCleanupFunctions.push(async () => { + // Delete event then organization (dependency order) + // Add Mutation_deleteOrganization / direct eventsTable delete here + }); const createEventResult = await mercuriusClient.mutate(Mutation_createEvent, { ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 1003 - 1064, This test leaks created resources; after creating the organization with createTestOrganization and after successfully creating the invite-only event (createEvent via mercuriusClient.mutate producing eventId), push appropriate cleanup callbacks into testCleanupFunctions to delete the event and the organization (and any membership) so the admin membership and org/event are removed after the test; reference createTestOrganization, eventId, createEventResult, and testCleanupFunctions when adding the cleanup entries.
241-270:⚠️ Potential issue | 🔴 CriticalOrg and event created by
setupTestDataare never added totestCleanupFunctions
setupTestDataat line 243 creates an organization, an admin membership, and an event in the database. Although this suite has anafterEachthat drainstestCleanupFunctions, nothing is ever pushed to that array here, so all three records leak permanently.🛡️ Proposed fix
const { authToken, userId } = await getAdminTokenAndUserId(); - const { event } = await setupTestData(authToken, userId); + const { organization, event } = await setupTestData(authToken, userId); + testCleanupFunctions.push(async () => { + // Delete event then organization (reverse dependency order) + await server.drizzleClient.delete(eventsTable).where(eq(eventsTable.id, event.id)); + // Add Mutation_deleteOrganization here if available in documentNodes + }); const deletedUser = await createAndDeleteTestUser(authToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 241 - 270, The test leaks DB records because setupTestData creates an organization, admin membership and event but neither returns cleanup handles nor adds them to testCleanupFunctions; update the test (or setupTestData) so the created resources are registered for cleanup: either have setupTestData return the created organization, membership and event and then push cleanup functions/ids into the shared testCleanupFunctions array from the test, or modify setupTestData to itself push appropriate cleanup closures into testCleanupFunctions; reference setupTestData and testCleanupFunctions when making this change so the org/membership/event get removed in afterEach.
757-762:⚠️ Potential issue | 🔴 Critical
testCleanupFunctions.pushplaced after assertions — cleanup silently skipped on assertion failureThis pattern appears in all four invite-only tests:
testCleanupFunctions.pushat lines 757–762, 883–888, 995–1000, and 1172–1177 is called after theexpect(…)assertions. If any assertion throws, the push is never reached, Vitest'safterEachdrains an empty array, and the created user is leaked permanently.Additionally, none of these tests register cleanup for the organization or event they create, so those records always leak regardless of test outcome.
Fix: move each
testCleanupFunctions.pushcall to immediately after resource creation (before any queries or assertions), and extend it to also delete the event and organization.🛡️ Representative fix for "registered-but-not-invited" test (apply same pattern to the other three)
const regularUserId = regularUser.user.id; const regularUserToken = regularUser.authenticationToken; + // Register cleanup immediately after creation, before any assertions + testCleanupFunctions.push(async () => { + await mercuriusClient.mutate(Mutation_deleteUser, { + headers: { authorization: `bearer ${adminAuthToken}` }, + variables: { input: { id: regularUserId } }, + }); + // Also delete event and organization here (dependency order: event first) + }); // Add user to organization await mercuriusClient.mutate(Mutation_createOrganizationMembership, { ... }); // ... // Query and assert const queryResult = await mercuriusClient.query(Query_event, { ... }); expect(queryResult.data?.event).not.toBeNull(); - // Cleanup - testCleanupFunctions.push(async () => { - await mercuriusClient.mutate(Mutation_deleteUser, { ... }); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 757 - 762, Move the test cleanup registration (testCleanupFunctions.push) to immediately after creating each resource (user, event, organization) instead of after assertions so cleanup still runs if an assertion fails; locate the blocks around create calls that later call mercuriusClient.mutate and Mutation_deleteUser (with adminAuthToken/regularUserId) and register cleanup there; extend each pushed cleanup function to also delete the created event and organization (call the appropriate mutate operations for deleting event and organization using their created IDs) so users, events, and organizations are all removed in afterEach; apply this change consistently for the four invite-only tests referencing testCleanupFunctions.push at the shown sites.
131-134: 🧹 Nitpick | 🔵 Trivial
Date.now()usage violates test determinism guidelinePer coding guidelines,
Date.now()should be avoided in tests in favour of fixed UTC timestamps or mocked timers. This helper is the root source ofDate.now()usage that propagates throughout the file (also direct at lines 484–485, 536–539, 583–584, 696–699, and more).Because
createEventrejects paststartAtvalues, using a fixed timestamp requires either a sufficiently far-future constant (e.g.,"2099-01-01T00:00:00Z") orvi.useFakeTimers():♻️ Option — far-future constant (simplest, no fake timers needed)
- const startAt = new Date(Date.now() + startOffset); - const endAt = new Date(startAt.getTime() + durationInHours * 60 * 60 * 1000); + const BASE_FUTURE_TS = new Date("2099-01-01T00:00:00Z").getTime(); + const startAt = new Date(BASE_FUTURE_TS + startOffset); + const endAt = new Date(startAt.getTime() + durationInHours * 60 * 60 * 1000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 131 - 134, The test uses Date.now() to compute startAt/endAt which breaks determinism; replace that usage by using a fixed base timestamp or mocked timers: either define a constant fixed UTC base (e.g., a far-future Date like "2099-01-01T00:00:00Z") and compute startAt/endAt from that, or call vi.useFakeTimers() and set the fake clock to a known timestamp before creating startAt/endAt; update the helper that computes startAt/endAt (and any direct Date.now() calls around createEvent) to use the fixed base or fake timer so createEvent's past-date rejection is avoided and tests remain deterministic.
980-1001:⚠️ Potential issue | 🟡 MinorIncomplete error assertion — missing error extension code check
This test only asserts
queryResult.data?.eventis null, but doesn't verify the error extension code the way the structurally identical unauthorized-access test at line 349–411 does. A nulleventcould result from any error path; asserting the specificcodeconfirms the denial is for the right reason.🛡️ Proposed addition
expect(queryResult.data?.event).toBeNull(); + + expect(queryResult.errors).toEqual( + expect.arrayContaining<TalawaGraphQLFormattedError>([ + expect.objectContaining<TalawaGraphQLFormattedError>({ + extensions: expect.objectContaining({ + code: "unauthorized_action_on_arguments_associated_resources", + }), + message: expect.any(String), + path: ["event"], + }), + ]), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 980 - 1001, The test for unauthorized regular member using Query_event currently only checks that queryResult.data?.event is null; update the test to also assert the GraphQL error extension code (from queryResult.errors?.[0].extensions.code) equals the same denial code used in the other unauthorized-access test (e.g., 'FORBIDDEN') so the failure reason is verified; locate the Query_event invocation and add an assertion on queryResult.errors to mirror the existing pattern in the other test.
427-458:⚠️ Potential issue | 🔴 CriticalRemove stale developer note and add missing cleanup for org/event
Two separate issues here:
- Line 427: The comment
// Then refactor the "admin user can access event" test to:is a leftover refactoring note — remove it before merging.- Lines 433–439:
createTestOrganizationandcreateTestEventwrite an organization, admin membership, and event to the database. The outersuite("Query field event")has noafterEach, and this test has notry/finally, so all three records are never cleaned up.🛡️ Proposed fix — wrap body in try/finally
- // Then refactor the "admin user can access event" test to: test("admin user can access event from any organization", async () => { const { authToken: adminAuthToken, userId: adminUserId } = await getAdminTokenAndUserId(); const organization = await createTestOrganization(adminAuthToken, adminUserId); const event = await createTestEvent(adminAuthToken, organization.id); + try { const queryResult = await mercuriusClient.query(Query_event, { ... }); // ... assertions ... + } finally { + await server.drizzleClient.delete(eventsTable).where(eq(eventsTable.id, event.id)); + // Delete organization via Mutation_deleteOrganization if available + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 427 - 458, Remove the stale comment line "// Then refactor the \"admin user can access event\" test to:" and wrap the test body in a try/finally so created DB records are always cleaned up; after creating organization via createTestOrganization(...) and event via createTestEvent(...), use the finally block to delete the created records (e.g. call the project’s cleanup helpers or issue deletions for event.id and organization.id using the adminAuthToken/adminUserId) to ensure both the event and organization (and any admin membership the helpers created) are removed even if assertions fail.
474-639:⚠️ Potential issue | 🔴 CriticalThree tests in "Additional event tests" create org/event records without registering any cleanup
None of the following tests push to
testCleanupFunctions, despite the suite having anafterEachthat drains it:
- Lines 474–520 ("past dates"):
createTestOrganization+ a directdrizzleClient.insertintoeventsTableat lines 480–493. The raw DB insert bypasses the mutation layer and is especially important to clean up.- Lines 522–574 ("multi-day"):
createTestOrganization+Mutation_createEvent.- Lines 576–639 ("recurring"):
createTestOrganization+ recurringMutation_createEvent.All three accumulate organizations, memberships, and events on every test run, making the suite a high-risk source of database bloat in sharded CI.
🛡️ Representative fix pattern (apply to all three tests)
test("handles events with past dates correctly", async () => { const { authToken, userId } = await getAdminTokenAndUserId(); const organization = await createTestOrganization(authToken, userId); + testCleanupFunctions.push(async () => { + // Clean up past event inserted directly, then the organization + await server.drizzleClient + .delete(eventsTable) + .where(eq(eventsTable.id, pastEventId)); + // Delete organization via Mutation_deleteOrganization if available + }); // ... rest of test body ...Note:
pastEventIdmust be declared before the push so it's in scope — declare it withletbefore thetestCleanupFunctions.pushcall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 474 - 639, These three tests (the past-dates test using server.drizzleClient.insert, the multi-day test using Mutation_createEvent, and the recurring event test using Mutation_createEvent) create organizations/events without registering cleanup; declare any IDs you need (e.g., change const pastEventId to let pastEventId before insertion) and push corresponding cleanup functions onto testCleanupFunctions immediately after creating those IDs/results: for the raw DB insert (server.drizzleClient.insert) push a function that deletes the event by pastEventId (and the created organization and membership) and for both mutation-created events (results from Mutation_createEvent returning createEvent.id) push cleanup functions that delete the event instance(s) and the organization/membership using the returned id(s); ensure each push references the correct symbols (pastEventId, multiDayEvent.id, baseRecurringEventId, createEventResult.data.createEvent) so the afterEach drain removes those DB records.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 349-425: The test leaks resources because createTestOrganization
and createTestEvent (creating organization.id and event.id) are not cleaned up
in the finally block which only calls Mutation_deleteUser; update the finally
block to also delete the created event and organization using the adminAuthToken
(call the corresponding mutations for deleting the event and the organization or
use existing teardown helpers), perform deletions in safe order (delete event
first, then organization, then user), and guard each deletion against null
ids/errors so cleanup always attempts all removals.
- Around line 40-48: Remove the unreachable redundant null-check assertions:
delete the two assertToBeNonNullish(authToken) and assertToBeNonNullish(userId)
calls immediately after the if-block that throws when (!authToken || !userId) in
the test setup, and likewise remove the similar redundant assertToBeNonNullish
call inside createTestOrganization; the thrown Error guard already guarantees
non-null values so only keep the initial guard/throw logic (refer to variables
authToken, userId and the createTestOrganization helper to locate the
assertions).
- Around line 1003-1064: This test leaks created resources; after creating the
organization with createTestOrganization and after successfully creating the
invite-only event (createEvent via mercuriusClient.mutate producing eventId),
push appropriate cleanup callbacks into testCleanupFunctions to delete the event
and the organization (and any membership) so the admin membership and org/event
are removed after the test; reference createTestOrganization, eventId,
createEventResult, and testCleanupFunctions when adding the cleanup entries.
- Around line 241-270: The test leaks DB records because setupTestData creates
an organization, admin membership and event but neither returns cleanup handles
nor adds them to testCleanupFunctions; update the test (or setupTestData) so the
created resources are registered for cleanup: either have setupTestData return
the created organization, membership and event and then push cleanup
functions/ids into the shared testCleanupFunctions array from the test, or
modify setupTestData to itself push appropriate cleanup closures into
testCleanupFunctions; reference setupTestData and testCleanupFunctions when
making this change so the org/membership/event get removed in afterEach.
- Around line 757-762: Move the test cleanup registration
(testCleanupFunctions.push) to immediately after creating each resource (user,
event, organization) instead of after assertions so cleanup still runs if an
assertion fails; locate the blocks around create calls that later call
mercuriusClient.mutate and Mutation_deleteUser (with
adminAuthToken/regularUserId) and register cleanup there; extend each pushed
cleanup function to also delete the created event and organization (call the
appropriate mutate operations for deleting event and organization using their
created IDs) so users, events, and organizations are all removed in afterEach;
apply this change consistently for the four invite-only tests referencing
testCleanupFunctions.push at the shown sites.
- Around line 131-134: The test uses Date.now() to compute startAt/endAt which
breaks determinism; replace that usage by using a fixed base timestamp or mocked
timers: either define a constant fixed UTC base (e.g., a far-future Date like
"2099-01-01T00:00:00Z") and compute startAt/endAt from that, or call
vi.useFakeTimers() and set the fake clock to a known timestamp before creating
startAt/endAt; update the helper that computes startAt/endAt (and any direct
Date.now() calls around createEvent) to use the fixed base or fake timer so
createEvent's past-date rejection is avoided and tests remain deterministic.
- Around line 980-1001: The test for unauthorized regular member using
Query_event currently only checks that queryResult.data?.event is null; update
the test to also assert the GraphQL error extension code (from
queryResult.errors?.[0].extensions.code) equals the same denial code used in the
other unauthorized-access test (e.g., 'FORBIDDEN') so the failure reason is
verified; locate the Query_event invocation and add an assertion on
queryResult.errors to mirror the existing pattern in the other test.
- Around line 427-458: Remove the stale comment line "// Then refactor the
\"admin user can access event\" test to:" and wrap the test body in a
try/finally so created DB records are always cleaned up; after creating
organization via createTestOrganization(...) and event via createTestEvent(...),
use the finally block to delete the created records (e.g. call the project’s
cleanup helpers or issue deletions for event.id and organization.id using the
adminAuthToken/adminUserId) to ensure both the event and organization (and any
admin membership the helpers created) are removed even if assertions fail.
- Around line 474-639: These three tests (the past-dates test using
server.drizzleClient.insert, the multi-day test using Mutation_createEvent, and
the recurring event test using Mutation_createEvent) create organizations/events
without registering cleanup; declare any IDs you need (e.g., change const
pastEventId to let pastEventId before insertion) and push corresponding cleanup
functions onto testCleanupFunctions immediately after creating those
IDs/results: for the raw DB insert (server.drizzleClient.insert) push a function
that deletes the event by pastEventId (and the created organization and
membership) and for both mutation-created events (results from
Mutation_createEvent returning createEvent.id) push cleanup functions that
delete the event instance(s) and the organization/membership using the returned
id(s); ensure each push references the correct symbols (pastEventId,
multiDayEvent.id, baseRecurringEventId, createEventResult.data.createEvent) so
the afterEach drain removes those DB records.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
test/graphql/types/Query/event.test.ts (6)
56-108: 🧹 Nitpick | 🔵 Trivial
createTestOrganizationcreates membership but has no cleanup path.This helper creates both an organization and an admin membership. Since it's called by most tests, any cleanup strategy needs to account for both. Consider returning the membership ID as well, or relying on cascade deletes from organization deletion to clean up associated memberships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 56 - 108, The helper createTestOrganization currently creates an organization and an admin membership but does not return or expose the created membership for cleanup; update createTestOrganization to also return the membership ID (or the full membership object) so tests can delete the membership explicitly, or alternatively ensure callers delete the organization and rely on cascade deletes; locate the creation of the membership in the mutate call using Mutation_createOrganizationMembership and return membershipResult.data (or membershipResult.data.createOrganizationMembership.id) alongside the organization so teardown can clean both entities.
238-267:⚠️ Potential issue | 🟠 MajorMissing cleanup: organization and event leak from
setupTestData.
setupTestDatacreates an organization and event but no cleanup functions are pushed totestCleanupFunctions. TheafterEachat line 205 runs but has nothing to clean. This leaves orphaned records in the database after every run.Proposed fix: register cleanup in testCleanupFunctions
test("client triggering the graphql operation has no existing user associated to their authentication context.", async () => { const { authToken, userId } = await getAdminTokenAndUserId(); const { event } = await setupTestData(authToken, userId); + + // Register cleanup for the organization and event created by setupTestData + testCleanupFunctions.push(async () => { + await server.drizzleClient + .delete(eventsTable) + .where((fields, operators) => operators.eq(fields.id, event.id)); + // Organization cleanup would require a delete mutation or direct DB delete + }); + const deletedUser = await createAndDeleteTestUser(authToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 238 - 267, The test leaks organization and event records because setupTestData creates an organization and event but does not register cleanup functions with testCleanupFunctions; update setupTestData (or the test) to push appropriate async cleanup callbacks into the existing testCleanupFunctions array that delete the created organization and event (use the created event.id and organization id returned by setupTestData), so afterEach will remove those records; reference the setupTestData helper, the testCleanupFunctions array and the afterEach hook when adding these cleanup registrations.
663-1215:⚠️ Potential issue | 🟠 MajorInvite-only event visibility suite: widespread resource leaks.
All five tests in this suite create organizations and events (and some create memberships, registrations, attendees) that are never cleaned up. Only user deletions are registered in
testCleanupFunctions. The "event creator can access" test (lines 1039–1100) has zero cleanup.Accumulated leaks per suite run: ≥5 organizations, ≥5 events, ≥4 memberships, plus registrations and attendees. Per the coding guidelines, this is a high-risk pattern for sharded CI execution (>10 entities with no cleanup).
Consider:
- Importing the organization and event tables for direct DB cleanup (events already imported).
- Pushing DB deletes for events into
testCleanupFunctionsin every test.- Adding organization table cleanup (may require importing
organizationsTable).- Cleaning up in reverse dependency order: attendees → registrations → events → memberships → organizations → users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 663 - 1215, The tests in the "Invite-only event visibility" suite leak DB entities; ensure every test that calls createTestOrganization, Mutation_createEvent, Mutation_createOrganizationMembership, Mutation_registerForEvent or Mutation_inviteEventAttendee registers cleanup steps into testCleanupFunctions to delete attendees → registrations → events → memberships → organizations → users in reverse dependency order; import and use the eventsTable (already imported) and organizationsTable (add import if missing) and push direct DB delete calls (or GraphQL delete mutations) for event ids, organization ids, membership ids, registration/attendee records after creating them so each test (including the "event creator can access" case) registers its cleanup immediately after creation.
28-54:⚠️ Potential issue | 🟠 MajorPR objective not achieved: admin tokens are not cached.
The PR title and linked issue
#5258state the goal is to cache admin sign-in tokens and reduce sign-in mutations from 13+ to 1. However,getAdminTokenAndUserId()performs a freshQuery_signInon every call, and it's invoked ~14 times across the file — yielding no reduction.Note: A retrieved learning from the maintainer states that per-test auth creation is preferred over
beforeAllcaching for sharded CI isolation. If the maintainer's intent has shifted to per-test auth, the PR title, description, and issue#5258should be updated to reflect the actual change (helper extraction / refactoring), not a performance optimization that doesn't exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 28 - 54, getAdminTokenAndUserId currently runs Query_signIn via mercuriusClient.query every time it’s called (and getAdminToken simply delegates to it), so admin tokens are not cached; change this by introducing a module-level cache (e.g., cachedAdminAuthToken and cachedAdminUserId) that getAdminTokenAndUserId checks before calling Query_signIn and only performs mercuriusClient.query(Query_signIn, ...) when cache is empty, populating the cache on first successful sign-in; alternatively, if the intended behavior is per-test auth creation rather than caching, update the PR title/description and issue reference to reflect refactor-only intent instead of claiming token caching/optimization.
346-458:⚠️ Potential issue | 🟠 MajorMissing cleanup for organizations and events in standalone tests.
Both tests (lines 346–424 and 426–458) create organizations and events that are never deleted. The comments claim cleanup mutations aren't available, but the test at line 485–488 already demonstrates direct DB deletion of events via
server.drizzleClient.delete(eventsTable). The same approach can be used here.Per the coding guidelines, tests creating database entities must clean them up to prevent data accumulation in sharded CI. These two tests alone leak 2 organizations and 2 events per run.
Proposed fix for "admin user can access event" test (lines 426–458)
test("admin user can access event from any organization", async () => { const { authToken: adminAuthToken, userId: adminUserId } = await getAdminTokenAndUserId(); - // Create test organization - const organization = await createTestOrganization( - adminAuthToken, - adminUserId, - ); - - // Create test event using helper - const event = await createTestEvent(adminAuthToken, organization.id); - - // Try to access event as admin - const queryResult = await mercuriusClient.query(Query_event, { - headers: { - authorization: `bearer ${adminAuthToken}`, - }, - variables: { - input: { - id: event.id, - }, - }, - }); - - const queriedEvent = queryResult.data.event; - expect(queriedEvent).not.toBeNull(); - assertToBeNonNullish(queriedEvent); - expect(queriedEvent.id).toBe(event.id); - expect(queryResult.errors).toBeUndefined(); - // Note: Event and organization cleanup would require additional mutations - // which are not currently available in the test helpers + const organization = await createTestOrganization(adminAuthToken, adminUserId); + const event = await createTestEvent(adminAuthToken, organization.id); + + try { + const queryResult = await mercuriusClient.query(Query_event, { + headers: { + authorization: `bearer ${adminAuthToken}`, + }, + variables: { + input: { + id: event.id, + }, + }, + }); + + const queriedEvent = queryResult.data.event; + expect(queriedEvent).not.toBeNull(); + assertToBeNonNullish(queriedEvent); + expect(queriedEvent.id).toBe(event.id); + expect(queryResult.errors).toBeUndefined(); + } finally { + await server.drizzleClient + .delete(eventsTable) + .where((fields, operators) => operators.eq(fields.id, event.id)); + // TODO: add organization cleanup via direct DB delete + } });As per coding guidelines: "Tests creating database entities (orgs, users, events, etc.) MUST clean them up" and "Flag missing database cleanup as HIGH PRIORITY blocking issue."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 346 - 458, The tests create organizations and events via createTestOrganization and createTestEvent but never remove them; add cleanup that deletes the created event(s) and organization(s) using the existing direct DB deletion pattern (e.g., call server.drizzleClient.delete(eventsTable) and server.drizzleClient.delete(organizationsTable) or equivalent) in the finally blocks (or after the test) for both the "unauthorized regular user..." and "admin user can access event..." tests; ensure you reference the created entity IDs (event.id and organization.id) and perform deletions with the same client (server.drizzleClient) used elsewhere in the suite so the database rows are removed after each test.
480-503: 🧹 Nitpick | 🔵 TrivialLeverage cascade deletes to simplify organization cleanup.
The events table schema specifies
onDelete: "cascade"on theorganizationIdforeign key (src/drizzle/tables/events.ts:68). This means deleting an organization automatically deletes all its events. Since each test creates an organization, you can simplify cleanup across the file by deleting organizations directly—which will cascade-delete all associated events. This pattern is already used in 20+ other test files (e.g., test/graphql/types/Comment/post.test.ts, test/graphql/types/Mutation/updateVenue.test.ts). ImportorganizationsTableand replace event-by-event cleanup with organization cleanup to make the cleanup story significantly more maintainable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 480 - 503, Replace the per-event direct delete with a cascade-backed organization delete: import organizationsTable, and in the cleanup pushed to testCleanupFunctions call server.drizzleClient.delete(organizationsTable).where(...) using the same organization.id (instead of deleting eventsTable by pastEventId). Keep the existing registration pattern (testCleanupFunctions.push(...)) and remove the explicit eventsTable.delete for pastEventId so the DB cascade on organizationId handles event removals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 636-640: The no-op cleanup pushed into testCleanupFunctions leaves
recurring events and their instances orphaned; replace the empty async function
with a real cleanup that calls the GraphQL mutations to remove the recurring
event, all generated instances, and the test organization (e.g. invoke
deleteEvent (or deleteRecurringEvent) with the recurring event's id and
deleteOrganization with the organization id), or call existing helper cleanup
functions used elsewhere in tests, ensuring the function awaits removal of
instances and the base event before returning so DB rows are not leaked.
- Around line 560-564: The pushed async cleanup in testCleanupFunctions is a
no-op (only comments) and must be replaced or removed; either implement real
deletion for the created event/organization (use the same approach as the other
cleanup in this file — e.g., call the project test DB helper or
prisma.event.delete/prisma.organization.delete with the created
eventId/organizationId inside the async () => { ... } you push) or remove the
empty push so tests don't pretend cleanup is handled; locate the no-op push to
testCleanupFunctions and replace it with the concrete delete calls referencing
the created resource IDs.
- Around line 1-3: Add vi to the vitest imports and call vi.restoreAllMocks()
inside each existing afterEach hook to prevent test pollution; specifically
update the import line to include vi (e.g., import { afterEach, expect, suite,
test, vi } from "vitest") and insert vi.restoreAllMocks() in the afterEach
callbacks used in this file (the afterEach function declarations).
---
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 56-108: The helper createTestOrganization currently creates an
organization and an admin membership but does not return or expose the created
membership for cleanup; update createTestOrganization to also return the
membership ID (or the full membership object) so tests can delete the membership
explicitly, or alternatively ensure callers delete the organization and rely on
cascade deletes; locate the creation of the membership in the mutate call using
Mutation_createOrganizationMembership and return membershipResult.data (or
membershipResult.data.createOrganizationMembership.id) alongside the
organization so teardown can clean both entities.
- Around line 238-267: The test leaks organization and event records because
setupTestData creates an organization and event but does not register cleanup
functions with testCleanupFunctions; update setupTestData (or the test) to push
appropriate async cleanup callbacks into the existing testCleanupFunctions array
that delete the created organization and event (use the created event.id and
organization id returned by setupTestData), so afterEach will remove those
records; reference the setupTestData helper, the testCleanupFunctions array and
the afterEach hook when adding these cleanup registrations.
- Around line 663-1215: The tests in the "Invite-only event visibility" suite
leak DB entities; ensure every test that calls createTestOrganization,
Mutation_createEvent, Mutation_createOrganizationMembership,
Mutation_registerForEvent or Mutation_inviteEventAttendee registers cleanup
steps into testCleanupFunctions to delete attendees → registrations → events →
memberships → organizations → users in reverse dependency order; import and use
the eventsTable (already imported) and organizationsTable (add import if
missing) and push direct DB delete calls (or GraphQL delete mutations) for event
ids, organization ids, membership ids, registration/attendee records after
creating them so each test (including the "event creator can access" case)
registers its cleanup immediately after creation.
- Around line 28-54: getAdminTokenAndUserId currently runs Query_signIn via
mercuriusClient.query every time it’s called (and getAdminToken simply delegates
to it), so admin tokens are not cached; change this by introducing a
module-level cache (e.g., cachedAdminAuthToken and cachedAdminUserId) that
getAdminTokenAndUserId checks before calling Query_signIn and only performs
mercuriusClient.query(Query_signIn, ...) when cache is empty, populating the
cache on first successful sign-in; alternatively, if the intended behavior is
per-test auth creation rather than caching, update the PR title/description and
issue reference to reflect refactor-only intent instead of claiming token
caching/optimization.
- Around line 346-458: The tests create organizations and events via
createTestOrganization and createTestEvent but never remove them; add cleanup
that deletes the created event(s) and organization(s) using the existing direct
DB deletion pattern (e.g., call server.drizzleClient.delete(eventsTable) and
server.drizzleClient.delete(organizationsTable) or equivalent) in the finally
blocks (or after the test) for both the "unauthorized regular user..." and
"admin user can access event..." tests; ensure you reference the created entity
IDs (event.id and organization.id) and perform deletions with the same client
(server.drizzleClient) used elsewhere in the suite so the database rows are
removed after each test.
- Around line 480-503: Replace the per-event direct delete with a cascade-backed
organization delete: import organizationsTable, and in the cleanup pushed to
testCleanupFunctions call
server.drizzleClient.delete(organizationsTable).where(...) using the same
organization.id (instead of deleting eventsTable by pastEventId). Keep the
existing registration pattern (testCleanupFunctions.push(...)) and remove the
explicit eventsTable.delete for pastEventId so the DB cascade on organizationId
handles event removals.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/graphql/types/Query/event.test.ts (3)
282-355:⚠️ Potential issue | 🟡 MinorMissing
afterEachwithvi.restoreAllMocks()in the "database query failure" suite.The two other suites in this file both include an
afterEachthat callsvi.restoreAllMocks()(lines 207–217 and 482–492). The suite at lines 282–355 has noafterEachat all. While no mocks or DB entities are created here today, the pattern should be consistent across suites to prevent future test pollution if a spy is ever added.🛠️ Proposed addition
suite( `results in a graphql error with database query failure...`, () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + test("fails with ULID of wrong length", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 282 - 355, Add an afterEach hook calling vi.restoreAllMocks() inside the suite whose title begins with `results in a graphql error with database query failure in the "errors" field and "null" as the value of "data.event" field if` so it mirrors the other suites; place the afterEach directly inside that suite block (next to existing tests for wrong-length ULID, invalid characters, and "invalid-id") to ensure spies/mocks are restored after each test (use vi.restoreAllMocks()).
30-56:⚠️ Potential issue | 🟠 MajorPer-call sign-in is correct per guidelines, but the PR's stated caching objective is unimplemented.
getAdminTokenAndUserId()andgetAdminToken()perform a freshQuery_signInmutation on every call — there is no caching, no memoization, and no call toensureAdminAuth(). Counting invocations across the file yields 14 separate sign-in round-trips per test run (lines 242, 286, 309, 332, 358, 440, 494, 551, 613, 688, 815, 945, 1080, 1151), which is more than the 13+ the linked issue#5258called out.This is worth flagging for two reasons:
- PR description is inaccurate. The PR title says "cache admin sign-in tokens" and the description says these helpers "call the existing
ensureAdminAuth()caching mechanism". Neither is true in the code.- The linked issue's acceptance criterion — reduce sign-in mutations to a single call — is not met.
Ironically, the per-call pattern is compliant with the coding guidelines ("Auth tokens should be created per-test to avoid cascading failures"), so the code itself is fine. But either the PR description should be corrected to reflect the actual change (per-test sign-in replacing a shared
beforeAlltoken), or the caching originally described should be implemented.#!/bin/bash # Verify whether ensureAdminAuth exists in the codebase and whether it was removed from this file. rg -n "ensureAdminAuth" --type=ts -C 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 30 - 56, The helpers getAdminTokenAndUserId and getAdminToken execute Query_signIn on every call (14 sign-ins per run) despite the PR claiming tokens are cached; either update the PR text to remove the caching claim or implement caching by routing calls through the existing ensureAdminAuth (or add a simple memoization wrapper) so Query_signIn runs only once per test run — locate getAdminTokenAndUserId/getAdminToken in this test file and replace direct mercuriusClient.query(Query_signIn, ...) usage with a call to ensureAdminAuth() (or a module-level cached promise) and return the authToken/userId from that single shared result.
357-476:⚠️ Potential issue | 🟠 MajorOrganization leaks if
createTestEventthrows —try/finallystarts too late.In both standalone tests, the
try/finallycleanup block (newly added at lines 387 and 452) is placed after the organization (and in the first test, after the user) has already been created. IfcreateTestEvent(...)at line 364 (or line 450) throws — which the helper does on mutation errors — the organization is never cleaned up because thefinallyblock is never entered.Affected:
- Lines 357–437 ("unauthorized regular user"): org created at line 360, event at line 364;
trystarts at line 387.- Lines 439–476 ("admin user can access event"): org created at line 444, event at line 450;
trystarts at line 452.🛡️ Proposed fix — wrap from the point of the first potentially-leaking resource
- const organization = await createTestOrganization(adminAuthToken, adminUserId); - const event = await createTestEvent(adminAuthToken, organization.id); - - // Create a regular user... - const userResult = await mercuriusClient.mutate(Mutation_createUser, { ... }); - const user = userResult.data?.createUser; - assertToBeNonNullish(user); - assertToBeNonNullish(user.authenticationToken); - assertToBeNonNullish(user.user); - - try { - // ... query + assertions ... - } finally { - // delete user, delete org - } + const organization = await createTestOrganization(adminAuthToken, adminUserId); + try { + const event = await createTestEvent(adminAuthToken, organization.id); + // Create a regular user... + const userResult = await mercuriusClient.mutate(Mutation_createUser, { ... }); + const user = userResult.data?.createUser; + assertToBeNonNullish(user); + assertToBeNonNullish(user.authenticationToken); + assertToBeNonNullish(user.user); + try { + // ... query + assertions ... + } finally { + await mercuriusClient.mutate(Mutation_deleteUser, { ... }); + } + } finally { + await server.drizzleClient + .delete(organizationsTable) + .where(eq(organizationsTable.id, organization.id)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 357 - 476, The tests createTestOrganization(...) is called before the try/finally, so if createTestEvent(...) throws the organization (and in the first test the created user) will leak; move the try block to start immediately after creating the organization (i.e., wrap from just after createTestOrganization through the rest of the test) so the finally always runs, hoist variables (organization, event, user) to outer scope so they are visible in finally, and in the finally block conditionally call the same cleanup steps you already have (mutate Mutation_deleteUser for user.user.id if user exists, and server.drizzleClient.delete(organizationsTable).where(eq(organizationsTable.id, organization.id)) if organization exists) to ensure proper teardown even on createTestEvent failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 578-584: The cleanup callbacks for created resources (e.g.,
organization from createTestOrganization and users from regularUser.user) must
be pushed into testCleanupFunctions immediately after each creation (before any
subsequent operations like Mutation_createEvent, assertToBeNonNullish, addToOrg,
etc.) — modify the tests (e.g., the "handles multi-day events" and the other
listed tests) so that right after createTestOrganization returns you call
testCleanupFunctions.push(async () => { /* delete organization via
server.drizzleClient.delete(organizationsTable).where(eq(organizationsTable.id,
organization.id)) */ }); and likewise push a separate cleanup immediately after
creating any user (delete via Mutation_deleteUser or equivalent) so the
afterEach reversal deletes user first then org and prevents orphaned records.
---
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 282-355: Add an afterEach hook calling vi.restoreAllMocks() inside
the suite whose title begins with `results in a graphql error with database
query failure in the "errors" field and "null" as the value of "data.event"
field if` so it mirrors the other suites; place the afterEach directly inside
that suite block (next to existing tests for wrong-length ULID, invalid
characters, and "invalid-id") to ensure spies/mocks are restored after each test
(use vi.restoreAllMocks()).
- Around line 30-56: The helpers getAdminTokenAndUserId and getAdminToken
execute Query_signIn on every call (14 sign-ins per run) despite the PR claiming
tokens are cached; either update the PR text to remove the caching claim or
implement caching by routing calls through the existing ensureAdminAuth (or add
a simple memoization wrapper) so Query_signIn runs only once per test run —
locate getAdminTokenAndUserId/getAdminToken in this test file and replace direct
mercuriusClient.query(Query_signIn, ...) usage with a call to ensureAdminAuth()
(or a module-level cached promise) and return the authToken/userId from that
single shared result.
- Around line 357-476: The tests createTestOrganization(...) is called before
the try/finally, so if createTestEvent(...) throws the organization (and in the
first test the created user) will leak; move the try block to start immediately
after creating the organization (i.e., wrap from just after
createTestOrganization through the rest of the test) so the finally always runs,
hoist variables (organization, event, user) to outer scope so they are visible
in finally, and in the finally block conditionally call the same cleanup steps
you already have (mutate Mutation_deleteUser for user.user.id if user exists,
and
server.drizzleClient.delete(organizationsTable).where(eq(organizationsTable.id,
organization.id)) if organization exists) to ensure proper teardown even on
createTestEvent failures.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/graphql/types/Query/event.test.ts (4)
112-158:⚠️ Potential issue | 🟠 Major
Date.now()violates the test determinism guidelines.Line 130 (
new Date(Date.now() + startOffset)) and every inline use ofnew Date(Date.now() + …)across the test suite violate the coding guideline under Timezone Safety & Test Determinism: "AvoidDate.now()in tests (use fixed timestamps or mock timers)".Use
vi.useFakeTimers({ now: new Date("2030-01-01T00:00:00Z") })(paired withvi.useRealTimers()inafterEach) as the base, and derive all timestamps from the fixed anchor — or pass pre-computed ISO strings as inputs.📅 Example fix for `createTestEvent` helper
- const startAt = new Date(Date.now() + startOffset); - const endAt = new Date( - startAt.getTime() + durationInHours * 60 * 60 * 1000, - ); + // Use a fixed far-future UTC anchor instead of Date.now() + const BASE_FUTURE_TS = new Date("2030-06-01T00:00:00Z").getTime(); + const startAt = new Date(BASE_FUTURE_TS + startOffset); + const endAt = new Date( + startAt.getTime() + durationInHours * 60 * 60 * 1000, + );As per coding guidelines, "All test dates must use fixed UTC timestamps (e.g.,
'2025-01-01T10:00:00Z')".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 112 - 158, The createTestEvent helper uses Date.now() (new Date(Date.now() + startOffset)) which breaks test determinism; change it to derive timestamps from a fixed anchor or from the test's fake timer instead: either accept a baseNow ISO/string/Date parameter on createTestEvent and compute startAt/endAt from that fixed base, or ensure tests call vi.useFakeTimers({ now: new Date("2030-01-01T00:00:00Z") }) and then compute startAt as new Date(baseNow.getTime() + startOffset) inside createTestEvent (replace Date.now() usages). Update callers to pass the fixed anchor or enable the fake timers in setup and restore with vi.useRealTimers() in afterEach.
167-200:⚠️ Potential issue | 🟡 Minor
createAndDeleteTestUserleaks the user if deletion fails.If
Mutation_deleteUser(line 188) throws (e.g., auth error), the function propagates the error but leaves the user record in the database. The caller'safterEachcleanup only covers the organization, not this user.Wrap the deletion in a try/catch that re-throws after noting the leak, or register the user's cleanup into the suite-level
testCleanupFunctionsbefore attempting deletion.Additionally, the name
createAndDeleteTestUserobscures the intent (generating a stale auth token for a deleted user). A clearer name likecreateUserAndReturnStaleTokenimproves readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 167 - 200, The helper createAndDeleteTestUser currently creates a user then attempts to delete it with Mutation_deleteUser and can leak the user if deletion throws; fix by registering a cleanup for the created user into the suite-level testCleanupFunctions immediately after creation (using the created user's id) and then attempt deletion inside a try/catch that re-throws after noting the leak, or at minimum wrap the Mutation_deleteUser call in try/catch and invoke the registered cleanup on error to guarantee eventual deletion; also rename the function to createUserAndReturnStaleToken to clarify intent (and update callers) so it documents that it returns a token for a deleted user.
357-437:⚠️ Potential issue | 🟠 MajorCleanup gap: resource creation is outside the
tryblock.The
tryblock at line 387 covers only the query and assertions. Organization (line 360), event (line 364), user (lines 367–380), and the threeassertToBeNonNullishguards (lines 382–385) all execute before thetry. If any of those throw, thefinallyclause never runs and all already-created resources are orphaned.Same pattern applies to the "admin user can access event" test (lines 444–450 are before the
tryat line 452): ifcreateTestEventthrows on line 450, the organization leaks.🛡️ Fix: move resource creation inside try or use immediate registration
test("unauthorized regular user cannot access event...", async () => { const { authToken: adminAuthToken, userId: adminUserId } = await getAdminTokenAndUserId(); - const organization = await createTestOrganization(adminAuthToken, adminUserId); - const event = await createTestEvent(adminAuthToken, organization.id); - - const userResult = await mercuriusClient.mutate(Mutation_createUser, { ... }); - const user = userResult.data?.createUser; - assertToBeNonNullish(user); - assertToBeNonNullish(user.authenticationToken); - assertToBeNonNullish(user.user); - - try { + let organization: Awaited<ReturnType<typeof createTestOrganization>> | undefined; + let user: { user: { id: string }; authenticationToken: string } | undefined; + try { + organization = await createTestOrganization(adminAuthToken, adminUserId); + const event = await createTestEvent(adminAuthToken, organization.id); + const userResult = await mercuriusClient.mutate(Mutation_createUser, { ... }); + user = userResult.data?.createUser; + assertToBeNonNullish(user); + assertToBeNonNullish(user.authenticationToken); + assertToBeNonNullish(user.user); // ... query and assertions ... } finally { + if (user?.user?.id) { await mercuriusClient.mutate(Mutation_deleteUser, { ... }); + } + if (organization?.id) { await server.drizzleClient.delete(organizationsTable) .where(eq(organizationsTable.id, organization.id)); + } } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 357 - 437, The cleanup leak happens because createTestOrganization, createTestEvent, user creation (Mutation_createUser) and the assertToBeNonNullish checks run before the try/finally so finally may not execute; fix by moving resource creation and the assertToBeNonNullish calls into the try block (for this test "unauthorized regular user cannot access event..." and the similar "admin user can access event" test) or immediately register the created resources for cleanup before any throws (e.g., push created ids into a local cleanup list) and ensure the finally block deletes via Mutation_deleteUser and server.drizzleClient.delete(organizationsTable) using those registered ids so cleanup always runs even if creation throws.
30-56:⚠️ Potential issue | 🟠 MajorPR objective not met —
getAdminTokenAndUserId()andgetAdminToken()perform fresh sign-ins on every call with no caching.Both helpers issue a full
Query_signInmutation on each invocation. There is no module-level cache,beforeAllhook, or delegation to the establishedensureAdminAuth()caching pattern used throughout peer test files (createEventVolunteer.test.ts, sendEventInvitations.test.ts, deleteEventVolunteer.test.ts, updateEventVolunteer.test.ts, etc.). With 14+ call-sites across the suite, this reproduces the same repeated sign-in problem that issue#5258aimed to fix.To meet the PR's stated goal, replace the custom helpers with the established
ensureAdminAuth()pattern already used in peer test files:async function ensureAdminAuth(): Promise<{ token: string; userId: string }> { const signInResult = await mercuriusClient.query(Query_signIn, { variables: { input: { emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, }, }, }); const authToken = signInResult.data?.signIn?.authenticationToken; const userId = signInResult.data?.signIn?.user?.id; if (!authToken || !userId) { throw new Error(`Failed to sign in as admin. Errors: ${JSON.stringify(signInResult.errors)}`); } return { authToken, userId }; }Then cache the result in
beforeAllto ensure a single sign-in per suite run:let adminToken: string; let adminUserId: string; beforeAll(async () => { const { token, userId } = await ensureAdminAuth(); adminToken = token; adminUserId = userId; });Replace all calls to
getAdminToken()andgetAdminTokenAndUserId()with references toadminTokenandadminUserId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 30 - 56, The two helpers getAdminTokenAndUserId and getAdminToken currently perform fresh Query_signIn requests on every call; replace them with the established ensureAdminAuth pattern that performs a single sign-in via mercuriusClient.query(Query_signIn) and returns { token, userId }, then cache the result in a beforeAll by assigning to module-level adminToken and adminUserId variables; finally remove/stop using getAdminToken*/and update all call-sites to reference adminToken and adminUserId so the suite only signs in once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 112-158: The createTestEvent helper uses Date.now() (new
Date(Date.now() + startOffset)) which breaks test determinism; change it to
derive timestamps from a fixed anchor or from the test's fake timer instead:
either accept a baseNow ISO/string/Date parameter on createTestEvent and compute
startAt/endAt from that fixed base, or ensure tests call vi.useFakeTimers({ now:
new Date("2030-01-01T00:00:00Z") }) and then compute startAt as new
Date(baseNow.getTime() + startOffset) inside createTestEvent (replace Date.now()
usages). Update callers to pass the fixed anchor or enable the fake timers in
setup and restore with vi.useRealTimers() in afterEach.
- Around line 167-200: The helper createAndDeleteTestUser currently creates a
user then attempts to delete it with Mutation_deleteUser and can leak the user
if deletion throws; fix by registering a cleanup for the created user into the
suite-level testCleanupFunctions immediately after creation (using the created
user's id) and then attempt deletion inside a try/catch that re-throws after
noting the leak, or at minimum wrap the Mutation_deleteUser call in try/catch
and invoke the registered cleanup on error to guarantee eventual deletion; also
rename the function to createUserAndReturnStaleToken to clarify intent (and
update callers) so it documents that it returns a token for a deleted user.
- Around line 357-437: The cleanup leak happens because createTestOrganization,
createTestEvent, user creation (Mutation_createUser) and the
assertToBeNonNullish checks run before the try/finally so finally may not
execute; fix by moving resource creation and the assertToBeNonNullish calls into
the try block (for this test "unauthorized regular user cannot access event..."
and the similar "admin user can access event" test) or immediately register the
created resources for cleanup before any throws (e.g., push created ids into a
local cleanup list) and ensure the finally block deletes via Mutation_deleteUser
and server.drizzleClient.delete(organizationsTable) using those registered ids
so cleanup always runs even if creation throws.
- Around line 30-56: The two helpers getAdminTokenAndUserId and getAdminToken
currently perform fresh Query_signIn requests on every call; replace them with
the established ensureAdminAuth pattern that performs a single sign-in via
mercuriusClient.query(Query_signIn) and returns { token, userId }, then cache
the result in a beforeAll by assigning to module-level adminToken and
adminUserId variables; finally remove/stop using getAdminToken*/and update all
call-sites to reference adminToken and adminUserId so the suite only signs in
once.
---
Duplicate comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 241-251: Tests register cleanup callbacks too late, causing
orphaned orgs/users if intermediate operations fail; fix by registering cleanup
callbacks immediately after resources are created. Specifically, after
createTestOrganization (or the org returned from
setupTestData/createTestOrganization) push a cleanup entry into
testCleanupFunctions right away (before performing further mutations like event
creation), and likewise push a cleanup callback immediately after creating any
user (e.g., after getAdminTokenAndUserId or user-creation helpers) so each
resource has its own callback; ensure you push user cleanup first and org
cleanup second so the final reverse() in afterEach deletes users before their
orgs.
|
Please fix the conflicting file |
- Fixed indentation issues that caused nested test functions - Corrected cleanup function registration logic - Fixed database insertion vs deletion logic - Removed orphaned code blocks that were causing syntax errors - Tests now have proper suite/test structure
|
Please fix the failing tests and checks |
|
Are you still working on this? |
Yes, I am working on this and shortly I will push the latest commit. Actually busy somewhere else, so I am going to do soon, for sure. Sorry for the delay. |
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. |
github-actions
Bot
added
no-pr-activity
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Evaluate whether the goals of the issue linked in the first PR comment are met. If there are test cases, ensure that the updated or created tests won't be flaky, especially in a sharded testing environment where test cleanup is important and response times are highly variable. Post your conclusion as an easily readable comment that also includes your findings outside the diffs. |
🧩 Analysis chainLet me gather the full file content and PR details to do a thorough review. 🏁 Script executed: Length of output: 40918
Full Review:
|
| # | Severity | Issue | Location |
|---|---|---|---|
| 1 | 🔴 High | Direct adminToken access bypasses helper — inconsistent |
Multiple tests across 3 suites |
| 2 | 🟡 Medium | Double-deletion causes noisy cleanup errors | createUserAndReturnStaleToken |
| 3 | 🟡 Medium | Recurring event test fragile — depends on undocumented server window | "creates a never-ending recurring event" |
| 4 | 🟡 Medium | Mixed cascade + explicit deletion causes noisy afterEach errors | "Access control" and "Additional event tests" suites |
| 5 | 🟢 Low | Suite-scoped cleanup array is safe sequentially but worth noting | All suites with testCleanupFunctions |
| 6 | 🟢 Low | Redundant admin membership creation | createTestOrganization |
Overall Conclusion
The PR meets the primary goal of issue #5258 — admin sign-in is now cached and performed exactly once per suite run via ensureAdminAuth(). The cleanup patterns are functional and generally prevent resource leaks. However, the codebase quality is weakened by the inconsistent use of the helper vs. direct variable access (Issue #1), and there are two real risks of noisy but non-fatal cleanup errors (Issues #2 and #4). Issue #3 is a genuine flakiness risk in CI environments where the server's materialization window is not guaranteed. These should be addressed before merging.
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/graphql/types/Query/event.test.ts (2)
190-261: 🧹 Nitpick | 🔵 TrivialCleanup registration addresses past review concerns, but double-deletion may produce noisy logs.
The helper now correctly registers cleanup immediately after user creation (lines 213-235), ensuring orphaned records are handled even if the subsequent deletion fails. However, when the immediate deletion (lines 244-254) succeeds, the cleanup function will attempt to delete the same user again, resulting in a console error log.
This is functional but creates test output noise. Consider tracking deletion success to conditionally skip cleanup:
♻️ Optional: Track deletion success to reduce log noise
async function createUserAndReturnStaleToken( authToken: string, cleanupFunctions: Array<() => Promise<void>>, ) { + let deletionSucceeded = false; const userResult = await mercuriusClient.mutate(Mutation_createUser, { // ... }); const user = userResult.data?.createUser; // Register cleanup immediately after creation in case deletion fails if (user?.user?.id) { const userId = user.user.id; cleanupFunctions.push(async () => { + if (deletionSucceeded) return; // Skip if already deleted try { await mercuriusClient.mutate(Mutation_deleteUser, { // ... }); } catch (error) { console.error( "Failed to cleanup user in createUserAndReturnStaleToken:", error, ); } }); } // ... // Attempt deletion - if it fails, cleanup will handle it try { await mercuriusClient.mutate(Mutation_deleteUser, { // ... }); + deletionSucceeded = true; } catch (error) { // Deletion failed, but cleanup is registered console.error("Failed to delete user immediately:", error); } return user; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 190 - 261, The helper createUserAndReturnStaleToken registers a cleanup function immediately but still calls Mutation_deleteUser afterwards, causing double-deletion noise; update the function to track whether the immediate deletion succeeded (e.g., set a local deleted flag after the mercuriusClient.mutate call to Mutation_deleteUser) and if deletion succeeds remove or skip pushing the cleanup function into cleanupFunctions (or conditionally no-op the registered cleanup), referencing createUserAndReturnStaleToken, cleanupFunctions, Mutation_deleteUser and mercuriusClient.mutate to locate where to change the logic.
615-764: 🧹 Nitpick | 🔵 TrivialCleanup is functional but uses mixed approaches.
The access control suite properly implements cleanup with
vi.restoreAllMocks()and immediate cleanup registration. However, cleanup approaches are inconsistent within this suite:
- Lines 640-642, 649-651: Use drizzle deletes
- Lines 730-732: Use drizzle delete for organization
- Lines 740-743: Use GraphQL mutation (
Mutation_deleteStandaloneEvent) for eventBoth approaches work correctly (cascade relationships handle dependencies), but the inconsistency could be confusing for future maintainers. Consider standardizing on one approach per entity type across the test file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 615 - 764, The test suite uses mixed cleanup methods—direct DB deletes via server.drizzleClient (organizationsTable, eventsTable) and GraphQL mutations (Mutation_deleteStandaloneEvent via mercuriusClient)—which is confusing; pick and apply a single consistent approach per entity (e.g., always use server.drizzleClient deletes for organizations and events or always use the corresponding GraphQL mutations via mercuriusClient), then update the cleanup registrations in the Access control tests (references: testCleanupFunctions, createTestOrganization, createTestEvent, Mutation_deleteStandaloneEvent, Mutation_deleteUser, organizationsTable, eventsTable) so all cleanup for an entity type uses the chosen method uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 190-261: The helper createUserAndReturnStaleToken registers a
cleanup function immediately but still calls Mutation_deleteUser afterwards,
causing double-deletion noise; update the function to track whether the
immediate deletion succeeded (e.g., set a local deleted flag after the
mercuriusClient.mutate call to Mutation_deleteUser) and if deletion succeeds
remove or skip pushing the cleanup function into cleanupFunctions (or
conditionally no-op the registered cleanup), referencing
createUserAndReturnStaleToken, cleanupFunctions, Mutation_deleteUser and
mercuriusClient.mutate to locate where to change the logic.
- Around line 615-764: The test suite uses mixed cleanup methods—direct DB
deletes via server.drizzleClient (organizationsTable, eventsTable) and GraphQL
mutations (Mutation_deleteStandaloneEvent via mercuriusClient)—which is
confusing; pick and apply a single consistent approach per entity (e.g., always
use server.drizzleClient deletes for organizations and events or always use the
corresponding GraphQL mutations via mercuriusClient), then update the cleanup
registrations in the Access control tests (references: testCleanupFunctions,
createTestOrganization, createTestEvent, Mutation_deleteStandaloneEvent,
Mutation_deleteUser, organizationsTable, eventsTable) so all cleanup for an
entity type uses the chosen method uniformly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ee7680a-e194-4f1c-97fd-45be057825d7
📒 Files selected for processing (3)
docker/compose.testing.yamltest/graphql/types/Query/event.test.tstest/server.ts
|
@h30s Can you address the comments here: #5261 (comment) |
Cioppolo14
left a comment
Cioppolo14
left a comment
There was a problem hiding this comment.
Please address the most recent coderabbit comments.
Done... |
|
@coderabbitai full review and reevaluate all your previous comments that may have been dismissed, ignored or resolved. Evaluate whether the goals of the issue linked in the first PR comment are met. If there are test cases, ensure that the updated or created tests won't be flaky, especially in a sharded testing environment where test cleanup is important and response times are highly variable. Post your conclusion as an easily readable comment that also includes your findings outside the diffs. |
🧩 Analysis chain🏁 Script executed: Length of output: 38323
Full Re-Review:
|
| Test | Org Cleanup Method |
|---|---|
"unauthorized regular user..." |
server.drizzleClient.delete(organizationsTable) (direct Drizzle) |
"admin user can access event..." |
server.drizzleClient.delete(organizationsTable) (direct Drizzle) |
"handles events with past dates..." |
Mutation_deleteOrganization (GraphQL mutation) |
"handles multi-day events..." |
Mutation_deleteOrganization (GraphQL mutation) |
| All invite-only tests | Mutation_deleteOrganization (GraphQL mutation) |
In a sharded environment with highly variable response times, direct Drizzle deletes (no HTTP round-trip, no server middleware) are more reliable for cleanup than GraphQL mutations. If the API server is slow or has a transient failure during afterEach, the GraphQL-mutation-based cleanups can fail and leave orphaned records, while the Drizzle-direct cleanups will not. Standardizing on direct Drizzle deletes for org cleanup (as done in the Access control suite) would be more resilient:
// Recommended uniform cleanup pattern for all suites:
testCleanupFunctions.push(async () => {
await server.drizzleClient
.delete(organizationsTable)
.where(eq(organizationsTable.id, organization.id));
});🟢 Issue 5 (Low): Suite-Scoped Cleanup Array — Acceptable, no change needed
🟡 Issue 6 (Low): Redundant Admin Membership in createTestOrganization — Still Present
createTestOrganization still explicitly adds the system admin user as an "administrator" member after creating the org. This adds an extra mutation to every test setup. It is not harmful, but it is unnecessary if the system admin's role already grants access regardless of membership.
Summary Table
| # | Severity | Issue | Status |
|---|---|---|---|
| 1 | Direct adminToken access bypasses helper |
✅ Fixed | |
| 2 | Double-deletion in createUserAndReturnStaleToken |
✅ Fixed | |
| 3 | 🟡 Medium | Recurring event relies on undocumented hotWindowMonthsAhead constant + month-overflow quirk |
|
| 4 | 🟡 Medium | Inconsistent org cleanup: Drizzle direct vs. GraphQL mutation across suites | |
| 5 | 🟢 Low | Suite-scoped cleanup array | ✅ Acceptable |
| 6 | 🟢 Low | Redundant admin membership in createTestOrganization |
Overall Conclusion
The PR is significantly improved from its previous state. The primary goals of issue #5258 are fully met, and the two major issues (inconsistent token access, double-deletion) have been cleanly resolved. The two remaining medium items — the hotWindowMonthsAhead assumption in the recurring event test and the inconsistent org cleanup strategy — are real but low-risk concerns. The org cleanup inconsistency (Issue 4) is the one most relevant to a sharded testing environment and is worth addressing before merging by standardizing all org cleanup to use direct Drizzle deletes.
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/graphql/types/Query/event.test.ts (1)
909-916:⚠️ Potential issue | 🔴 CriticalRemove runtime clock usage from this test path.
new Date()at Line 912 makes this scenario time-dependent and non-deterministic. This violates the test determinism requirement and can still flake in sharded CI over time boundaries.As per coding guidelines: "Avoid
new Date()without arguments" and "All test dates must use fixed UTC timestamps."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/graphql/types/Query/event.test.ts` around lines 909 - 916, The test uses new Date() in event.test.ts (variables baseDate and startAt) making the scenario time-dependent; replace the runtime clock usage by constructing a deterministic fixed UTC base timestamp (e.g., a fixed ISO string) and derive startAt from that fixed base so the test no longer depends on the current time—update the baseDate creation and any setUTCMonth/setUTCHours adjustments in this test path (same function/scope where baseDate and startAt are defined) to use the fixed UTC timestamp approach so the test is fully deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 218-248: The current try/catch around
mercuriusClient.mutate(Mutation_deleteUser, ...) only handles thrown exceptions
but ignores GraphQL errors returned in the mutation response; update the call to
capture the mutation result (e.g., const result = await
mercuriusClient.mutate(...)) and check result.errors (or result.data validity)
after the call and, if errors exist or deletion was not confirmed, register the
cleanupFunctions push fallback just like in the catch branch; keep the catch to
handle thrown exceptions and ensure references to Mutation_deleteUser,
mercuriusClient.mutate, cleanupFunctions, authToken and userId are used to
locate and modify the code.
---
Outside diff comments:
In `@test/graphql/types/Query/event.test.ts`:
- Around line 909-916: The test uses new Date() in event.test.ts (variables
baseDate and startAt) making the scenario time-dependent; replace the runtime
clock usage by constructing a deterministic fixed UTC base timestamp (e.g., a
fixed ISO string) and derive startAt from that fixed base so the test no longer
depends on the current time—update the baseDate creation and any
setUTCMonth/setUTCHours adjustments in this test path (same function/scope where
baseDate and startAt are defined) to use the fixed UTC timestamp approach so the
test is fully deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c519a74-1533-4e98-9498-dc9e03aa5586
📒 Files selected for processing (3)
docker/compose.testing.yamltest/graphql/types/Query/event.test.tstest/server.ts
| // Delete the user immediately; register cleanup only if deletion fails | ||
| try { | ||
| await mercuriusClient.mutate(Mutation_deleteUser, { | ||
| headers: { | ||
| authorization: `bearer ${authToken}`, | ||
| }, | ||
| }, | ||
| }); | ||
| variables: { | ||
| input: { | ||
| id: userId, | ||
| }, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| // Immediate deletion failed — register cleanup as fallback | ||
| console.error( | ||
| "Immediate user deletion failed, registering cleanup:", | ||
| error, | ||
| ); | ||
| cleanupFunctions.push(async () => { | ||
| await mercuriusClient.mutate(Mutation_deleteUser, { | ||
| headers: { | ||
| authorization: `bearer ${authToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: userId, | ||
| }, | ||
| }, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Mercurius v16 test clients, are GraphQL resolver/mutation failures returned on result.errorsinstead of being thrown as exceptions forquery/mutate calls?
💡 Result:
Yes. Mercurius test clients return GraphQL execution failures (e.g., a resolver/mutation throws) as a normal GraphQL response with a populated errors array (i.e., result.errors), rather than throwing from query() / mutate() themselves. The mercurius-integration-testing client explicitly documents queryResponse.errors / mutationResponse.errors as the “Possible array of errors from the API.” [1] Mercurius’s own programmatic API (app.graphql(...)) likewise returns the GraphQL execution result object (which, per GraphQL conventions, includes errors when execution fails) instead of treating resolver errors as thrown exceptions. [2]
Sources: [1] [2]
Handle GraphQL mutation errors explicitly in stale-token creation.
The current try/catch only catches thrown exceptions. Mercurius returns GraphQL errors as a populated result.errors array rather than throwing exceptions. If Mutation_deleteUser returns GraphQL errors in the response, this helper skips the fallback cleanup registration entirely.
🛠️ Proposed fix
const userId = user.user.id;
+ const cleanupDeleteUser = async () => {
+ await mercuriusClient.mutate(Mutation_deleteUser, {
+ headers: {
+ authorization: `bearer ${authToken}`,
+ },
+ variables: {
+ input: { id: userId },
+ },
+ });
+ };
+ // Register fallback cleanup first; remove it only after verified success.
+ cleanupFunctions.push(cleanupDeleteUser);
- // Delete the user immediately; register cleanup only if deletion fails
- try {
- await mercuriusClient.mutate(Mutation_deleteUser, {
- headers: {
- authorization: `bearer ${authToken}`,
- },
- variables: {
- input: {
- id: userId,
- },
- },
- });
- } catch (error) {
- // Immediate deletion failed — register cleanup as fallback
- console.error(
- "Immediate user deletion failed, registering cleanup:",
- error,
- );
- cleanupFunctions.push(async () => {
- await mercuriusClient.mutate(Mutation_deleteUser, {
- headers: {
- authorization: `bearer ${authToken}`,
- },
- variables: {
- input: {
- id: userId,
- },
- },
- });
- });
- }
+ const deleteResult = await mercuriusClient.mutate(Mutation_deleteUser, {
+ headers: {
+ authorization: `bearer ${authToken}`,
+ },
+ variables: {
+ input: { id: userId },
+ },
+ });
+ if (deleteResult.errors?.length) {
+ throw new Error(
+ `Immediate user deletion failed: ${JSON.stringify(deleteResult.errors)}`,
+ );
+ }
+ cleanupFunctions.pop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/graphql/types/Query/event.test.ts` around lines 218 - 248, The current
try/catch around mercuriusClient.mutate(Mutation_deleteUser, ...) only handles
thrown exceptions but ignores GraphQL errors returned in the mutation response;
update the call to capture the mutation result (e.g., const result = await
mercuriusClient.mutate(...)) and check result.errors (or result.data validity)
after the call and, if errors exist or deletion was not confirmed, register the
cleanupFunctions push fallback just like in the catch branch; keep the catch to
handle thrown exceptions and ensure references to Mutation_deleteUser,
mercuriusClient.mutate, cleanupFunctions, authToken and userId are used to
locate and modify the code.
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. |
github-actions
Bot
added
no-pr-activity
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. |
github-actions
Bot
added
no-pr-activity
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. |
github-actions
Bot
added
no-pr-activity
|
This pull request did not get any activity in the past 10 days and will be closed in 360 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. |
github-actions
Bot
added
no-pr-activity
3 participants
What kind of change does this PR introduce?
Performance optimization (test suite)
Issue Number:
Fixes #5258
Snapshots/Videos:
N/A - This is a test performance optimization with no visual changes.
If relevant, did you update the documentation?
No documentation updates needed. This is an internal test optimization.
Summary
This PR optimizes test performance in
test/graphql/types/Query/event.test.tsby implementing token caching for admin authentication.Problem:
The helper functions
getAdminToken()andgetAdminTokenAndUserId()were performing fresh GraphQL sign-in mutations on every call. With 13+ calls throughout the test suite, this created significant performance overhead and increased test execution time.Solution:
Refactored the helper functions to use the existing
ensureAdminAuth()caching mechanism that was already present in the file but not being utilized by these helpers.Changes:
getAdminTokenAndUserId()to callensureAdminAuth()instead of performing fresh sign-insgetAdminToken()to callensureAdminAuth()instead of performing fresh sign-insPerformance Impact:
beforeAllhook)Test Isolation:
This optimization maintains proper test isolation because:
beforeAllusage with immutable setupDoes this PR introduce a breaking change?
No. This is a test-only optimization with no changes to production code or test behavior.
Checklist
CodeRabbit AI Review
Test Coverage
Other information
This optimization was identified during the review of PR #5188. The caching mechanism (
ensureAdminAuth()) was already present in the file but the helper functions were not using it, resulting in unnecessary performance overhead.Have you read the contributing guide?
Yes
Summary by CodeRabbit
Refactor
Tests
Chores