chore: convert unit tests from Alsatian to NodeJS built-in framework#37592
Merged
dionisio-bot[bot] merged 21 commits intodevelopfrom Mar 25, 2026
Merged
chore: convert unit tests from Alsatian to NodeJS built-in framework#37592dionisio-bot[bot] merged 21 commits intodevelopfrom
dionisio-bot[bot] merged 21 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces legacy Alsatian .spec tests with new Node.js/TypeScript test suites under tests/node-tests, removes the legacy test runner, updates package.json test scripts, fixes test-data import paths, adds many node test files, deletes corresponding Alsatian specs, and makes a small async change to SlashCommandsExtend.provideSlashCommand. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes |
d-gubert
force-pushed
the
chore/apps-engine-tests-conversion-start
branch
3 times, most recently
from
a26e94a to
65704b0
Compare
Contributor
📦 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 01:06 (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.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]
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]
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.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]
Statistics (last 12 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 04 Dec 2025 01:06:16 GMT |
d-gubert
force-pushed
the
chore/apps-engine-tests-conversion-start
branch
from
65704b0 to
efab48a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37592 +/- ##
===========================================
- Coverage 70.61% 70.59% -0.02%
===========================================
Files 3256 3256
Lines 115778 115778
Branches 21042 21014 -28
===========================================
- Hits 81756 81733 -23
- Misses 31965 31974 +9
- Partials 2057 2071 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
force-pushed
the
chore/apps-engine-tests-conversion-start
branch
3 times, most recently
from
5ac05ef to
ccfc97d
Compare
d-gubert
force-pushed
the
chore/apps-engine-tests-conversion-start
branch
from
ccfc97d to
096c003
Compare
Member
Author
|
@copilot following the pattern in this PR, convert the file Avoid modifying any other files. If there are any suggestions you find relevant, document those in the PR description. |
Member
Author
|
@copilot review this pr |
d-gubert
force-pushed
the
chore/apps-engine-tests-conversion-start
branch
from
91eda49 to
e76a09d
Compare
Member
Author
|
@copilot following the pattern in this PR, convert the file packages/apps-engine/tests/server/managers/AppAccessorManager.spec.ts to a new test using NodeJS built-in test framework. Avoid modifying any other files. If there are any suggestions you find relevant, document those in the PR description. |
Contributor
Member
Author
|
@copilot select another test file inside packages/apps-engine/tests/server/*, and convert it to use the NodeJS builtin framework, following the pattern in this PR. Be sure to select only one test file to convert, and avoid modifying files outside the packages/apps-engine/tests/node-tests directory. Double check test hooks and implementation so that the new test file tests for the same conditions as the source Alsatian test file. |
Contributor
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: d-gubert <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts (1)
283-283: Replace explicitanytypes with concrete types in test cases and mock callbacks.The
packages/apps-enginetsconfig enforcesnoImplicitAny: true, so explicitanytypes should be replaced with specific types to maintain type safety and avoid masking regressions in argument shapes.♻️ Proposed refactor
+ type CustomizeUrlCase = { + name: string; + call: ReturnType<typeof TestData.getVideoConfDataExtended>; + user: ReturnType<typeof TestData.getVideoConferenceUser> | undefined; + options: Record<string, never>; + runCustomizeUrl: string; + result: string; + }; - const cases: any = [ + const cases: CustomizeUrlCase[] = [ { name: 'test', call, user, options: {}, runCustomizeUrl: 'test/first-call#caller', result: 'test/first-call#caller', }, @@ - mock.method(testProvider, 'runCustomizeUrl', (_call: any, user: any) => user ? 'test/first-call#caller' : 'test/first-call#'); - mock.method(test2Provider, 'runCustomizeUrl', (_call: any, user: any) => user ? 'test2/first-call#caller' : 'test2/first-call#'); - mock.method(differentProvider, 'runCustomizeUrl', (_call: any, user: any) => user ? 'differentProvider/first-call#caller' : 'differentProvider/first-call#'); + mock.method(testProvider, 'runCustomizeUrl', (_call, currentUser) => currentUser ? 'test/first-call#caller' : 'test/first-call#'); + mock.method(test2Provider, 'runCustomizeUrl', (_call, currentUser) => currentUser ? 'test2/first-call#caller' : 'test2/first-call#'); + mock.method(differentProvider, 'runCustomizeUrl', (_call, currentUser) => currentUser ? 'differentProvider/first-call#caller' : 'differentProvider/first-call#');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts` at line 283, The tests declare const cases: any and use untyped mock callbacks; replace the explicit any by declaring a concrete test-case type describing the shape used in the tests (e.g. an interface with fields like name/description, input, expected and the callback signature) and annotate cases with Array<ThatType>; update any mock callback declarations to use the precise function type (or import the relevant types from the module under test) so the compiler can check argument/return shapes for functions like the mock provider callbacks and the entries in cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts`:
- Around line 45-47: Change the getOneById stub signature to return ProxiedApp |
undefined (allow undefined) and update its implementation so it returns a
ProxiedApp whose id matches the requested appId instead of always returning the
same mockApp; e.g., when appId === 'failMePlease' return undefined, otherwise
clone or create a new mock app/proxy with id = appId (so tests referencing
'firstApp' and 'secondApp' exercise distinct identities). Ensure any helper or
mockApp factory used by tests (the object referenced as app/mockApp) supports
supplying the id parameter.
---
Nitpick comments:
In
`@packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts`:
- Line 283: The tests declare const cases: any and use untyped mock callbacks;
replace the explicit any by declaring a concrete test-case type describing the
shape used in the tests (e.g. an interface with fields like name/description,
input, expected and the callback signature) and annotate cases with
Array<ThatType>; update any mock callback declarations to use the precise
function type (or import the relevant types from the module under test) so the
compiler can check argument/return shapes for functions like the mock provider
callbacks and the entries in cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b7b1021-4ffe-4228-9b19-015e3518e9a2
📒 Files selected for processing (8)
packages/apps-engine/package.jsonpackages/apps-engine/src/server/accessors/SlashCommandsExtend.tspackages/apps-engine/tests/node-tests/server/accessors/ConfigurationModify.test.tspackages/apps-engine/tests/node-tests/server/accessors/ModifyUpdater.test.tspackages/apps-engine/tests/node-tests/server/accessors/SlashCommandsExtend.test.tspackages/apps-engine/tests/node-tests/server/logging/AppConsole.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/apps-engine/tests/node-tests/server/accessors/ConfigurationModify.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/ModifyUpdater.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/SlashCommandsExtend.test.ts
- packages/apps-engine/tests/node-tests/server/logging/AppConsole.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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:
packages/apps-engine/src/server/accessors/SlashCommandsExtend.tspackages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
🧠 Learnings (26)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
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
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
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
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
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.
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 : All test files must be created in `apps/meteor/tests/e2e/` directory
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
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
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
packages/apps-engine/src/server/accessors/SlashCommandsExtend.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps-engine/src/server/accessors/SlashCommandsExtend.tspackages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps-engine/src/server/accessors/SlashCommandsExtend.tspackages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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:
packages/apps-engine/package.jsonpackages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.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:
packages/apps-engine/package.jsonpackages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
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:
packages/apps-engine/package.jsonpackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
packages/apps-engine/package.jsonpackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2026-02-04T12:09:05.769Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
🔇 Additional comments (4)
packages/apps-engine/src/server/accessors/SlashCommandsExtend.ts (1)
11-12: Async conversion is correct and improves promise-consistent error handling.This keeps the
Promise<void>contract intact and ensuresaddCommandfailures are consistently surfaced as promise rejections for callers/tests.packages/apps-engine/tests/node-tests/server/managers/AppVideoConfProviderManager.test.ts (2)
161-170: Good duplicate-provider conflict coverage.This validates the global provider-name uniqueness behavior across apps and protects against cross-app collisions.
215-236: Nice multi-provider routing coverage for bothgenerateUrlandcustomizeUrl.The tests clearly exercise provider selection across apps and registered states.
Also applies to: 308-333
packages/apps-engine/package.json (1)
54-54: No action needed — migration is already complete.The
tests/serverdirectory no longer exists, and all files intests/node-testsare already named*.test.ts. The.:test:node-builtinrunner correctly targets the migrated test suite with no coverage gaps.> Likely an incorrect or invalid review comment.
packages/apps-engine/tests/server/managers/AppOutboundCommunicationProviderManager.test.ts
Show resolved
Hide resolved
Copilot AI
added a commit
that referenced
this pull request
Mar 17, 2026
Co-authored-by: d-gubert <[email protected]>
…test suite (#39695) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: d-gubert <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.ts (1)
89-100: Apply consistent null-guard pattern forgetExternalComponents()calls.Lines 96 and 99 access
.sizedirectly on the result ofgetExternalComponents()without a null guard. This is the same issue that was addressed at lines 73-75. For consistency and test robustness, consider guarding against null here as well.Suggested diff
it('verifyAddExternalComponent', () => { const aecm1 = new AppExternalComponentManager(); const component1 = mockExternalComponent1; const component3 = mockExternalComponent3; aecm1.addExternalComponent(component1.appId, component1); assert.strictEqual(aecm1.getAppTouchedExternalComponents().size, 1); - assert.strictEqual(aecm1.getExternalComponents(component1.appId).size, 1); + const componentsAfterFirst = aecm1.getExternalComponents(component1.appId); + assert.ok(componentsAfterFirst); + assert.strictEqual(componentsAfterFirst.size, 1); aecm1.addExternalComponent(component1.appId, component3); - assert.strictEqual(aecm1.getExternalComponents(component1.appId).size, 2); + const componentsAfterSecond = aecm1.getExternalComponents(component1.appId); + assert.ok(componentsAfterSecond); + assert.strictEqual(componentsAfterSecond.size, 2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.ts` around lines 89 - 100, The test verifyAddExternalComponent calls AppExternalComponentManager.getExternalComponents(...) and accesses .size without a null guard; update the test to capture the return value (e.g., const comps = aecm1.getExternalComponents(component1.appId)), assert it is not null/undefined (using assert.ok or assert.notStrictEqual(comps, null)), then assert.strictEqual(comps.size, ...). Apply this pattern for both places where getExternalComponents(...) is used in the test to match the earlier null-guard approach.packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.ts (1)
227-229: Re-instantiateLivenessManagerwith the required options instead of mutating private fields.Directly mutating
optionsviaas anybypasses the constructor's option-merging logic and couples the test to implementation details. Sincestop(),attach(), and the constructor are public and available in the test, instantiate a freshLivenessManagerwithconsecutiveTimeoutLimit: 2:Suggested refactor
it('should call restart when consecutive timeout count reaches limit from options', async () => { - (livenessManager as any).options.consecutiveTimeoutLimit = 2; + livenessManager.stop(); + livenessManager = new LivenessManager( + { + controller: mockController, + messenger: mockMessenger, + debug, + }, + { + pingTimeoutInMS: PING_TIMEOUT_MS, + pingIntervalInMS: PING_INTERVAL_MS, + consecutiveTimeoutLimit: 2, + maxRestarts: MAX_RESTARTS, + restartAttemptDelayInMS: RESTART_ATTEMPT_DELAY_MS, + }, + ); + livenessManager.attach(mockSubprocess); const restartSpy = mock.method(livenessManager as any, 'restartProcess', () => Promise.resolve());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.ts` around lines 227 - 229, The test currently mutates the private options on the existing LivenessManager instance; instead, construct a fresh LivenessManager with the desired option (consecutiveTimeoutLimit: 2) so constructor option-merging runs and tests don't rely on implementation internals—create a new instance of LivenessManager passing { consecutiveTimeoutLimit: 2 }, call stop() on the old instance if needed and attach() or otherwise prepare the new instance, and then install the mock on the new instance's restartProcess method (reference: LivenessManager, restartProcess, stop, attach).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.ts`:
- Around line 89-100: The test verifyAddExternalComponent calls
AppExternalComponentManager.getExternalComponents(...) and accesses .size
without a null guard; update the test to capture the return value (e.g., const
comps = aecm1.getExternalComponents(component1.appId)), assert it is not
null/undefined (using assert.ok or assert.notStrictEqual(comps, null)), then
assert.strictEqual(comps.size, ...). Apply this pattern for both places where
getExternalComponents(...) is used in the test to match the earlier null-guard
approach.
In
`@packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.ts`:
- Around line 227-229: The test currently mutates the private options on the
existing LivenessManager instance; instead, construct a fresh LivenessManager
with the desired option (consecutiveTimeoutLimit: 2) so constructor
option-merging runs and tests don't rely on implementation internals—create a
new instance of LivenessManager passing { consecutiveTimeoutLimit: 2 }, call
stop() on the old instance if needed and attach() or otherwise prepare the new
instance, and then install the mock on the new instance's restartProcess method
(reference: LivenessManager, restartProcess, stop, attach).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0731ff67-0e49-4102-926d-713d45bce3fa
📒 Files selected for processing (14)
packages/apps-engine/tests/node-tests/server/AppManager.test.tspackages/apps-engine/tests/node-tests/server/accessors/ConfigurationExtend.test.tspackages/apps-engine/tests/node-tests/server/accessors/MessageRead.test.tspackages/apps-engine/tests/node-tests/server/accessors/Notifier.test.tspackages/apps-engine/tests/node-tests/server/accessors/PersistenceRead.test.tspackages/apps-engine/tests/node-tests/server/accessors/ServerSettingRead.test.tspackages/apps-engine/tests/node-tests/server/accessors/UserRead.test.tspackages/apps-engine/tests/node-tests/server/compiler/AppFabricationFulfillment.test.tspackages/apps-engine/tests/node-tests/server/logging/AppConsole.test.tspackages/apps-engine/tests/node-tests/server/managers/AppApiManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/DenoRuntimeSubprocessController.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/apps-engine/tests/node-tests/server/accessors/UserRead.test.ts
- packages/apps-engine/tests/node-tests/server/AppManager.test.ts
- packages/apps-engine/tests/node-tests/server/logging/AppConsole.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/PersistenceRead.test.ts
- packages/apps-engine/tests/node-tests/server/runtime/DenoRuntimeSubprocessController.test.ts
- packages/apps-engine/tests/node-tests/server/compiler/AppFabricationFulfillment.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/ConfigurationExtend.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/MessageRead.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/ServerSettingRead.test.ts
- packages/apps-engine/tests/node-tests/server/managers/AppApiManager.test.ts
- packages/apps-engine/tests/node-tests/server/accessors/Notifier.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🧰 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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
🧠 Learnings (29)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
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 : Ensure tests run reliably in parallel without shared state conflicts
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
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 : All test files must be created in `apps/meteor/tests/e2e/` directory
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
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
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.
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
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 `.spec.ts` extension for test files (e.g., `login.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 : Group related tests in the same file
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.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} : Store commonly used locators in variables/constants for reuse
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.tspackages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
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:
packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.tspackages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-02T16:31:41.304Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.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:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-03-09T21:20:12.687Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39386
File: apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts:12-15
Timestamp: 2026-03-09T21:20:12.687Z
Learning: In `apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`, the early return `if (data.voipToken) return null` (Lines 13-15) is intentionally correct. VoIP token updates always include an `_id`, so they are handled by the `_id` lookup block above (Lines 5-9) and never reach this guard. The guard is only a safety net for edge cases where `_id` is absent or no document was found, preventing an incorrect `token + appName` fallback match for VoIP-only payloads.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts
🔇 Additional comments (9)
packages/apps-engine/tests/node-tests/server/managers/AppExternalComponentManager.test.ts (4)
1-38: LGTM!Imports, mock component definitions, and the helper function are well-structured. The mock objects correctly satisfy the required fields of
IExternalComponent, and theregisterhelper cleanly encapsulates the add-then-register pattern.
40-45: LGTM!Accessing private fields via
(aecm as any)is an acceptable pattern for verifying internal state in unit tests. Field names match the implementation per context snippet.
67-76: LGTM!The null guard pattern here is correct—asserting non-null before accessing
.sizeensures a clean test failure instead of a runtime TypeError.
102-135: LGTM!The remaining tests for
registerExternalComponents,unregisterExternalComponents, andpurgeExternalComponentscorrectly verify state transitions and cover the full lifecycle of external components.packages/apps-engine/tests/node-tests/server/managers/AppSettingsManager.test.ts (3)
30-98: LGTM!The test setup correctly initializes all required mocks. The
getOneByIdimplementation now properly returnsProxiedApp | undefinedvia type inference, addressing the previous review concern. The mock structure accurately reflects the dependencies required byAppSettingsManager.
104-122: LGTM!Well-structured test covering:
- Construction validation
getAppSettings/getAppSettingreturning cloned (but deeply equal) objects- Proper error handling for non-existent apps/settings
- Immutability enforcement via attempted property assignment
124-157: LGTM!The test thoroughly verifies
updateAppSettingbehavior:
- Error rejection for non-existent app/setting
AppMetadataStorage.updateSettingcalled with correct_idand setting objectdoOnAppSettingsChangebridge method invoked with correct argumentsAppMethod.ONSETTINGUPDATEDcalled on the app with the updated settingThe filtering of
callspy results byAppMethod.ONSETTINGUPDATEDis correct sinceON_PRE_SETTING_UPDATEis also invoked during the update flow.packages/apps-engine/tests/node-tests/server/runtime/deno/LivenessManager.test.ts (2)
3-3:node:testMock typing is correctly applied.Using
type MockwithMock<ProcessMessenger['send']>is the right fix and keeps this suite aligned with current typings.Also applies to: 26-26
121-224: Behavioral coverage for heartbeat/ping/timeout flows is strong.The cases exercise stale vs recent heartbeat, timeout accumulation/reset, and restart triggering with concrete assertions tied to runtime data.
Also applies to: 230-270
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps-engine/package.json (1)
108-108: Consider removing unusedtap-barkdependency.
tap-barkwas used as a TAP output reporter with Alsatian, which has been removed. The new test script uses--test-reporter specwith Node's built-in test runner, making this dependency unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/package.json` at line 108, The package.json still lists the unused dependency "tap-bark"; remove the "tap-bark": "^1.0.0" entry from dependencies (or devDependencies) so the package no longer depends on the obsolete TAP reporter—update the package.json to delete the "tap-bark" key and run npm/yarn/pnpm install to refresh lockfiles accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/apps-engine/package.json`:
- Line 108: The package.json still lists the unused dependency "tap-bark";
remove the "tap-bark": "^1.0.0" entry from dependencies (or devDependencies) so
the package no longer depends on the obsolete TAP reporter—update the
package.json to delete the "tap-bark" key and run npm/yarn/pnpm install to
refresh lockfiles accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e49d4cc-9023-429c-9d0a-25bbf5b2202e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
packages/apps-engine/package.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
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
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
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 : All test files must be created in `apps/meteor/tests/e2e/` directory
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
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: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
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.
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
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
📚 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:
packages/apps-engine/package.json
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
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:
packages/apps-engine/package.json
📚 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:
packages/apps-engine/package.json
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
packages/apps-engine/package.json
📚 Learning: 2026-03-06T18:02:20.381Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/RelativeTime.spec.tsx:63-70
Timestamp: 2026-03-06T18:02:20.381Z
Learning: In RocketChat/Rocket.Chat, tests in the `packages/gazzodown` package (and likely the broader test suite) are always expected to run in the UTC timezone. This makes `toLocaleString()` output deterministic, so snapshot tests that include locale/timezone-sensitive content (such as `title` attributes from `toLocaleString()`) are stable and do not need to be replaced with targeted assertions.
Applied to files:
packages/apps-engine/package.json
📚 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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
packages/apps-engine/package.json
📚 Learning: 2026-03-17T16:08:37.572Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39690
File: packages/ui-voip/package.json:11-11
Timestamp: 2026-03-17T16:08:37.572Z
Learning: In `packages/ui-voip/package.json` (RocketChat/Rocket.Chat), the team deliberately chose to use `rm -rf dist` directly in the `"build"` script instead of `rimraf`, as they decided against introducing the `rimraf` dependency for this package. Do not flag `rm -rf dist` in the ui-voip build script as a cross-platform issue requiring rimraf.
Applied to files:
packages/apps-engine/package.json
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
packages/apps-engine/package.json
📚 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:
packages/apps-engine/package.json
📚 Learning: 2026-02-24T19:22:52.984Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:52.984Z
Learning: In RocketChat/Rocket.Chat Playwright e2e tests, prefer using translated text and ARIA roles (getByRole, getByText, etc.) over data-qa locators. If translation values change, update the corresponding test locators accordingly. Never prefer data-qa locators.
Applied to files:
packages/apps-engine/package.json
🔇 Additional comments (2)
packages/apps-engine/package.json (2)
63-63: LGTM!The
testunitscript correctly references the new.:test:node-builtinscript while maintaining parallel execution with Deno tests.
54-54: No action needed. The ts-node version 6.2.0 is compatible with TypeScript 5.9.3 (ts-node 6.2.0 supports TypeScript >=2.0 as a peer dependency). While the version is old and newer releases (10.9.x) may offer bug fixes and performance improvements, there are no compatibility blockers preventing the test script from running as configured.> Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 66 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/apps-engine/package.json">
<violation number="1" location="packages/apps-engine/package.json:54">
P1: Renaming this script breaks `testunit`, which still calls `.:test:node-builtin`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
d-gubert
force-pushed
the
chore/apps-engine-tests-conversion-start
branch
from
c0bcd33 to
15cab5c
Compare
This was referenced Mar 23, 2026
This was referenced Mar 25, 2026
d-gubert
added
the
stat: QA assured
ggazzo
approved these changes
Mar 25, 2026
dionisio-bot
bot
added
the
stat: ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Converting all tests written for Alsatian to new tests written using NodeJS built-in test framework.
Conversion largely done by Copilot, with me doing the reviewing and steering
Issue(s)
CORE-1266
Steps to test or reproduce
Further comments
Summary by CodeRabbit