test(federation): improve and slim down config and scripts#37769
test(federation): improve and slim down config and scripts#37769sampaiodiego merged 6 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughConsolidates federation test configuration: introduces a typed FederationServerConfig and renames per-server field Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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 (1)
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 |
sampaiodiego
force-pushed
the
federation-tests-improve-scripts
branch
from
64edfad to
e2ec1f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
327-346: Update NO_TEST instructions to usetest:federationThe main path now runs
IS_EE=true NODE_EXTRA_CA_CERTS=… yarn test:federation, but the NO_TEST branch still tells users to runyarn testend-to-end. This is misleading with the new script name.Consider updating the hint to something like:
- log_info "To run tests manually, execute: yarn testend-to-end" + log_info "To run tests manually, execute: IS_EE=true NODE_EXTRA_CA_CERTS=\$(pwd)/docker-compose/traefik/certs/ca/rootCA.crt yarn test:federation"so manual runs match what the script does.
🧹 Nitpick comments (2)
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
27-29: Redundant removal of freshly createdBUILD_DIR
BUILD_DIRis created viamktemp -dand then immediately removed withrm -rf "$BUILD_DIR"before the Meteor build recreates it. That works in practice but is a bit confusing; you could drop the cleanup or switch torm -rf "$BUILD_DIR"/*to keep the directory while clearing contents.Also applies to: 191-193
ee/packages/federation-matrix/tests/helper/config.ts (1)
38-96: Centralized env handling is good; consider optional explicit URL overrides
validateEnvVarplusgetFederationConfignicely de-duplicate env parsing and enforce non‑empty values, while deriving:
urlashttps://${domain}- Matrix IDs as
@user:${domain}for both RC1 and HS1.
If you ever need to run federation tests against endpoints that don’t follow this pattern (e.g., different scheme/port or a nontrivial path), you might want to add optional
FEDERATION_RC1_URL/FEDERATION_SYNAPSE_URLenvs that, when set, override the derivedhttps://${domain}value, falling back to the current derivation otherwise. That keeps today’s behavior but restores flexibility for more exotic setups.
📜 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 (5)
ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/tests/end-to-end/messaging.spec.ts(1 hunks)ee/packages/federation-matrix/tests/end-to-end/room.spec.ts(4 hunks)ee/packages/federation-matrix/tests/helper/config.ts(3 hunks)ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh(7 hunks)
🧰 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/messaging.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/helper/config.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/messaging.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.ts
🧠 Learnings (7)
📚 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/package.jsonee/packages/federation-matrix/tests/end-to-end/messaging.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/helper/config.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/package.jsonee/packages/federation-matrix/tests/end-to-end/messaging.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/helper/config.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/federation-matrix/package.jsonee/packages/federation-matrix/tests/end-to-end/room.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/messaging.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/tests/scripts/run-integration-tests.sh
📚 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/messaging.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/helper/config.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/room.spec.tsee/packages/federation-matrix/tests/helper/config.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/room.spec.tsee/packages/federation-matrix/tests/helper/config.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/tests/end-to-end/messaging.spec.ts (1)
ee/packages/federation-matrix/tests/helper/config.ts (1)
federationConfig(113-113)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (2)
apps/meteor/tests/data/users.helper.ts (1)
getRequestConfig(35-43)ee/packages/federation-matrix/tests/helper/ddp-listener.ts (1)
createDDPListener(166-168)
🔇 Additional comments (6)
ee/packages/federation-matrix/package.json (1)
17-17: Newtest:federationscript wiring looks correctHooking Jest via
jest.config.federation.tsmatches the updated integration runner and cleanly scopes federation tests; no issues here.ee/packages/federation-matrix/tests/end-to-end/messaging.spec.ts (1)
22-28: Switch tofederationConfig.rc1.urlis consistent with new config helperUsing
federationConfig.rc1.urlas the base URL forgetRequestConfigmatches the newFederationServerConfigshape and still provides a full origin tosupertest; behavior stays the same.ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (1)
32-36: Consistent use offederationConfig.rc1.urlfor HTTP and DDP clientsUpdating all RC1 request/connection points (
getRequestConfigandcreateDDPListener) to usefederationConfig.rc1.urlmakes the origin a single source of truth underFederationServerConfig. The change is type‑safe and should be behavior‑preserving as long as your newurl(https://${FEDERATION_RC1_DOMAIN}) matches whatapiUrlused to be, including any port differences.Please confirm that any environments which previously relied on a custom RC1 API URL (e.g., non‑443 port or extra path) are still correctly covered by the new domain‑based URL derivation; if not, you may need an explicit
FEDERATION_RC1_URLoverride in the helper config.Also applies to: 50-54, 85-88, 258-261
ee/packages/federation-matrix/tests/scripts/run-integration-tests.sh (1)
262-302: Capturing curl output inwait_for_serviceimproves diagnosabilityStoring
curl_output/curl_exit_codeand logging the error when services are not yet ready is a solid debugging aid for federation test flakiness; scope is limited to local rc1/hs1 health checks, so no concern from a security/noise standpoint.ee/packages/federation-matrix/tests/helper/config.ts (2)
9-24: FederationServerConfig shape is clear and symmetricDefining
FederationServerConfigand using it for bothrc1andhs1gives a clean, consistent contract (URL, domain, admin, additional user) that matches how the tests consume the config. No issues here.
105-111: Fail-fast federation config initialization is appropriateInitializing
federationConfigat module load and exiting the process on configuration errors makes misconfigured environments fail immediately instead of producing confusing test failures later; that’s a good trade‑off for this test helper.
📦 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 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 17:41", "12/11 19:29 (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, 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, 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, 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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
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, 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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
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, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 18 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 11 Dec 2025 19:29:40 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37769 +/- ##
===========================================
+ Coverage 67.70% 67.79% +0.08%
===========================================
Files 3452 3452
Lines 113975 113985 +10
Branches 20940 20943 +3
===========================================
+ Hits 77168 77274 +106
+ Misses 34678 34580 -98
- Partials 2129 2131 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ee/packages/federation-matrix/tests/setup-qase.ts (1)
101-143: Apply the same fix to global.test wrapper.The global.test wrapper has the same suitePathStack manipulation issue as global.it. Apply the same simplification here.
📜 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 (2)
ee/packages/federation-matrix/jest.config.federation.ts(2 hunks)ee/packages/federation-matrix/tests/setup-qase.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.ts
🧠 Learnings (14)
📓 Common learnings
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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
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: 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
📚 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/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
ee/packages/federation-matrix/jest.config.federation.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/federation-matrix/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.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/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.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/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
ee/packages/federation-matrix/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.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/jest.config.federation.tsee/packages/federation-matrix/tests/setup-qase.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 `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.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 `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.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 : Group related tests in the same file
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.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 `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.ts
🔇 Additional comments (2)
ee/packages/federation-matrix/jest.config.federation.ts (2)
11-25: LGTM! Clean dynamic title generation.The function correctly builds a descriptive Qase run title by incorporating optional CI metadata (PR number, GitHub run ID) and a timestamp for unique identification.
55-58: LGTM! Dynamic title integration works correctly.The Qase run configuration now uses the dynamically generated title, providing better traceability for federation test runs in CI.
| global.it = Object.assign( | ||
| (testName: any, fn?: any, timeout?: number) => { | ||
| // Handle qase-wrapped test names (qase returns a string) | ||
| if (typeof testName === 'string' && fn) { | ||
| return currentIt( | ||
| testName, | ||
| async () => { | ||
| // Set suite immediately at the start of the test | ||
| qase.suite(currentPath); | ||
| // Call the original test function and return the result | ||
| return fn(); | ||
| }, | ||
| timeout, | ||
| ); | ||
| } | ||
| // Handle cases where testName might be a number or other type | ||
| return currentIt(testName, fn, timeout); | ||
| }, | ||
| { | ||
| skip: (name: string, fn: () => void) => { | ||
| suitePathStack.push(name); | ||
| try { | ||
| currentIt.skip(name, fn); | ||
| } finally { | ||
| suitePathStack.pop(); | ||
| } | ||
| }, | ||
| only: (name: string, fn: () => void) => { | ||
| suitePathStack.push(name); | ||
| try { | ||
| currentIt.only(name, fn); | ||
| } finally { | ||
| suitePathStack.pop(); | ||
| } | ||
| }, | ||
| todo: (name: string) => { | ||
| suitePathStack.push(name); | ||
| try { | ||
| currentIt.todo(name); | ||
| } finally { | ||
| suitePathStack.pop(); | ||
| } | ||
| }, | ||
| }, | ||
| ) as typeof global.it; |
There was a problem hiding this comment.
Remove suitePathStack manipulation from skip/only/todo methods.
The skip/only/todo implementations incorrectly push test names onto suitePathStack, which is designed to track suite (describe block) hierarchy, not individual test names. While the immediate push/pop minimizes runtime impact, this corrupts the conceptual model and adds confusing, unnecessary operations.
These methods should simply delegate to the original implementation without stack manipulation:
global.it = Object.assign(
(testName: any, fn?: any, timeout?: number) => {
if (typeof testName === 'string' && fn) {
return currentIt(
testName,
async () => {
qase.suite(currentPath);
return fn();
},
timeout,
);
}
return currentIt(testName, fn, timeout);
},
{
- skip: (name: string, fn: () => void) => {
- suitePathStack.push(name);
- try {
- currentIt.skip(name, fn);
- } finally {
- suitePathStack.pop();
- }
- },
+ skip: currentIt.skip.bind(currentIt),
- only: (name: string, fn: () => void) => {
- suitePathStack.push(name);
- try {
- currentIt.only(name, fn);
- } finally {
- suitePathStack.pop();
- }
- },
+ only: currentIt.only.bind(currentIt),
- todo: (name: string) => {
- suitePathStack.push(name);
- try {
- currentIt.todo(name);
- } finally {
- suitePathStack.pop();
- }
- },
+ todo: currentIt.todo.bind(currentIt),
},
) as typeof global.it;🤖 Prompt for AI Agents
In ee/packages/federation-matrix/tests/setup-qase.ts around lines 54 to 98, the
skip/only/todo methods currently push and pop test names on suitePathStack
(which tracks suite/describe hierarchy) — remove that manipulation and have
these methods simply delegate directly to the original currentIt
implementations; replace the bodies so skip calls currentIt.skip(name, fn), only
calls currentIt.only(name, fn), and todo calls currentIt.todo(name) with no
push/pop or try/finally around them.
| try { | ||
| fn(); | ||
| } catch (error) { | ||
| console.error('Error in describe block:', error); | ||
| } |
There was a problem hiding this comment.
Re-throw errors after logging to avoid swallowing exceptions.
The try-catch block logs errors from the describe block but doesn't re-throw them, which swallows exceptions and could hide real problems during test setup. Jest needs to see these errors to properly fail the test suite.
try {
fn();
} catch (error) {
console.error('Error in describe block:', error);
+ throw error;
}📝 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.
| try { | |
| fn(); | |
| } catch (error) { | |
| console.error('Error in describe block:', error); | |
| } | |
| try { | |
| fn(); | |
| } catch (error) { | |
| console.error('Error in describe block:', error); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/tests/setup-qase.ts around lines 146-150 the
try-catch around fn() logs errors but does not re-throw them, which swallows
exceptions; update the catch block to re-throw the caught error after logging
(e.g., console.error(...); throw error;) so Jest receives the failure and the
test suite can fail appropriately.
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
test:federation, updated integration test invocation to use the new command and environment setup, and exposed PR number to test runs.✏️ Tip: You can customize this high-level summary in your review settings.