chore: federation-matrix integration tests #37219
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughThis PR extends test helper APIs to support injected request configurations and establishes comprehensive federation-matrix end-to-end testing infrastructure, including Jest configuration, Docker orchestration, SynapseClient wrapper, federation test scenarios, and CI integration. Changes
Sequence Diagram(s)sequenceDiagram
participant RC1 as Rocket.Chat (RC1)
participant HS1 as Synapse (HS1)
participant Admin as Test Admin
participant User as Test User
Admin->>RC1: Create federated room with HS1 user
RC1->>HS1: Send room invitation (federation)
HS1->>User: Notify room invitation
User->>HS1: Accept invitation
HS1->>RC1: Confirm acceptance (federation)
RC1->>RC1: Update room membership
Admin->>RC1: Query room info & members
RC1-->>Admin: Return room state with federated members
Admin->>HS1: Verify room via SynapseClient
HS1-->>Admin: Confirm membership across servers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ 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). (3)
🔇 Additional comments (5)
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 |
dhulke
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
apps/meteor/tests/data/rooms.helper.ts (1)
224-239: LGTM!The
getGroupHistoryfunction correctly implements the config pattern and provides proper type safety.Note: The code duplication concern was already raised in the review comment for
getRoomMembers(lines 169-184).apps/meteor/tests/data/users.helper.ts (3)
159-193: Inconsistent config.credentials handling in both functions.Both
setUserAwayandsetUserOnlinesuffer from the same inconsistency: they only useconfig?.requestand ignoreconfig?.credentials, usingoverrideCredentialsinstead.Apply the same fix to both functions:
export const setUserAway = ( overrideCredentials = credentials, config?: RequestConfig ) => { const requestInstance = config?.request || request; + const credentialsInstance = config?.credentials || overrideCredentials; return requestInstance .post(methodCall('UserPresence:away')) - .set(overrideCredentials) + .set(credentialsInstance) .send({ // ... }); }; export const setUserOnline = ( overrideCredentials = credentials, config?: RequestConfig ) => { const requestInstance = config?.request || request; + const credentialsInstance = config?.credentials || overrideCredentials; return requestInstance .post(methodCall('UserPresence:online')) - .set(overrideCredentials) + .set(credentialsInstance) .send({ // ... }); };
147-157: Inconsistent config.credentials handling.Similar to
getMe(lines 112-126), this function only usesconfig?.requestand ignoresconfig?.credentials, usingoverrideCredentialsinstead. This inconsistency should be addressed.Apply the same fix as suggested for
getMe:export const setUserStatus = ( overrideCredentials = credentials, status = UserStatus.ONLINE, config?: RequestConfig ) => { const requestInstance = config?.request || request; + const credentialsInstance = config?.credentials || overrideCredentials; - return requestInstance.post(api('users.setStatus')).set(overrideCredentials).send({ + return requestInstance.post(api('users.setStatus')).set(credentialsInstance).send({ message: '', status, }); };
195-211: Inconsistent config.credentials handling.The function correctly passes
configtogetUserByUsernamebut then ignoresconfig?.credentialswhen making the update request, usingoverrideCredentialsinstead.Apply this diff:
export const removeRoleFromUser = ( username: string, roleId: string, overrideCredentials = credentials, config?: RequestConfig ) => getUserByUsername(username, config).then((user) => { const requestInstance = config?.request || request; + const credentialsInstance = config?.credentials || overrideCredentials; return requestInstance .post(api('users.update')) - .set(overrideCredentials) + .set(credentialsInstance) .send({ userId: user._id, data: { roles: user.roles.filter((role) => role !== roleId) }, }) .expect(200); });
🧹 Nitpick comments (12)
apps/meteor/tests/data/rooms.helper.ts (2)
169-184: Prefer a helper function to reduce duplication.The config resolution and promise wrapper pattern is repeated across
getRoomInfo,getRoomMembers, andgetGroupHistory. Consider extracting a generic helper function to reduce duplication.Example helper:
const makeRoomRequest = <TEndpoint extends keyof Endpoints>( endpoint: TEndpoint, query: Record<string, any>, config?: RequestConfig ): Promise<ReturnType<Endpoints[TEndpoint]['GET']>> => { const requestInstance = config?.request || request; const credentialsInstance = config?.credentials || credentials; return new Promise((resolve) => { void requestInstance .get(api(endpoint)) .set(credentialsInstance) .query(query) .end((_err: any, req: any) => { resolve(req.body); }); }); };Then simplify each function to:
export const getRoomMembers = (roomId: IRoom['_id'], config?: RequestConfig) => makeRoomRequest('rooms.membersOrderedByRole', { roomId }, config);
186-222: LGTM! Optional: Consider structured logging.The retry logic with configurable delays and error handling is well-implemented. The function correctly handles the initial delay, retry loop, and returns
nullwhen the member is not found.The
console.warnon line 213 may create noisy test output. Consider using a structured logging approach or a debug flag to control logging verbosity in test environments.apps/meteor/tests/data/users.helper.ts (3)
10-13: Consider typing therequestproperty.The
requestproperty is typed asany, which sacrifices type safety. Consider typing it assupertest.SuperTest<supertest.Test>to enable better IntelliSense and catch type errors at compile time.Apply this diff:
+import type supertest from 'supertest'; + export interface RequestConfig { credentials: Credentials; - request: any; + request: supertest.SuperTest<supertest.Test>; }
15-23: Consider refactoring the credentials bootstrapping.The function works correctly but has two areas for improvement:
Line 17: Casting an empty object as
Credentials({} as Credentials) is a type-safety escape hatch. While this works becauselogindoesn't use the credentials parameter when building the login request itself, it's fragile.Line 21: The object property can use shorthand syntax.
Consider one of these approaches:
Option 1: Make the dependency explicit
export async function getRequestConfig(domain: string, user: string, password: string): Promise<RequestConfig> { const request = supertest(domain); - const credentials = await login(user, password, { request, credentials: {} as Credentials }); + // Login doesn't need credentials parameter for the initial login call itself + const credentials = await login(user, password, { request, credentials: null as any as Credentials }); return { credentials, - request: request, + request, }; }Option 2: Add an overload to login that doesn't require credentials in config
This would require changes to the
loginsignature andRequestConfiginterface to make credentials optional.
42-48: Remove redundant password in send payload.Line 48 includes
password: userData.passwordand then spreads...userData. IfuserDatacontains apasswordproperty, it will override the explicit one, making the explicit declaration redundant. IfuserData.passwordisundefined, spreading it would still work correctly.Apply this diff:
void requestInstance .post(api('users.create')) .set(credentialsInstance) - .send({ email, name: username, username, password: userData.password, ...userData }) + .send({ email, name: username, username, ...userData })Note: This assumes
userData.passwordis meant to be passed via the spread operator.ee/packages/federation-matrix/tests/helper/config.ts (1)
4-15: Add rc1.homeserver to config (avoid deriving Matrix domain from API URL)Tests currently derive the RC Matrix ID domain from
rc1.apiUrlby string replacement. That’s brittle if the URL includes ports/paths. Mirrorhs1.homeserverwithrc1.homeserver.Example change:
export interface FederationConfig { rc1: { apiUrl: string; + homeserver: string; adminUser: string; adminPassword: string; adminMatrixUserId: string; additionalUser1: { username: string; password: string; matrixUserId: string; }; };rc1: { apiUrl: validateEnvVar('FEDERATION_RC1_API_URL', process.env.FEDERATION_RC1_API_URL, 'https://rc1'), + homeserver: validateEnvVar('FEDERATION_RC1_HOMESERVER', process.env.FEDERATION_RC1_HOMESERVER, 'rc1'), adminUser: validateEnvVar('FEDERATION_RC1_ADMIN_USER', process.env.FEDERATION_RC1_ADMIN_USER, 'admin'), ... },Also applies to: 49-59
ee/packages/federation-matrix/tests/teardown.ts (1)
6-15: Teardown should explicitly stop Matrix clients/websocketsRelying on GC + delay won’t close SDK connections. Prefer an explicit registry and stop/logout all clients (e.g., client.stopClient(), remove listeners, close transports). This reduces reliance on
forceExit/detectOpenHandles.If feasible, expose a lightweight global registry (register/unregister) from your Synapse client helper and drain it here.
ee/packages/federation-matrix/tests/end-to-end/federation.spec.ts (3)
1-5: Simplify noisy relative import paths
../../.././../../is harder to read and maintain. Use clean relative paths or TS path aliases.Example:
-import { IS_EE } from '../../.././../../apps/meteor/tests/e2e/config/constants'; +import { IS_EE } from '../../../../apps/meteor/tests/e2e/config/constants';
33-37: Avoid deriving Matrix IDs from API URL
rc1User1MatrixIdis built by stripping schemes fromapiUrl. Use a dedicatedrc1.homeserverfrom config (mirrorshs1.homeserver) to compute@user:domain.Use:
@${rc1User1Username}:${federationConfig.rc1.homeserver}
24-81: Consider removing per-hook timeouts; rely on Jest’s 30s E2E default
beforeAll(..., 10000/15000)may be tight for federation propagation. Prefer the 30s default from Jest config (or increase if flakes observed).Remove the numeric timeout arg or bump to match global.
Also applies to: 200-238, 355-386
ee/packages/federation-matrix/tests/helper/synapse-client.ts (2)
26-31: Wait for initial sync before using the client
startClient()may return before rooms/memberships are available. Wait for first sync event to reduce flakiness.async initialize(): Promise<void> { const client = await this.createClient(this.username, this.password, this.url); - await client.startClient(); + await client.startClient(); + await new Promise<void>((resolve) => { + client.once('sync', () => resolve()); + }); this.matrixClient = client; }
180-189: Harden cleanup to avoid unhandled rejections during teardownWrap cleanup in try/catch to keep tests from failing on logout/clear errors.
async close(): Promise<void> { if (this.matrixClient) { - this.matrixClient.stopClient(); - await this.matrixClient.store?.deleteAllData?.(); - await this.matrixClient.clearStores?.(); - this.matrixClient.removeAllListeners(); - await this.matrixClient.logout(true); + try { + this.matrixClient.stopClient(); + await this.matrixClient.store?.deleteAllData?.(); + await this.matrixClient.clearStores?.(); + this.matrixClient.removeAllListeners(); + await this.matrixClient.logout(true); + } catch { + // swallow cleanup errors in tests + } this.matrixClient = null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
apps/meteor/.mocharc.federation.js(1 hunks)apps/meteor/package.json(1 hunks)apps/meteor/tests/data/api-data.ts(1 hunks)apps/meteor/tests/data/rooms.helper.ts(5 hunks)apps/meteor/tests/data/users.helper.ts(4 hunks)ee/packages/federation-matrix/jest.config.federation.ts(1 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/tests/end-to-end/federation.spec.ts(1 hunks)ee/packages/federation-matrix/tests/helper/config.ts(1 hunks)ee/packages/federation-matrix/tests/helper/synapse-client.ts(1 hunks)ee/packages/federation-matrix/tests/teardown.ts(1 hunks)
🔇 Additional comments (14)
apps/meteor/tests/data/api-data.ts (1)
3-7: Import spacing looks goodBlank line cleanly separates third-party imports from local modules; no issues here.
apps/meteor/.mocharc.federation.js (1)
7-20: Verify that referenced paths exist in the repository.The configuration looks correct overall. The timeout, bail, and retries settings are appropriate for integration tests. However, ensure that the referenced files and directories exist:
.mocharc.base.json(base configuration)tests/end-to-end/teardown.ts(teardown file)tests/end-to-end/federation/**/*(test spec pattern)tests/setup/chaiPlugins.ts(Chai plugins setup)Run the following script to verify the paths:
apps/meteor/tests/data/rooms.helper.ts (5)
2-6: LGTM!The new imports are necessary for the added functionality:
IUserfor thefindRoomMemberreturn type,Endpointsfor typed API responses, andRequestConfigfor the new config parameter pattern.
18-18: LGTM!Adding the optional
configparameter aligns with the pattern established inusers.helper.tsand maintains backward compatibility.
31-38: LGTM!The config-based request/credentials resolution is well-designed. The three-way fallback for
credentialsInstance(config?.credentials || customCredentials || credentials) correctly maintains backward compatibility with the existingcustomCredentialsparameter while supporting the new config mechanism.
46-50: LGTM!The room creation logic correctly uses the derived
requestInstanceandcredentialsInstance, maintaining consistency across both voip and generic room creation paths.Also applies to: 68-75
152-167: LGTM!The
getRoomInfofunction correctly implements the config pattern and provides proper type safety withReturnType<Endpoints['/v1/rooms.info']['GET']>.apps/meteor/tests/data/users.helper.ts (4)
57-76: LGTM!The
loginfunction correctly implements the config pattern, using onlyconfig?.request(credentials are not applicable for the login operation itself).
95-110: LGTM!The
getUserByUsernamefunction correctly implements the config pattern with proper type safety.
128-145: LGTM!The
setUserActiveStatusfunction correctly implements the config pattern.
78-93: Verify that the async function change is non-breaking.The function signature changed from a synchronous function returning a request object to an
asyncfunction. Ensure that all callers ofdeleteUserare updated to handle the new async signature.Run the following script to check all usages of
deleteUser:ee/packages/federation-matrix/jest.config.federation.ts (1)
4-21: Jest federation config looks goodPreset, E2E matchers, teardown, and transformIgnore for matrix-js-sdk are correctly set.
ee/packages/federation-matrix/package.json (1)
17-18: Remove unused Mocha and drop duplicated CLI flags in test script
- Jest is the configured runner; Mocha appears unused here. Remove it to avoid drift.
--forceExitand--testTimeoutare already set injest.config.federation.ts; keep config as single source of truth.Run this to confirm Mocha is unused:
Proposed edits:
"devDependencies": { ... - "matrix-js-sdk": "^38.4.0", - "mocha": "^11.7.4", + "matrix-js-sdk": "^38.4.0", ... }, "scripts": { ... - "testendtoend": "IS_EE=true jest --config jest.config.federation.ts --forceExit --testTimeout=30000", + "testendtoend": "IS_EE=true jest --config jest.config.federation.ts", ... }Also applies to: 26-26
ee/packages/federation-matrix/tests/helper/synapse-client.ts (1)
152-177: Constructing userId from URL is fragile; add timeout to admin fetch and ensure Node fetch availabilityString replace can leave trailing slashes/paths in the homeserver part (invalid userId). Use URL.host. Add an AbortController timeout to avoid hanging tests. Ensure Node provides global fetch or polyfill.
- const userId = `@${username}:${this.url.replace('https://', '').replace('http://', '')}`; + const host = new URL(this.url).host; // e.g., synapse:8008 + const userId = `@${username}:${host}`; try { // Use the admin API to create a user - const response = await fetch(`${this.url}/_synapse/admin/v2/users/${encodeURIComponent(userId)}`, { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 20_000); + const response = await fetch(`${this.url}/_synapse/admin/v2/users/${encodeURIComponent(userId)}`, { method: 'PUT', headers: { 'Authorization': `Bearer ${this.matrixClient.getAccessToken()}`, 'Content-Type': 'application/json', }, body: JSON.stringify({ password, displayname: displayName || username, admin: false, }), - }); + signal: controller.signal, + }).finally(() => clearTimeout(timeout));Action: Confirm CI/test Node version exposes global fetch; otherwise import a polyfill (e.g., undici or node-fetch) for the test environment.
dhulke
force-pushed
the
test/federation-integration
branch
from
10f3ea2 to
a80be3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
apps/meteor/.mocharc.federation.js (1)
3-5: Update the comment to reflect federation tests.The comment incorrectly states "REST API integration tests" but this file configures federation integration tests.
ee/packages/federation-matrix/tests/helper/config.ts (1)
55-57: Align label strings with env var keys for additionalUser1Labels should match the actual envs to avoid misleading errors.
- username: validateEnvVar('FEDERATION_RC1_ADDITIONAL_USER', process.env.FEDERATION_RC1_ADDITIONAL_USER1, 'user2'), - password: validateEnvVar('FEDERATION_RC1_ADDITIONAL_PASSWORD', process.env.FEDERATION_RC1_ADDITIONAL_USER1_PASSWORD, 'user2pass'), - matrixUserId: validateEnvVar('FEDERATION_RC1_ADDITIONAL_USER_ID', process.env.FEDERATION_RC1_ADDITIONAL_USER1_MATRIX_ID, '@user2:rc1'), + username: validateEnvVar('FEDERATION_RC1_ADDITIONAL_USER1', process.env.FEDERATION_RC1_ADDITIONAL_USER1, 'user2'), + password: validateEnvVar('FEDERATION_RC1_ADDITIONAL_USER1_PASSWORD', process.env.FEDERATION_RC1_ADDITIONAL_USER1_PASSWORD, 'user2pass'), + matrixUserId: validateEnvVar('FEDERATION_RC1_ADDITIONAL_USER1_MATRIX_ID', process.env.FEDERATION_RC1_ADDITIONAL_USER1_MATRIX_ID, '@user2:rc1'), @@ - username: validateEnvVar('FEDERATION_SYNAPSE_ADDITIONAL_USER', process.env.FEDERATION_SYNAPSE_ADDITIONAL_USER1, 'user2'), - password: validateEnvVar('FEDERATION_SYNAPSE_ADDITIONAL_PASSWORD', process.env.FEDERATION_SYNAPSE_ADDITIONAL_USER1_PASSWORD, 'user2pass'), - matrixUserId: validateEnvVar('FEDERATION_SYNAPSE_ADDITIONAL_USER_ID', process.env.FEDERATION_SYNAPSE_ADDITIONAL_USER1_MATRIX_ID, '@user2:hs1'), + username: validateEnvVar('FEDERATION_SYNAPSE_ADDITIONAL_USER1', process.env.FEDERATION_SYNAPSE_ADDITIONAL_USER1, 'user2'), + password: validateEnvVar('FEDERATION_SYNAPSE_ADDITIONAL_USER1_PASSWORD', process.env.FEDERATION_SYNAPSE_ADDITIONAL_USER1_PASSWORD, 'user2pass'), + matrixUserId: validateEnvVar('FEDERATION_SYNAPSE_ADDITIONAL_USER1_MATRIX_ID', process.env.FEDERATION_SYNAPSE_ADDITIONAL_USER1_MATRIX_ID, '@user2:hs1'),Also applies to: 68-70
ee/packages/federation-matrix/tests/helper/synapse-client.ts (3)
67-76: Same guard for membership lookup; simplify error message-getRoomIdByRoomNameAndMembership(roomName: string, membership: KnownMembership): string { - const room = this.matrixClient?.getRooms() - .find(room => room.name === roomName && room.getMyMembership() === membership); +getRoomIdByRoomNameAndMembership(roomName: string, membership: KnownMembership): string { + if (!this.matrixClient) { + throw new Error('Matrix client not initialized'); + } + const room = this.matrixClient.getRooms().find( + (room) => room.name === roomName && room.getMyMembership() === membership, + ); if (room) { return room.roomId; } - throw new Error(`No room found with name ${roomName} and membership ${membership.toString()}`); + throw new Error(`No room found with name ${roomName} and membership ${membership}`); }
56-65: Guard client initialization; avoid calling .find on undefined-getRoom(roomName: string): Room { - const room = this.matrixClient?.getRooms() - .find(room => room.name === roomName); +getRoom(roomName: string): Room { + if (!this.matrixClient) { + throw new Error('Matrix client not initialized'); + } + const room = this.matrixClient.getRooms().find((room) => room.name === roomName); if (room) { return room; } throw new Error(`No room found with name ${roomName}`); }
78-102: Invitation acceptance: guard init, clamp retries, remove optional join, don’t throw nullasync acceptInvitationForRoomName( roomName: string, maxRetries: number = 5, retryDelay: number = 1000, initialDelay: number = 5000 ): Promise<string> { - if (initialDelay) { + if (!this.matrixClient) { + throw new Error('Matrix client not initialized'); + } + if (initialDelay) { await wait(initialDelay); } - let lastError: Error | null = null; - for (let attempt = 1; attempt <= maxRetries; attempt++) { + const retries = Math.max(1, maxRetries); + let lastError: unknown = null; + for (let attempt = 1; attempt <= retries; attempt++) { try { const roomId = this.getRoomIdByRoomNameAndMembership(roomName, KnownMembership.Invite); - await this.matrixClient?.joinRoom(roomId); + await this.matrixClient.joinRoom(roomId); return roomId; } catch (error) { - if (attempt < maxRetries) { + if (attempt < retries) { await wait(retryDelay); } - lastError = error as Error; + lastError = error; } } - - throw lastError; + + throw new Error( + `Failed to accept invitation for "${roomName}" after ${retries} attempts: ${ + (lastError as Error | undefined)?.message ?? lastError + }`, + ); }
🧹 Nitpick comments (6)
ee/packages/federation-matrix/docker-compose/traefik/dynamic_conf.yml (1)
33-37: Avoid hard-coding host.docker.internal; prefer container DNS or config flag.host.docker.internal can fail on Linux. Consider pointing to the Rocket.Chat container by name on the Docker network, or make the backend URL configurable for CI/dev.
Example (if RC container is named rc1):
- servers: - - url: "http://host.docker.internal:3000" + servers: + - url: "http://rc1:3000"If you must reach the host, document Linux setup (extra_hosts) or gate the URL behind an env template used to generate this file.
apps/meteor/tests/data/rooms.helper.ts (2)
186-222: Optional: make retry helper cancellable and surface final error.To improve test ergonomics, add an optional AbortSignal/timeout and throw last error when no match is found (behind a flag) to aid debugging.
Example minimal tweaks:
- Accept options: { signal?: AbortSignal; throwOnNotFound?: boolean }.
- Before each wait, check signal?.aborted.
- After loop, if throwOnNotFound, throw new Error with username/roomId context.
152-167: Minor: use ‘res’ in callbacks for clarity, and consider rejecting on HTTP errors.Current pattern ignores _err; optionally reject on non-2xx to fail fast in tests. Variable rename improves readability.
- .end((_err: any, req: any) => { - resolve(req.body); - }); + .end((err: any, res: any) => { + if (err) return reject(err); + resolve(res.body); + });Also applies to: 224-239
ee/packages/federation-matrix/scripts/run-integration-tests.sh (2)
16-23: Fix SC2155 and reuse CA pathAvoid command-substitution in export and define a reusable CA_CERT:
@@ -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -PACKAGE_ROOT="$(dirname "$SCRIPT_DIR")" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PACKAGE_ROOT="$(dirname "$SCRIPT_DIR")" +CA_CERT="$PACKAGE_ROOT/docker-compose/traefik/certs/ca/rootCA.crt" @@ -# Set NODE_EXTRA_CA_CERTS to use the custom CA certificate -export NODE_EXTRA_CA_CERTS="$(pwd)/docker-compose/traefik/certs/ca/rootCA.crt" +# Set NODE_EXTRA_CA_CERTS to use the custom CA certificate +export NODE_EXTRA_CA_CERTS="$CA_CERT"Also applies to: 200-208
124-131: Compose v2 compatibility: use docker compose when availableMany environments only ship the v2 plugin. Prefer docker compose, fallback to docker-compose.
@@ -# Start services +# Start services +COMPOSE_CMD="docker compose" +if ! $COMPOSE_CMD version >/dev/null 2>&1; then + COMPOSE_CMD="docker-compose" +fi @@ - docker-compose -f "$DOCKER_COMPOSE_FILE" --profile test --profile element up -d --build + $COMPOSE_CMD -f "$DOCKER_COMPOSE_FILE" --profile test --profile element up -d --build @@ - docker-compose -f "$DOCKER_COMPOSE_FILE" --profile test up -d --build + $COMPOSE_CMD -f "$DOCKER_COMPOSE_FILE" --profile test up -d --buildAnd in cleanup:
- docker-compose -f "$DOCKER_COMPOSE_FILE" --profile test --profile element down -v 2>/dev/null || true + $COMPOSE_CMD -f "$DOCKER_COMPOSE_FILE" --profile test --profile element down -v 2>/dev/null || true @@ - docker-compose -f "$DOCKER_COMPOSE_FILE" --profile test down -v 2>/dev/null || true + $COMPOSE_CMD -f "$DOCKER_COMPOSE_FILE" --profile test down -v 2>/dev/null || trueAlso applies to: 91-100, 93-97
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (1)
114-121: Reduce flakiness: increase retries or initial delays for federation joinsFederation syncs can be slow; current delays may be tight. Consider bumping initialDelay/retry counts in acceptInvitationForRoomName/findRoomMember calls to reduce intermittent failures.
Also applies to: 219-230, 374-377, 447-467
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
apps/meteor/.mocharc.federation.js(1 hunks)apps/meteor/package.json(1 hunks)apps/meteor/tests/data/api-data.ts(1 hunks)apps/meteor/tests/data/rooms.helper.ts(5 hunks)apps/meteor/tests/data/users.helper.ts(4 hunks)ee/packages/federation-matrix/.env.example(1 hunks)ee/packages/federation-matrix/docker-compose.test.yml(1 hunks)ee/packages/federation-matrix/docker-compose/element/config.json(1 hunks)ee/packages/federation-matrix/docker-compose/hs1/homeserver.yaml(1 hunks)ee/packages/federation-matrix/docker-compose/hs1/hs1.log.config(1 hunks)ee/packages/federation-matrix/docker-compose/hs1/hs1.signing.key(1 hunks)ee/packages/federation-matrix/docker-compose/traefik/certs/ca/rootCA.crt(1 hunks)ee/packages/federation-matrix/docker-compose/traefik/dynamic_conf.yml(1 hunks)ee/packages/federation-matrix/docker-compose/traefik/traefik.yml(1 hunks)ee/packages/federation-matrix/jest.config.federation.ts(1 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/scripts/run-integration-tests.sh(1 hunks)ee/packages/federation-matrix/tests/end-to-end/room.spec.ts(1 hunks)ee/packages/federation-matrix/tests/helper/config.ts(1 hunks)ee/packages/federation-matrix/tests/helper/synapse-client.ts(1 hunks)ee/packages/federation-matrix/tests/teardown.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- ee/packages/federation-matrix/docker-compose/element/config.json
- ee/packages/federation-matrix/docker-compose/hs1/hs1.signing.key
- ee/packages/federation-matrix/.env.example
- ee/packages/federation-matrix/docker-compose/traefik/certs/ca/rootCA.crt
🚧 Files skipped from review as they are similar to previous changes (6)
- ee/packages/federation-matrix/tests/teardown.ts
- ee/packages/federation-matrix/jest.config.federation.ts
- apps/meteor/tests/data/api-data.ts
- apps/meteor/tests/data/users.helper.ts
- ee/packages/federation-matrix/package.json
- apps/meteor/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (4)
ee/packages/federation-matrix/tests/helper/synapse-client.ts (2)
SynapseClient(13-157)findRoomMember(110-145)apps/meteor/tests/data/users.helper.ts (2)
getRequestConfig(15-23)createUser(25-55)apps/meteor/tests/data/rooms.helper.ts (1)
getGroupHistory(224-239)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
ee/packages/federation-matrix/tests/helper/synapse-client.ts (1)
apps/meteor/tests/data/api-data.ts (1)
wait(14-16)
apps/meteor/tests/data/rooms.helper.ts (4)
apps/meteor/tests/data/users.helper.ts (1)
RequestConfig(10-13)apps/meteor/tests/data/api-data.ts (3)
request(11-11)credentials(40-43)api(47-49)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
🪛 Shellcheck (0.11.0)
ee/packages/federation-matrix/scripts/run-integration-tests.sh
[warning] 205-205: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/.mocharc.federation.js (1)
7-20: LGTM! Configuration is appropriate for federation integration tests.The Mocha configuration is well-structured:
- Appropriate timeout (10s) and slow threshold (6s) for integration tests
bail: trueensures fail-fast behavior- Spec pattern correctly targets federation tests
- Required modules (ts-node, chai plugins) are properly configured
apps/meteor/tests/data/rooms.helper.ts (2)
31-39: Config-driven request/credentials injection looks good.Using requestInstance/credentialsInstance with config fallbacks is clean and backward compatible.
Also applies to: 46-50, 68-75
169-184: No action needed: endpoint ‘rooms.membersOrderedByRole’ is defined in rest-typings and implemented in the server.ee/packages/federation-matrix/docker-compose/hs1/hs1.log.config (1)
24-28: Logging config is appropriate for tests.Console-only with SQL logger at INFO avoids sensitive debug logs. LGTM.
Also applies to: 32-37
dhulke
force-pushed
the
test/federation-integration
branch
2 times, most recently
from
374e5a7 to
7d34b34
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37219 +/- ##
===========================================
- Coverage 68.97% 68.96% -0.01%
===========================================
Files 3358 3358
Lines 114228 114228
Branches 20537 20537
===========================================
- Hits 78786 78782 -4
- Misses 33349 33353 +4
Partials 2093 2093
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dhulke
changed the title
dhulke
force-pushed
the
test/federation-integration
branch
2 times, most recently
from
190130b to
a595175
Compare
dhulke
force-pushed
the
test/federation-integration
branch
2 times, most recently
from
f386011 to
5b13ae1
Compare
…nd pre-built federation integration tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
123-123: Consider refactoring to reduce complexity in future work.The complexity directive suppresses a legitimate maintainability concern. The
createRoomfunction handles numerous responsibilities across ~190 lines: validation, permission checks, Apps engine integration, room creation, subscriptions, callbacks, and federation logic. While acceptable for this test-focused PR, consider decomposing this function in future work—for example, extracting validation, permission checks, and Apps engine interactions into separate helper functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/ci.yml(6 hunks).gitignore(1 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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: 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.
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.
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: 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.
📚 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:
apps/meteor/app/lib/server/functions/createRoom.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:
apps/meteor/app/lib/server/functions/createRoom.ts
🔇 Additional comments (6)
.gitignore (1)
57-58: Pattern allows test data to be tracked as intended for federation tests.The negation rule on Line 58 correctly re-includes
tests/data/directories while preserving the parentdata/ignore rule. This is consistent with existing negation patterns in the file (e.g.,.yarn/exceptions on lines 39–42) and aligns with the PR objective to introduce federation-matrix integration tests..github/workflows/ci.yml (5)
633-633: Verify that theENTERPRISE_LICENSE_RC1secret is defined.The step references
secrets.ENTERPRISE_LICENSE_RC1, but this secret may not be configured in the repository. Therelease-versionsjob uses a dummy enterprise license (enterprise-license) for similar purposes. Confirm that this secret exists and is properly configured in the repository settings.To verify the secret is defined, you can check the repository secrets via GitHub CLI or web interface. If this is a new secret, ensure it's added to the repository settings.
592-592: Clarify the runner architecture choice.This job uses
ubuntu-24.04(x86-64), while most other test jobs in the workflow useubuntu-24.04-arm. If this is intentional for federation testing compatibility or docker requirements, consider adding a comment. Otherwise, ensure consistency with the workflow pattern.Verify whether
ubuntu-24.04-armwould also work for federation integration tests or if x86-64 is a requirement.
624-627: Verify sudo availability and /etc/hosts modification persistence.The steps modify
/etc/hostsviasudo, assuming passwordless sudo is available on the runner. Confirm:
- The Ubuntu 24.04 runner allows passwordless sudo for these commands.
- These changes persist for the duration of the docker-compose services that follow.
Consider whether a docker compose
extra_hostsconfiguration would be more reliable than modifying the host file.
691-691: Verify the integration of test-federation-matrix into the dependency chain.The
test-federation-matrixjob is now a dependency oftests-done, and the aggregation logic checks its result. Ensure that:
- The job is properly configured to fail on test failures.
- The aggregation check (lines 724–726) is correct and consistent with other test job checks.
630-634: No issues found.The
yarn test:integration --imagecommand is properly defined in the federation-matrix package and correctly accepts the--imageparameter. The script atee/packages/federation-matrix/tests/scripts/run-integration-tests.shhandles the parameter as expected, with documentation at lines 70-75 confirming it supports both explicit image values and defaults torocketchat/rocket.chat:latestwhen provided without a value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
608-611: Inconsistent artifact download action version.This step uses
actions/download-artifact@v4, but other download-artifact steps throughout the workflow use@v6(lines 304, 399, 650, 745, 816). Update to@v6for consistency unless there's a specific reason for the older version.- name: Restore turbo build - uses: actions/download-artifact@v4 + uses: actions/download-artifact@v6 with: name: turbo-build path: .turbo/cache
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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: 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.
⏰ 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). (12)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
589-592: Verify runner OS selection for federation tests.Line 592 uses
ubuntu-24.04while all other e2e test jobs (test-api, test-ui, test-api-ee, test-ui-ee, test-ui-ee-watcher) useubuntu-24.04-arm. Confirm this is intentional (e.g., federation tests require x86 architecture or have unsupported tooling on ARM) or update for consistency.
632-634: Verify ENTERPRISE_LICENSE_RC1 secret is available.The federation test step references
${{ secrets.ENTERPRISE_LICENSE_RC1 }}, but this secret is not visible in the release-versions outputs or common secret definitions in the workflow. Confirm this secret is defined in the repository settings and available for federation matrix tests.
689-726: Federation test integration into test aggregation is correct.The test-federation-matrix job has been properly integrated into the tests-done aggregation: it's added as a dependency (line 691) and its result is checked (lines 724-726) to fail the overall test suite if federation tests do not succeed. This ensures federation test failures block the pipeline, aligning with other e2e test enforcement.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
624-627: Add error handling to /etc/hosts configuration.The /etc/hosts modifications (lines 624–627) don't check for success. If these modifications fail, the tests will proceed and fail downstream when trying to reach the federation services. Consider using
set -eor checking for errors explicitly.Suggested improvement:
- - name: Configure /etc/hosts for federation services - run: | - sudo -- sh -c "echo '127.0.0.1 hs1' >> /etc/hosts" - sudo -- sh -c "echo '127.0.0.1 rc1' >> /etc/hosts" + - name: Configure /etc/hosts for federation services + run: | + set -e + sudo -- sh -c "echo '127.0.0.1 hs1' >> /etc/hosts" + sudo -- sh -c "echo '127.0.0.1 rc1' >> /etc/hosts"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (12)
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
282-287: Formatting change only—no functional impact.The service matrix is restructured for readability; the content remains unchanged.
691-691: Federation test properly integrated into test aggregation.The
tests-donejob correctly includestest-federation-matrixin its needs (line 691) and checks its result (lines 724–726), following the established pattern for other test jobs. This ensures the CI pipeline fails if federation tests fail.Also applies to: 724-726
830-830: Whitespace/formatting adjustment only.This line contains no functional changes.
862-862: Whitespace/formatting adjustment only.This line contains no functional changes.
589-634: Original concern about runner architecture is not an issue; Docker login timing has minor risk.The non-arm runner choice (
ubuntu-24.04) is intentional and consistent with thereport-coveragejob. No flags needed there.The Docker login condition (lines 616–622) does create a minor timing asymmetry: login is conditional on repo ownership/release/develop branches, but the federation test (lines 629–634) runs unconditionally. On forks, the test will attempt to pull the pre-built image from
ghcr.iowithout authentication. Since the container registry is public, this should work, but you may encounter rate-limiting on unauthenticated pulls during concurrent test runs. The workflow already handles similar fork limitations elsewhere (see the dummy enterprise license inrelease-versions), so this is an acceptable trade-off.The
ee/packages/federation-matrixdirectory,test:integrationscript, and working directory path are all correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
592-592: Verify runner OS choice and consider setting timeout.The job runs on
ubuntu-24.04(without-arm), which differs from most other test jobs in this workflow that useubuntu-24.04-arm. Confirm this is intentional (e.g., for Synapse/Matrix compatibility or federation-specific requirements).Additionally, consider adding
timeout-minutesto the job to prevent indefinite hangs if federation tests stall or deadlock.Example timeout addition:
test-federation-matrix: name: 🔨 Test Federation Matrix needs: [checks, build-gh-docker-publish, packages-build, release-versions] runs-on: ubuntu-24.04 + timeout-minutes: 30Also applies to: 618-623
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
282-287: Matrix formatting improvement.The service matrix declaration is now more readable in multi-line format. No functional changes.
634-634: Verify ENTERPRISE_LICENSE_RC1 secret exists.The job references
ENTERPRISE_LICENSE_RC1, but this secret is not visible in the provided context. Verify that this secret is defined in the repository settings, or confirm whether a different secret or theenterprise-licenseoutput fromrelease-versionsshould be used instead.
635-635: Verify yarn test:integration command exists.Ensure that the
yarn test:integrationcommand is defined and functional in theee/packages/federation-matrixpackage. Confirm that the--imageflag is supported and that the command executes as expected.
692-692: Pipeline integration for federation tests.The federation test job is properly integrated into the test aggregation. The dependency declaration and result check follow the established pattern for other test jobs.
Also applies to: 725-727
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (12)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
692-692: Integration of federation test results into overall CI status looks good.The
test-federation-matrixjob is properly wired into the workflow:
- Dependency declared in
tests-done(line 692)- Result check added to fail if federation tests don't succeed (lines 725-727)
- Consistent pattern with other test jobs
Also applies to: 725-727
282-287: Minor formatting improvement to build matrix.Reformatted the
build-gh-dockermatrix'sservicefield to multi-line format for readability. No functional changes.
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 19:34", "11/15 19:47", "11/15 20:39", "11/15 21:23", "11/15 21:37", "11/15 22:04", "11/15 22:28", "11/16 00:55", "11/16 01:28", "11/17 12:35", "11/17 12:48", "11/17 12:54", "11/17 13:21 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
line "rocketchat" [0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 12 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 17 Nov 2025 13:21:52 GMT |
https://rocketchat.atlassian.net/browse/FDR-212
Proposed changes (including videos or screenshots)
Federation integration tests
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Tests
Chores