-
Notifications
You must be signed in to change notification settings - Fork 13k
test: message media tests #37528
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.
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
test: message media tests #37528
Conversation
…nd pre-built federation integration tests
…issue with temp build dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
ee/packages/federation-matrix/tests/setup-qase.ts (4)
54-70: Consider proper typing for test function parameters.The parameters are typed as
any, which bypasses TypeScript's type safety. Additionally, the wrapper converts all test functions to async, which changes the execution semantics for synchronous tests.Apply this diff to improve type safety:
- global.it = ((testName: any, fn?: any, timeout?: number) => { + global.it = ((testName: string | Function, fn?: jest.ProvidesCallback, 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(); + return fn.call(this); }, timeout, ); } - // Handle cases where testName might be a number or other type return currentIt(testName, fn, timeout); }) as typeof global.it;
73-87: Consider proper typing for test function parameters.Same issue as with
it()- the parameters useanyand the wrapper forces async execution.Apply this diff:
- global.test = ((testName: any, fn?: any, timeout?: number) => { + global.test = ((testName: string | Function, fn?: jest.ProvidesCallback, timeout?: number) => { if (typeof testName === 'string' && fn) { return currentTest( 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(); + return fn.call(this); }, timeout, ); } return currentTest(testName, fn, timeout); }) as typeof global.test;
42-95: Add error handling to ensure global state is restored.If
fn()throws an exception, the originalitandtestfunctions won't be restored, potentially affecting subsequent describe blocks.Apply this diff:
originalDescribe(name, () => { // Add beforeEach to set suite for all tests in this describe block // This must be called before the test runs so the reporter picks it up global.beforeEach(() => { qase.suite(currentPath); }); // Store current it and test wrappers (they might be wrapped by parent describe) const currentIt = global.it; const currentTest = global.test; // Wrap it() to automatically set suite at the very start global.it = ((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); }) as typeof global.it; // Wrap test() to automatically set suite at the very start global.test = ((testName: any, fn?: any, timeout?: number) => { if (typeof testName === 'string' && fn) { return currentTest( 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, ); } return currentTest(testName, fn, timeout); }) as typeof global.test; - // Execute the describe block - fn(); - - // Restore previous wrappers - global.it = currentIt; - global.test = currentTest; + try { + // Execute the describe block + fn(); + } finally { + // Restore previous wrappers + global.it = currentIt; + global.test = currentTest; + } });
45-47: Redundant qase.suite() calls.The suite is set in
beforeEach(lines 45-47) and then set again at the start of each test wrapper (lines 61, 79). This appears redundant - ifbeforeEachalready sets the suite before each test, the additional calls inside the test wrappers may be unnecessary.Consider removing either the
beforeEachhook or the calls inside the test wrappers, depending on which approach is more reliable for the Qase reporter.ee/packages/federation-matrix/jest.config.federation.ts (1)
28-46: Consider extracting reporter configuration for better maintainability.The conditional spreading with type assertions works but is somewhat fragile. If the jest-qase-reporter API changes, the type assertion at line 43 won't catch it.
Consider extracting the reporter configuration:
const qaseReporter = process.env.QASE_TESTOPS_JEST_API_TOKEN ? [ 'jest-qase-reporter', { mode: 'testops', testops: { api: { token: process.env.QASE_TESTOPS_JEST_API_TOKEN }, project: 'RC', run: { complete: true }, }, debug: true, }, ] as const : null; const reporters: Config['reporters'] = ['default', ...(qaseReporter ? [qaseReporter] : [])];This separates concerns and makes the configuration easier to read and maintain.
📜 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 (6)
.github/workflows/ci.yml(1 hunks)ee/packages/federation-matrix/jest.config.federation.ts(1 hunks)ee/packages/federation-matrix/tests/end-to-end/room.spec.ts(0 hunks)ee/packages/federation-matrix/tests/helper/ddp-listener.ts(0 hunks)ee/packages/federation-matrix/tests/setup-qase.ts(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (2)
- ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
- ee/packages/federation-matrix/tests/helper/ddp-listener.ts
🧰 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/tests/setup-qase.tsee/packages/federation-matrix/jest.config.federation.ts
🧠 Learnings (9)
📚 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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/federation-matrix/tests/setup-qase.tsee/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 : Maintain test isolation between test cases 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 : Ensure tests run reliably in parallel without shared state conflicts
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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior 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 : Ensure clean state for each test execution 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:
package.json
⏰ 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 (3)
.github/workflows/ci.yml (1)
657-657: LGTM!The environment variable is correctly sourced from secrets for the Qase test reporter integration.
ee/packages/federation-matrix/jest.config.federation.ts (1)
24-25: LGTM!The setup file is correctly configured to load before tests run.
package.json (1)
24-24: jest-qase-reporter@^2.1.3 is the current latest version with no known security vulnerabilities.Verification confirms that version ^2.1.3 specified in package.json is up-to-date and no security advisories exist for this package in GitHub's vulnerability database.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
123-314: Complexity rule suppression is fine for now, but consider decomposingcreateRoomover timeThe added
// eslint-disable-next-line complexityavoids churn in a heavily used function and doesn’t affect runtime behavior, which is acceptable for this PR. That said, the size and branching ofcreateRoom(validation, federation checks, callbacks, DB writes, app events, etc.) justify the lint warning: long‑term maintainability would benefit from extracting dedicated helpers (e.g., validation, federation gating, app‑events orchestration, channel/private‑group post‑hooks) instead of relying on a permanent suppression.
📜 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 (4)
.github/workflows/ci.yml(1 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)ee/packages/federation-matrix/tests/end-to-end/room.spec.ts(0 hunks)ee/packages/federation-matrix/tests/setup-qase.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- ee/packages/federation-matrix/tests/setup-qase.ts
🧰 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:
apps/meteor/app/lib/server/functions/createRoom.ts
🧠 Learnings (2)
📓 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: 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
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
📚 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
⏰ 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
Proposed changes (including videos or screenshots)
Media messaging integration tests
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.