-
Notifications
You must be signed in to change notification settings - Fork 297
test(WPB-19964): add regression tests for message reactions #20232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #20232 +/- ##
=======================================
Coverage 45.18% 45.18%
=======================================
Files 1632 1632
Lines 40166 40166
Branches 8301 8301
=======================================
Hits 18150 18150
Misses 20090 20090
Partials 1926 1926
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
specs/AppLock/AppLock.spec.ts (❌ 1 failed,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds Playwright E2E regression coverage for message reactions across different message/asset types and interaction modes, plus a small enhancement to the conversation page object to support these tests.
Changes:
- Introduces
Reactions/reactions.spec.tswith regression tests for adding/removing reactions on links, media assets, files, locations, own messages, and system messages. - Verifies reaction UX details such as reaction pill counts,
aria-pressedstate, and reaction tooltips from multiple users’ perspectives. - Extends the
ConversationPagepage object with a dedicated heart-reaction locator and refinesreactOnMessageto map reaction types to the appropriate UI controls more precisely.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/webapp/test/e2e_tests/specs/Reactions/reactions.spec.ts | Adds a suite of Playwright regression tests covering heart/thumbs-up reactions on various message types, reaction lists, toggling via pills and emoji picker, and ensuring system messages remain non-reactable. |
| apps/webapp/test/e2e_tests/pageManager/webapp/pages/conversation.page.ts | Updates the conversation page object with a reusable heart reaction pill locator and tightens reactOnMessage logic so each reaction type triggers only its intended UI control. |
471e4a5 to
f18ad32
Compare
apps/webapp/test/e2e_tests/pageManager/webapp/pages/conversation.page.ts
Outdated
Show resolved
Hide resolved
f18ad32 to
946782b
Compare
946782b to
839c8af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
| const tooltip = userAPages | ||
| .conversation() | ||
| .page.getByRole('tooltip') | ||
| .filter({hasText: `${userA.fullName} reacted with`}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filters tooltips using the full English text ${userA.fullName} reacted with which can be localized or have different punctuation/format, this makes the assertion brittle.
Can we assert presence of the username inside the tooltip, not an exact English phrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it's totally fine to use english text within locators, it's done all the time. We're not going to re-test the whole application for each supported language, so english is the default language set for all tests. (Except e.g. a test which might test switching the locale but that's an edge case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Mark. What do you think, Arjita?
| const editedMessage = userAPages.conversation().getMessage({content: 'Edited Message'}); | ||
| await expect(editedMessage).toBeVisible(); | ||
|
|
||
| await expect(reaction).not.toBeVisible(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing an element handle for the reaction before the message is edited and later checking that same handle is not visible can be causing stale element problems. The DOM containing that element may be replaced during edit.
// Re-query the reaction on the edited message to avoid stale element handle
await expect(userAPages.conversation().getReactionOnMessage(editedMessage, 'heart')).not.toBeVisible();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an element handle, it's a playwright locator. Locators in playwright are re-querried every time an assertion or action is triggered on them. (See: https://playwright.dev/docs/locators#locating-elements)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
| this.systemMessages = page.locator( | ||
| `${selectByDataAttribute('item-message')}${selectByClass('system-message')} ${selectByClass('message-header')}`, | ||
| ); | ||
| this.systemMessages = page.locator(`${selectByDataAttribute('item-message')}${selectByClass('system-message')}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you removing ${selectByClass('message-header')}? Now you’re targeting the whole system message container (item-message.system-message) any reason to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To align this locator with the other message-locators. I added now the selector via the attribute send status to make it clearer.
839c8af to
621fbf8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
This commit introduces a new test suite `reactions.spec.ts` to verify message reaction functionality. Implemented scenarios include: - Reacting to various content types: link previews, pictures, audio, video, shared files, and locations (@TC-1527 - @TC-1533). - Reacting to own messages and partner messages in 1:1 chats. - Verifying that system messages cannot be reacted to (@TC-1535). - Verifying reactions are reset when a message is edited (@TC-1538). - Validating reaction tooltips (@TC-1540). - Adding/removing reactions via pills and the emoji picker (@TC-1548, @TC-1549). The `ConversationPage` page object has been updated to support these interactions, specifically: - Added `reactionWithHeartEmoji` locator. - Refactored `reactOnMessage`. Refs: WPB-19964
This commit reactors the reactions regression tests according to review comments. Refs: WPB-19964
This commit reactors the reactions regression tests according to review comments. Refs: WPB-19964
This commit reactors the reactions regression tests according to review comments. Refs: WPB-19964
1a57c82 to
b8c8fc5
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.



https://wearezeta.atlassian.net/browse/WPB-19964
Pull Request
Summary
Add regression tests for reactions on messages and assets
Security Checklist (required)
Accessibility (required)
Standards Acknowledgement (required)
Note for reviewers
Code duplication cannot be reduced any further because it only refers to signing in users.