test(apps): add tests for ModifyDeleter accessor#39796
Open
tanayyo1 wants to merge 4 commits intoRocketChat:developfrom
Open
test(apps): add tests for ModifyDeleter accessor#39796tanayyo1 wants to merge 4 commits intoRocketChat:developfrom
tanayyo1 wants to merge 4 commits intoRocketChat:developfrom
Conversation
Fixes RocketChat#39795 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
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:
WalkthroughAdded a new test suite for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing Alsatian unit tests for the ModifyDeleter accessor in packages/apps-engine, aligning its coverage with the other modify accessors and validating its bridge delegation and 50-user guard behavior.
Changes:
- Introduces a new
ModifyDeleteraccessor spec verifying delegation fordeleteRoom,deleteMessage, anddeleteUsers. - Adds coverage for
removeUsersFromRoomfor normal usage, the 50-user boundary, and exceeding-limit error behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+124
| const tooManyUsers = Array.from({ length: 51 }, (_, i) => `user${i}`); | ||
|
|
||
| await Expect(async () => md.removeUsersFromRoom('room-id', tooManyUsers)).toThrowAsync(); |
There was a problem hiding this comment.
The "exceeding 50-user limit" test currently only asserts that some error is thrown. To ensure this specifically covers the guard in ModifyDeleter.removeUsersFromRoom (and not an unrelated failure), assert the thrown error type/message (e.g. Error with the exact message) and consider also spying on doRemoveUsers to verify it is not invoked when the limit is exceeded.
Suggested change
| const tooManyUsers = Array.from({ length: 51 }, (_, i) => `user${i}`); | |
| await Expect(async () => md.removeUsersFromRoom('room-id', tooManyUsers)).toThrowAsync(); | |
| const spRemove = SpyOn(this.mockRoomBridge, 'doRemoveUsers'); | |
| const tooManyUsers = Array.from({ length: 51 }, (_, i) => `user${i}`); | |
| await Expect(async () => md.removeUsersFromRoom('room-id', tooManyUsers)).toThrowAsync(Error); | |
| Expect(this.mockRoomBridge.doRemoveUsers).not.toHaveBeenCalled(); | |
| spRemove.restore(); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Member
|
Hi, we're migrating away from the Alsatian framework for tests here #37592 So we won't be accepting files that add new Alsatian tests |
Author
|
got it, will convert to node:test |
1 similar comment
Author
|
got it, will convert to node:test |
tanayyo1
force-pushed
the
test/add-modify-deleter-tests
branch
from
a9c0021 to
f0a3140
Compare
tanayyo1
force-pushed
the
test/add-modify-deleter-tests
branch
from
f0a3140 to
22c494e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts (1)
88-90: Add explicit call-count assertion for the seconddeleteUsersinvocationOn Line 88 and Line 89, assert total calls are exactly
2after the second invocation to avoid false positives if extra calls are introduced.Suggested patch
await md.deleteUsers('app-id', UserType.BOT); + assert.strictEqual(spy.mock.calls.length, 2); assert.deepStrictEqual(spy.mock.calls[1].arguments, ['app-id', UserType.BOT]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts` around lines 88 - 90, After calling md.deleteUsers('app-id', UserType.BOT) a second time, add an explicit assertion that the spy was called exactly twice (e.g., assert.strictEqual(spy.mock.calls.length, 2) or equivalent) before asserting the arguments for the second call; this ensures no extra unexpected calls are present when checking spy.mock.calls[1].arguments for the deleteUsers invocation in ModifyDeleter.test.ts.
🤖 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/server/accessors/ModifyDeleter.test.ts`:
- Around line 88-90: After calling md.deleteUsers('app-id', UserType.BOT) a
second time, add an explicit assertion that the spy was called exactly twice
(e.g., assert.strictEqual(spy.mock.calls.length, 2) or equivalent) before
asserting the arguments for the second call; this ensures no extra unexpected
calls are present when checking spy.mock.calls[1].arguments for the deleteUsers
invocation in ModifyDeleter.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e11512d0-0ed9-4f71-a7d6-fed5eab22cf7
📒 Files selected for processing (1)
packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts
📜 Review details
🧰 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/server/accessors/ModifyDeleter.test.ts
🧠 Learnings (13)
📓 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: 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: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
📚 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/server/accessors/ModifyDeleter.test.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
packages/apps-engine/tests/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.test.ts
📚 Learning: 2026-03-19T14:53:45.726Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/tests/test-data/storage/storage.ts:13-23
Timestamp: 2026-03-19T14:53:45.726Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the `TestsAppStorage` singleton pattern (via `TestsAppStorage.getInstance()`) is intentional. The maintainer (d-gubert) has confirmed that potential cross-test state leakage from the process-wide singleton is not a concern for this test suite. Do not flag this as a state-leakage or flaky-test risk in future reviews.
Applied to files:
packages/apps-engine/tests/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.test.ts
🔇 Additional comments (1)
packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts (1)
56-125: Good coverage and migration directionThis suite cleanly validates bridge delegation and the 50-user guard, and it uses
node:test/node:assertinstead of Alsatian as requested.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39796 +/- ##
===========================================
- Coverage 70.61% 70.58% -0.03%
===========================================
Files 3257 3257
Lines 115789 115789
Branches 21022 21037 +15
===========================================
- Hits 81759 81733 -26
- Misses 31968 31996 +28
+ Partials 2062 2060 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Author
|
Converted to node:test — renamed .spec.ts to .test.ts, switched from Alsatian decorators to describe/it/assert from node:test and node:assert. Ready for re-review. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts (1)
37-37: TightendeleteUsersmock parameter typing to match production contract.Use
UserType.BOT | UserType.APPinstead ofUserTypein the mock signature to keep compile-time checks aligned withUserBridge.doDeleteUsersCreatedByApp.♻️ Proposed typing refinement
- doDeleteUsersCreatedByApp(appId: string, userType: UserType): Promise<boolean> { + doDeleteUsersCreatedByApp(appId: string, userType: UserType.BOT | UserType.APP): Promise<boolean> { return Promise.resolve(true); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts` at line 37, The mock for deleteUsers currently types its parameter as UserType (too broad); update the mock signature to accept only UserType.BOT | UserType.APP to match the production contract used by UserBridge.doDeleteUsersCreatedByApp. Find the mock implementation named deleteUsers in the test and change the parameter type to the union (UserType.BOT | UserType.APP) so TypeScript enforces the same constraints as the real doDeleteUsersCreatedByApp.
🤖 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/server/accessors/ModifyDeleter.test.ts`:
- Line 37: The mock for deleteUsers currently types its parameter as UserType
(too broad); update the mock signature to accept only UserType.BOT |
UserType.APP to match the production contract used by
UserBridge.doDeleteUsersCreatedByApp. Find the mock implementation named
deleteUsers in the test and change the parameter type to the union (UserType.BOT
| UserType.APP) so TypeScript enforces the same constraints as the real
doDeleteUsersCreatedByApp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98408c09-c840-46f1-91a5-12fe4eb859b9
📒 Files selected for processing (1)
packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts
📜 Review details
🧰 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/server/accessors/ModifyDeleter.test.ts
🧠 Learnings (10)
📓 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: 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.
📚 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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.test.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
packages/apps-engine/tests/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.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/server/accessors/ModifyDeleter.test.ts
🔇 Additional comments (3)
packages/apps-engine/tests/server/accessors/ModifyDeleter.test.ts (3)
18-54: Solid test isolation and cleanup setup.
beforeEachreinitialization plusafterEachmock.restoreAll()keeps cases isolated and avoids leaked spies across tests.
56-91: Good delegation verification for delete operations.These cases correctly assert both bridge delegation arguments and call counts, including the second
deleteUsersinvocation path.
93-126: Boundary and guard coverage forremoveUsersFromRoomis complete.You covered normal, exact-limit (50), and over-limit (51) scenarios, and also assert no bridge call on rejection. Great regression protection for the limit check.
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.
Fixes #39795
adds test coverage for
ModifyDeleterinpackages/apps-engine/src/server/accessors/. covers all 4 methods:deleteRoom— verifies bridge delegationdeleteMessage— verifies bridge delegationdeleteUsers— verifies bridge delegation for bothAPPandBOTtypes, checks return valueremoveUsersFromRoom— verifies normal case, boundary (exactly 50 users), and throws when exceeding 50-user limitfollows the same Alsatian pattern used by the existing accessor tests (ModifyCreator, ModifyUpdater, ModifyExtender).
Summary by CodeRabbit