test: add unit tests for message sending validation#41565
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughAdds a Mocha/Chai unit test suite covering ChangesMessage preparation tests
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/lib/messages/sendMessage.spec.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed message fixtures instead of
any.Use
Partial<IMessage>or a repository-appropriate test fixture type so these tests verify the actual TypeScript contract rather than disabling type checking.As per coding guidelines, TypeScript tests should use accurate typing.
Also applies to: 16-16, 22-22, 28-28, 37-37, 44-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/lib/messages/sendMessage.spec.ts` at line 9, Replace the any-typed message fixtures in the sendMessage tests with Partial<IMessage> or the repository’s established message fixture type. Update each affected declaration while preserving the existing fixture fields and test behavior, so the tests continue validating the TypeScript contract.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/lib/messages/sendMessage.spec.ts`:
- Around line 36-41: Resolve the mismatch between the test and
prepareMessageObject’s existing contract: either add missing-username validation
in prepareMessageObject/sendMessage.ts that throws error-invalid-user, or update
the test to assert the current behavior instead of expecting an exception. Keep
the test and implementation consistent.
---
Nitpick comments:
In `@apps/meteor/server/lib/messages/sendMessage.spec.ts`:
- Line 9: Replace the any-typed message fixtures in the sendMessage tests with
Partial<IMessage> or the repository’s established message fixture type. Update
each affected declaration while preserving the existing fixture fields and test
behavior, so the tests continue validating the TypeScript contract.
🪄 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 Plus
Run ID: 3bb42472-4a42-464f-9079-21ab8c2e6111
📒 Files selected for processing (1)
apps/meteor/server/lib/messages/sendMessage.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/lib/messages/sendMessage.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/server/lib/messages/sendMessage.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-02-24T19:22:48.358Z
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:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/server/lib/messages/sendMessage.spec.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:
apps/meteor/server/lib/messages/sendMessage.spec.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:
apps/meteor/server/lib/messages/sendMessage.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/server/lib/messages/sendMessage.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/lib/messages/sendMessage.spec.ts
| it('should throw an error if username is not provided', () => { | ||
| const message: any = {}; | ||
| expect(() => { | ||
| prepareMessageObject(message, 'rid123', { _id: 'u123', name: 'Test User' }); | ||
| }).to.throw('error-invalid-user'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
This test contradicts prepareMessageObject and will fail.
The supplied implementation does not validate or throw for a missing username. Either add the intended validation in sendMessage.ts, or change this test to assert the existing contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/server/lib/messages/sendMessage.spec.ts` around lines 36 - 41,
Resolve the mismatch between the test and prepareMessageObject’s existing
contract: either add missing-username validation in
prepareMessageObject/sendMessage.ts that throws error-invalid-user, or update
the test to assert the current behavior instead of expecting an exception. Keep
the test and implementation consistent.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
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="apps/meteor/server/lib/messages/sendMessage.spec.ts">
<violation number="1" location="apps/meteor/server/lib/messages/sendMessage.spec.ts:36">
P0: Test 'should throw an error if username is not provided' asserts that `prepareMessageObject` throws `'error-invalid-user'` when username is omitted from the user object, but the implementation never throws — it simply sets `message.u.username = username as string` (which becomes `undefined`). This test will fail at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| expect(message.u.name).to.equal('Test User'); | ||
| }); | ||
|
|
||
| it('should throw an error if username is not provided', () => { |
There was a problem hiding this comment.
P0: Test 'should throw an error if username is not provided' asserts that prepareMessageObject throws 'error-invalid-user' when username is omitted from the user object, but the implementation never throws — it simply sets message.u.username = username as string (which becomes undefined). This test will fail at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/messages/sendMessage.spec.ts, line 36:
<comment>Test 'should throw an error if username is not provided' asserts that `prepareMessageObject` throws `'error-invalid-user'` when username is omitted from the user object, but the implementation never throws — it simply sets `message.u.username = username as string` (which becomes `undefined`). This test will fail at runtime.</comment>
<file context>
@@ -0,0 +1,49 @@
+ expect(message.u.name).to.equal('Test User');
+ });
+
+ it('should throw an error if username is not provided', () => {
+ const message: any = {};
+ expect(() => {
</file context>
2 participants
Summary
This PR adds comprehensive unit tests for the
prepareMessageObjectlogic withinsendMessage.tsto ensure robust message validation.What Changed
apps/meteor/server/lib/messages/sendMessage.spec.tststimestamp generation, property preservation,ridassignment, and user detail assignment.error-invalid-userexception is thrown when a username is missing.tshowproperty behavior.Why
Adding these unit tests ensures that critical message dispatch logic (and previous validation fixes) will not regress in future updates.
Testing
Checklist
Summary by CodeRabbit