test(federation): fix display name test flakyness - #40085
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
WalkthroughThe change modifies end-to-end federation matrix DM tests by increasing timeout thresholds, adding display-name reset logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40085 +/- ##
===========================================
- Coverage 70.13% 70.10% -0.03%
===========================================
Files 3273 3273
Lines 116239 116239
Branches 20775 20616 -159
===========================================
- Hits 81522 81495 -27
- Misses 31426 31449 +23
- Partials 3291 3295 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
test-improve-federation-tests-order-again
branch
from
April 8, 2026 20:26
66eca08 to
1a20a64
Compare
sampaiodiego
changed the title
sampaiodiego
force-pushed
the
test-improve-federation-tests-order-again
branch
from
April 8, 2026 22:19
1a20a64 to
41207c5
Compare
sampaiodiego
marked this pull request as ready for review
April 8, 2026 22:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts`:
- Around line 1915-1925: The cleanup currently performs deleteUser(rcUser, {},
rc1AdminRequestConfig) before attempting the Synapse admin display-name reset,
so if deleteUser throws the reset is skipped; move the display-name reset
(hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser)) into a
finally block so it always runs regardless of deleteUser errors, i.e., wrap the
deleteUser call in try/catch or try and perform the setDisplayName in the
finally clause of the same afterAll to guarantee the admin display name is
reset.
- Around line 1906-1912: The retry call waiting for rcRoom uses the helper
defaults (very short budget); update the retry invocation around
rcUserConfig.request.get(api('rooms.get')) so it uses the longer retry budget
used elsewhere in federation setup (e.g., pass the same timeout/attempts/backoff
options used by other federation waits) to avoid intermittent missing rcRoom;
modify the retry(...) call that wraps the rooms.get request (which assigns
rcRoom) to include the longer retry configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1496a1a-2656-4c47-b3b3-a4e7d36459ce
📒 Files selected for processing (1)
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
🧠 Learnings (21)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
📚 Learning: 2026-03-02T16:31:41.304Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-02-04T12:09:05.769Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-03-06T18:02:20.381Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/RelativeTime.spec.tsx:63-70
Timestamp: 2026-03-06T18:02:20.381Z
Learning: In RocketChat/Rocket.Chat, tests in the `packages/gazzodown` package (and likely the broader test suite) are always expected to run in the UTC timezone. This makes `toLocaleString()` output deterministic, so snapshot tests that include locale/timezone-sensitive content (such as `title` attributes from `toLocaleString()`) are stable and do not need to be replaced with targeted assertions.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
Comment on lines
+1906
to
+1912
| await retry('waiting for the room to be created in RC', async () => { | ||
| const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200); | ||
|
|
||
| rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId); | ||
|
|
||
| expect(rcRoom).toHaveProperty('_id'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the longer retry budget in this new setup poll.
This retry() call still uses the helper defaults, which only gives you ~40ms total here. That is much shorter than the other federation waits in this PR, so rcRoom can still be missing intermittently during setup.
Suggested fix
- await retry('waiting for the room to be created in RC', async () => {
- const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200);
-
- rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId);
-
- expect(rcRoom).toHaveProperty('_id');
- });
+ await retry(
+ 'waiting for the room to be created in RC',
+ async () => {
+ const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200);
+
+ rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId);
+
+ expect(rcRoom).toHaveProperty('_id');
+ },
+ { retries: 5, delayMs: 1000 },
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await retry('waiting for the room to be created in RC', async () => { | |
| const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200); | |
| rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId); | |
| expect(rcRoom).toHaveProperty('_id'); | |
| }); | |
| await retry( | |
| 'waiting for the room to be created in RC', | |
| async () => { | |
| const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200); | |
| rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId); | |
| expect(rcRoom).toHaveProperty('_id'); | |
| }, | |
| { retries: 5, delayMs: 1000 }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts` around lines 1906
- 1912, The retry call waiting for rcRoom uses the helper defaults (very short
budget); update the retry invocation around
rcUserConfig.request.get(api('rooms.get')) so it uses the longer retry budget
used elsewhere in federation setup (e.g., pass the same timeout/attempts/backoff
options used by other federation waits) to avoid intermittent missing rcRoom;
modify the retry(...) call that wraps the rooms.get request (which assigns
rcRoom) to include the longer retry configuration.
Comment on lines
+1915
to
+1925
| afterAll(async () => { | ||
| await deleteUser(rcUser, {}, rc1AdminRequestConfig); | ||
|
|
||
| // Best-effort reset of the Synapse admin display name. | ||
| // The top-level beforeAll will also reset it on the next run, | ||
| // so this is not critical for test reliability. | ||
| try { | ||
| await hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser); | ||
| } catch { | ||
| // ignore — the top-level beforeAll will handle it | ||
| } |
Contributor
There was a problem hiding this comment.
Run the Synapse display-name reset from finally.
If deleteUser() fails on Line 1916, the reset never runs and the shared admin display name stays dirty for whatever executes next. That keeps the cross-test contamination path open.
Suggested fix
afterAll(async () => {
- await deleteUser(rcUser, {}, rc1AdminRequestConfig);
-
- // Best-effort reset of the Synapse admin display name.
- // The top-level beforeAll will also reset it on the next run,
- // so this is not critical for test reliability.
- try {
- await hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser);
- } catch {
- // ignore — the top-level beforeAll will handle it
- }
+ try {
+ await deleteUser(rcUser, {}, rc1AdminRequestConfig);
+ } finally {
+ try {
+ await hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser);
+ } catch {
+ // ignore — the top-level beforeAll will handle it
+ }
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| afterAll(async () => { | |
| await deleteUser(rcUser, {}, rc1AdminRequestConfig); | |
| // Best-effort reset of the Synapse admin display name. | |
| // The top-level beforeAll will also reset it on the next run, | |
| // so this is not critical for test reliability. | |
| try { | |
| await hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser); | |
| } catch { | |
| // ignore — the top-level beforeAll will handle it | |
| } | |
| afterAll(async () => { | |
| try { | |
| await deleteUser(rcUser, {}, rc1AdminRequestConfig); | |
| } finally { | |
| try { | |
| await hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser); | |
| } catch { | |
| // ignore — the top-level beforeAll will handle it | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts` around lines 1915
- 1925, The cleanup currently performs deleteUser(rcUser, {},
rc1AdminRequestConfig) before attempting the Synapse admin display-name reset,
so if deleteUser throws the reset is skipped; move the display-name reset
(hs1AdminApp.matrixClient.setDisplayName(federationConfig.hs1.adminUser)) into a
finally block so it always runs regardless of deleteUser errors, i.e., wrap the
deleteUser call in try/catch or try and perform the setDisplayName in the
finally clause of the same afterAll to guarantee the admin display name is
reset.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts">
<violation number="1" location="ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts:1906">
P2: This `retry()` call still uses default parameters, while every other federation `retry()` in this PR was updated to `{ retries: 5, delayMs: 1000 }`. The default budget is likely too short for federation room creation, which is the exact class of flakiness this PR aims to fix.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+1906
to
+1912
| await retry('waiting for the room to be created in RC', async () => { | ||
| const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200); | ||
|
|
||
| rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId); | ||
|
|
||
| expect(rcRoom).toHaveProperty('_id'); | ||
| }); |
Contributor
There was a problem hiding this comment.
P2: This retry() call still uses default parameters, while every other federation retry() in this PR was updated to { retries: 5, delayMs: 1000 }. The default budget is likely too short for federation room creation, which is the exact class of flakiness this PR aims to fix.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts, line 1906:
<comment>This `retry()` call still uses default parameters, while every other federation `retry()` in this PR was updated to `{ retries: 5, delayMs: 1000 }`. The default budget is likely too short for federation room creation, which is the exact class of flakiness this PR aims to fix.</comment>
<file context>
@@ -1868,10 +1870,83 @@ const waitForRoomEvent = async (
+
+ hs1Room = (await hs1AdminApp.createDM([userDmId])) as Room;
+
+ await retry('waiting for the room to be created in RC', async () => {
+ const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200);
+
</file context>
Suggested change
| await retry('waiting for the room to be created in RC', async () => { | |
| const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200); | |
| rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId); | |
| expect(rcRoom).toHaveProperty('_id'); | |
| }); | |
| await retry( | |
| 'waiting for the room to be created in RC', | |
| async () => { | |
| const roomsResponse = await rcUserConfig.request.get(api('rooms.get')).set(rcUserConfig.credentials).expect(200); | |
| rcRoom = roomsResponse.body.update.find((room: IRoomNativeFederated) => room.federation.mrid === hs1Room.roomId); | |
| expect(rcRoom).toHaveProperty('_id'); | |
| }, | |
| { retries: 5, delayMs: 1000 }, | |
| ); |
ggazzo
approved these changes
Apr 9, 2026
ggazzo
approved these changes
Apr 9, 2026
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
The main changes in this PR to improve the reliability of the tests are moving the "display name" tests to the very end of the test suite and also increasing the time between retries, so giving more time for federation events to be processed and sent down the line.
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
FGA-61