Skip to content

test(federation): fix test to assert better subscription names when invite comes from Rocket.Chat #37969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggazzo merged 1 commit into from
Dec 29, 2025
Merged

test(federation): fix test to assert better subscription names when invite comes from Rocket.Chat #37969

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 45 additions & 37 deletions ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@
const room = await hs1User.matrixClient.getRoom(rcRoom.federation.mrid);

expect(room).toBeDefined();
expect(room!.getMyMembership()).toBe('invite');

Check warning on line 416 in ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts

View workflow job for this annotation

GitHub Actions / 🔎 Code Check / Code Lint 

Forbidden non-null assertion
});

// Accept the invitation
Expand Down Expand Up @@ -861,55 +861,63 @@
let rcRoom: IRoomNativeFederated;
let hs1Room1: Room;

let rcUser1: TestUser<IUser>;
let rcUserConfig1: IRequestConfig;

let rcUser2: TestUser<IUser>;
let rcUserConfig2: IRequestConfig;

const rcUserName1 = `dm-rc-multi-user1-${Date.now()}`;
const rcUserFullName1 = `DM RC Multi User1 ${Date.now()}`;
const rcUser1 = {
username: `dm-rc-multi-user1-${Date.now()}`,
fullName: `DM RC Multi User1 ${Date.now()}`,
get matrixId() {
return `@${this.username}:${federationConfig.rc1.domain}`;
},
config: {} as IRequestConfig,
user: {} as TestUser<IUser>,
};

const rcUserName2 = `dm-rc-multi-user2-${Date.now()}`;
const rcUserFullName2 = `DM RC Multi User2 ${Date.now()}`;
const rcUser2 = {
username: `dm-rc-multi-user2-${Date.now()}`,
fullName: `DM RC Multi User2 ${Date.now()}`,
get matrixId() {
return `@${this.username}:${federationConfig.rc1.domain}`;
},
config: {} as IRequestConfig,
user: {} as TestUser<IUser>,
};
Comment on lines +864 to +882
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unsafe initialization with type assertions could cause runtime errors.

The properties config and user are initialized with empty objects cast to their respective types ({} as IRequestConfig, {} as TestUser<IUser>). This bypasses type safety and creates invalid objects that could cause runtime errors if accessed before beforeAll completes or if setup fails.

🔎 Safer initialization approaches

Option 1: Use optional properties and proper initialization

 const rcUser1 = {
   username: `dm-rc-multi-user1-${Date.now()}`,
   fullName: `DM RC Multi User1 ${Date.now()}`,
   get matrixId() {
     return `@${this.username}:${federationConfig.rc1.domain}`;
   },
-  config: {} as IRequestConfig,
-  user: {} as TestUser<IUser>,
+  config: undefined as IRequestConfig | undefined,
+  user: undefined as TestUser<IUser> | undefined,
 };

 const rcUser2 = {
   username: `dm-rc-multi-user2-${Date.now()}`,
   fullName: `DM RC Multi User2 ${Date.now()}`,
   get matrixId() {
     return `@${this.username}:${federationConfig.rc1.domain}`;
   },
-  config: {} as IRequestConfig,
-  user: {} as TestUser<IUser>,
+  config: undefined as IRequestConfig | undefined,
+  user: undefined as TestUser<IUser> | undefined,
 };

Then add non-null assertions when using: rcUser1.config!.request (or check for undefined).

Option 2: Initialize in a factory function

