Development: Fix course messages e2e tests
#9536
Conversation
WalkthroughThe pull request includes modifications to the test suite for course messages in Playwright, specifically updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/test/playwright/support/pageobjects/course/CourseMessagesPage.ts (3)
20-23: LGTM with a minor suggestion.The changes to
createChannelButtonmethod look good and align with the PR objectives. The new selectors seem more robust to potential UI changes.Consider adding a short wait between the two click actions to ensure the UI has time to respond:
async createChannelButton() { await this.page.click('.square-button > .ng-fa-icon'); await this.page.waitForTimeout(100); // Add a short wait await this.page.click('text=Create channel'); }
28-31: Approved with a suggestion for error handling.The changes to
browseChannelsButtonmethod look good. The method is now asynchronous and uses more specific selectors, which should improve reliability.Consider adding error handling to make the test more robust:
async browseChannelsButton() { try { await this.page.locator('.btn-primary.btn-sm.square-button').click(); await this.page.locator('button', { hasText: 'Browse Channels' }).click(); } catch (error) { console.error('Error in browseChannelsButton:', error); throw error; // Re-throw the error to fail the test } }
291-294: Approved with a suggestion for consistency.The changes to
createGroupChatButtonmethod look good and align with the PR objectives. The new selectors are more specific and should improve test reliability.For consistency with the
createChannelButtonmethod, consider using thetext=selector for the second click:async createGroupChatButton() { await this.page.locator('.btn-primary.btn-sm.square-button').click(); await this.page.click('text=Create Group Chat'); }This makes the selector more resilient to changes in the button's attributes while maintaining the intended functionality.
src/test/playwright/e2e/course/CourseMessages.spec.ts (3)
110-111: Approved changes with a suggestion for clarityThe simplification of the
browseChannelsButtoncall and the update to the expected channel name are consistent with the PR objectives and the new naming conventions.Consider adding a comment explaining the expected channel naming convention for lectures, to improve clarity for future maintainers. For example:
// Channel names for lectures are expected to be in the format 'test-<lecture_name>' await courseMessages.checkChannelsExists('test-lecture');
118-119: Approved changes with a suggestion for consistencyThe simplification of the
browseChannelsButtoncall and the update to the expected channel name for exercises are consistent with the PR objectives and the new naming conventions.For consistency with the previous suggestion, consider adding a comment explaining the expected channel naming convention for exercises:
// Channel names for exercises are expected to be in the format 'test-<exercise_name>' await courseMessages.checkChannelsExists('test-exercise');
190-190: Final simplifiedbrowseChannelsButtoncall approvedThe simplification of the
browseChannelsButtoncall in this test case is consistent with all previous changes and completes the implementation of the PR objective.These changes collectively improve the test suite by:
- Simplifying the
browseChannelsButtonmethod calls across all test cases.- Updating expected channel names to align with new naming conventions.
- Enhancing code consistency and maintainability.
Consider updating the documentation for the
browseChannelsButtonmethod in theCourseMessagesPageclass to reflect its new usage without specific channel type arguments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/test/playwright/e2e/course/CourseMessages.spec.ts (5 hunks)
- src/test/playwright/support/pageobjects/course/CourseMessagesPage.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/test/playwright/support/pageobjects/course/CourseMessagesPage.ts (1)
Line range hint
1-380: Overall assessment: Changes look good and align with PR objectives.The modifications to
createChannelButton,browseChannelsButton, andcreateGroupChatButtonmethods improve the robustness and reliability of the test suite. These changes address the issues with end-to-end tests caused by recent changes in the sidebar design, as mentioned in the PR objectives.Key improvements:
- More specific and reliable selectors are now used.
- The
browseChannelsButtonmethod has been simplified and made asynchronous.- The changes are consistent across the modified methods.
Minor suggestions have been made for each method to further enhance error handling, timing, and consistency. These suggestions, if implemented, would make the tests even more robust and maintainable.
src/test/playwright/e2e/course/CourseMessages.spec.ts (4)
35-35: SimplifiedbrowseChannelsButtoncall approvedThe removal of the specific channel type argument from the
browseChannelsButtoncall aligns with the PR objective and simplifies the code. This change enhances maintainability and readability.
127-127: SimplifiedbrowseChannelsButtoncall approvedThe removal of the specific channel type argument from the
browseChannelsButtoncall is consistent with previous changes and aligns with the PR objectives. This change contributes to the overall simplification and maintainability of the test suite.
166-166: SimplifiedbrowseChannelsButtoncall approvedThe simplification of the
browseChannelsButtoncall in this test case is consistent with previous changes and supports the PR's goal of updating the function to work without specific channel type arguments. This change contributes to the overall improvement in code consistency and maintainability.
Line range hint
1-424: Summary of changes and final recommendationsThe changes in this file successfully address the PR objectives by simplifying the
browseChannelsButtonmethod calls and updating expected channel names. These modifications should resolve the test failures caused by recent changes in the sidebar design.To ensure the changes have fully resolved the issues:
- Run the entire test suite to verify that all tests pass with the new implementation.
- Consider adding a test case that specifically checks the functionality of the new "Create Channel" and "Browse Channel" buttons in the sidebar.
Here's a script to run the tests and check for any remaining failures:
This script will help verify that the changes have resolved the failing tests and that no new issues have been introduced.
| browseChannelsButton(channelGroup: string) { | ||
| return this.page.locator(`#plusButton-${channelGroup}`); | ||
| async browseChannelsButton() { | ||
| await this.page.locator('.btn-primary.btn-sm.square-button').click(); | ||
| await this.page.locator('button', { hasText: 'Browse Channels' }).click(); |
There was a problem hiding this comment.
The method name is not super intuitive to me - I'd expect the method to return a locator to the browse button (like it did in the old implementation). Could we rename it to openBrowseChannelModal or something similar?
| * Clicks the button to initiate channel creation. | ||
| */ | ||
| async createChannelButton() { |
There was a problem hiding this comment.
I'd suggest the name openChannelCreation here
| async createGroupChatButton() { | ||
| await this.page.locator('#plusButton-groupChats').click(); | ||
| await this.page.locator('.btn-primary.btn-sm.square-button').click(); | ||
| await this.page.locator('button', { hasText: 'Create Group Chat' }).click(); |
There was a problem hiding this comment.
Maybe openGroupChatCreation?
| @@ -18,15 +18,16 @@ export class CourseMessagesPage { | |||
| * Clicks the button to initiate channel creation. | |||
| */ | |||
| async createChannelButton() { | |||
| await this.page.locator('#plusButton-generalChannels').click(); | |||
| await this.page.locator('.modal-content #createChannel').click(); | |||
| await this.page.click('.square-button > .ng-fa-icon'); | |||
There was a problem hiding this comment.
As I see it, all three buttons (create channel, browse channels, create group chat) are under the same menu and can be accessed via the same button.
Why do you use this.page.click('.square-button > .ng-fa-icon'); here but this.page.locator('.btn-primary.btn-sm.square-button').click() in the other methods? Could we simply assign an ID to the button that opens the dropdown and use that to find it?
(cherry picked from commit 9d8b398)
Checklist
General
Motivation and Context
Some of the tests in
CourseMessages.spec.tswere failing after the changes in sidebar design. The tests could not identify the newly added button for create channel/browse channels.Description
browseChannelButton(),createChannelButton(), andcreateGroupChatButton()have been updated so that tests can identify these buttons again.Steps for Testing
Check that the test runs through correctly.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Bug Fixes
New Features
These enhancements improve user experience by ensuring consistent channel management and clearer test outcomes.