const createUserDescriptor = (username: string, fullName: string) => ({
  username,
  fullName,
  get matrixId() {
    return `@${this.username}:${federationConfig.rc1.domain}`;
  },
  config: null as IRequestConfig | null,
  user: null as TestUser<IUser> | null,
});
📝 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
const rcUser1 = {
username: `dm-rc-multi-user1-${Date.now()}`,
fullName: `DM RC Multi User1 ${Date.now()}`,
get matrixId() {
return `@${this.username}:${federationConfig.rc1.domain}`;
},
config: {} as IRequestConfig,
user: {} as TestUser<IUser>,
};
const rcUserName2 = `dm-rc-multi-user2-${Date.now()}`;
const rcUserFullName2 = `DM RC Multi User2 ${Date.now()}`;
const rcUser2 = {
username: `dm-rc-multi-user2-${Date.now()}`,
fullName: `DM RC Multi User2 ${Date.now()}`,
get matrixId() {
return `@${this.username}:${federationConfig.rc1.domain}`;
},
config: {} as IRequestConfig,
user: {} as TestUser<IUser>,
};
const rcUser1 = {
username: `dm-rc-multi-user1-${Date.now()}`,
fullName: `DM RC Multi User1 ${Date.now()}`,
get matrixId() {
return `@${this.username}:${federationConfig.rc1.domain}`;
},
config: undefined as IRequestConfig | undefined,
user: undefined as TestUser<IUser> | undefined,
};
const rcUser2 = {
username: `dm-rc-multi-user2-${Date.now()}`,
fullName: `DM RC Multi User2 ${Date.now()}`,
get matrixId() {
return `@${this.username}:${federationConfig.rc1.domain}`;
},
config: undefined as IRequestConfig | undefined,
user: undefined as TestUser<IUser> | undefined,
};


beforeAll(async () => {
// Create RC user
rcUser1 = await createUser(
rcUser1.user = await createUser(
{
username: rcUserName1,
username: rcUser1.username,
password: 'random',
email: `${rcUserName1}@rocket.chat`,
name: rcUserFullName1,
email: `${rcUser1.username}@rocket.chat`,
name: rcUser1.fullName,
},
rc1AdminRequestConfig,
);

rcUserConfig1 = await getRequestConfig(federationConfig.rc1.url, rcUser1.username, 'random');
rcUser1.config = await getRequestConfig(federationConfig.rc1.url, rcUser1.username, 'random');

rcUser2 = await createUser(
rcUser2.user = await createUser(
{
username: rcUserName2,
username: rcUser2.username,
password: 'random',
email: `${rcUserName2}@rocket.chat`,
name: rcUserFullName2,
email: `${rcUser2.username}@rocket.chat`,
name: rcUser2.fullName,
},
rc1AdminRequestConfig,
);

rcUserConfig2 = await getRequestConfig(federationConfig.rc1.url, rcUser2.username, 'random');
rcUser2.config = await getRequestConfig(federationConfig.rc1.url, rcUser2.username, 'random');
});

afterAll(async () => {
await deleteUser(rcUser1, {}, rc1AdminRequestConfig);
await deleteUser(rcUser2, {}, rc1AdminRequestConfig);
await deleteUser(rcUser1.user, {}, rc1AdminRequestConfig);
await deleteUser(rcUser2.user, {}, rc1AdminRequestConfig);
});

it('should create a group DM with a Synapse and Rocket.Chat user', async () => {
// Create group DM from RC user to two Synapse users
const response = await rcUserConfig1.request
const response = await rcUser1.config.request
.post(api('dm.create'))
.set(rcUserConfig1.credentials)
.set(rcUser1.config.credentials)
.send({
usernames: [federationConfig.hs1.adminMatrixUserId, rcUser2.username].join(','),
})
Expand All @@ -918,7 +926,7 @@
expect(response.body).toHaveProperty('success', true);
expect(response.body).toHaveProperty('room');

const roomInfo = await getRoomInfo(response.body.room._id, rcUserConfig1);
const roomInfo = await getRoomInfo(response.body.room._id, rcUser1.config);

expect(roomInfo).toHaveProperty('room');

Expand All @@ -943,19 +951,19 @@

it('should display the fname containing the two invited users for the inviter', async () => {
// Check the subscription for the inviter
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUserConfig1.credentials, rcUserConfig1.request);
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser1.config.credentials, rcUser1.config.request);

// Should contain both invited users in the name
expect(sub).toHaveProperty('name', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser2.username}`);
expect(sub).toHaveProperty('fname', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser2.name}`);
expect(sub).toHaveProperty('fname', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser2.fullName}`);
});

it.failing("should display only the inviter's username for the invited user on Rocket.Chat", async () => {
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUserConfig2.credentials, rcUserConfig2.request);
it("should display only the inviter's username for the invited user on Rocket.Chat", async () => {
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser2.config.credentials, rcUser2.config.request);

expect(sub).toHaveProperty('status', 'INVITED');
expect(sub).toHaveProperty('name', rcUser1.name);
expect(sub).toHaveProperty('fname', rcUser1.username);
expect(sub).toHaveProperty('name', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.username}`);
expect(sub).toHaveProperty('fname', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.fullName}`);
});
Comment on lines +961 to 967
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Test name doesn't match the assertion.

The test is named "should display only the inviter's username for the invited user on Rocket.Chat" but the assertion checks that the subscription name includes both federationConfig.hs1.adminMatrixUserId and rcUser1.username, not just the inviter's username.

🔎 Suggested fix 
-it("should display only the inviter's username for the invited user on Rocket.Chat", async () => {
+it("should display all group DM members for invited Rocket.Chat user from same server", async () => {
   const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser2.config.credentials, rcUser2.config.request);

   expect(sub).toHaveProperty('status', 'INVITED');
   expect(sub).toHaveProperty('name', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.username}`);
   expect(sub).toHaveProperty('fname', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.fullName}`);
 });

This better reflects that when Rocket.Chat invites another Rocket.Chat user on the same server, it's acceptable to display all members (as mentioned in the PR objectives).

🤖 Prompt for AI Agents 
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts around lines 961 to
967: the test title says "should display only the inviter's username for the
invited user on Rocket.Chat" but the assertions expect the subscription
name/fname to include both the Matrix admin user ID and the inviter's
username/fullName; update the test title to accurately reflect the expectation
(e.g., "should display inviter and Matrix admin identifiers for the invited user
on Rocket.Chat" or similar) so the name matches the assertion, or alternatively
change the assertions to match the original intent if you truly want only the
inviter shown.


it('should accept the invitation on Synapse', async () => {
Expand All @@ -974,15 +982,15 @@
expect(hs1Room1.name).toBe(`${rcUser1.username} and ${rcUser2.username}`);
});

it.failing('should keep the fname to the RC invited user when the Synapse invited user accepts the DM', async () => {
it('should keep the fname to the RC invited user when the Synapse invited user accepts the DM', async () => {
await retry(
'this is an async operation, so we need to wait for the event to be processed',
async () => {
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUserConfig2.credentials, rcUserConfig2.request);
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser2.config.credentials, rcUser2.config.request);

expect(sub).toHaveProperty('status', 'INVITED');
expect(sub).toHaveProperty('name', rcUser1.name);
expect(sub).toHaveProperty('fname', rcUser1.username);
expect(sub).toHaveProperty('name', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.username}`);
expect(sub).toHaveProperty('fname', `${federationConfig.hs1.adminMatrixUserId}, ${rcUser1.fullName}`);
},
{ delayMs: 100 },
);
Expand All @@ -994,10 +1002,10 @@
await retry(
'this is an async operation, so we need to wait for the event to be processed',
async () => {
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUserConfig1.credentials, rcUserConfig1.request);
const sub = await getSubscriptionByRoomId(rcRoom._id, rcUser1.config.credentials, rcUser1.config.request);

expect(sub).toHaveProperty('name', rcUser2.username);
expect(sub).toHaveProperty('fname', rcUser2.name);
expect(sub).toHaveProperty('fname', rcUser2.fullName);
},
{ delayMs: 100 },
);
Expand Down
Loading