qa e2e#350
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the E2E testing framework and covering a good range of scenarios. I'm requesting a few changes to align the submission with the core requirements before this can be approved.
A key requirement was to use data-qa attributes for all selectors, but the current implementation uses data-cy or brittle text-based selectors. Please update these throughout your tests and page objects. Additionally, several critical tests are incomplete or don't test the right thing:
- The positive sign-up test in
signUp.cy.jsmust interact with the UI form instead of making a direct API call. - The tests for updating email and password in
settings.cy.jsneed assertions to verify that the changes were successful. - The user follow test in
user.cy.jsis missing the required 'unfollow' scenario and lacks an assertion for the follow action.
Finally, please ensure tests are independent. For example, in signIn.cy.js, the user should be created in a before() hook so that each test can run on its own. Addressing these points will make your test suite much more robust!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great progress on the tests, especially with the follow/unfollow feature and adding assertions to the settings page! I'm requesting a few changes to address some critical requirements that were also mentioned in the last review.
Here are the main points to focus on:
-
Use
data-qafor all selectors: Several tests and page objects are still usingdata-cyor text-based selectors (e.g., insignIn.pageObject.js,home.pageObject.js, andarticle.cy.js). A core requirement is to usedata-qaattributes for all selectors to ensure stability. You may need to update the attributes in the Vue components first, then update your tests to usecy.getByDataQa(). -
Implement the positive sign-up test via UI: The positive sign-up test in
cypress/e2e/signUp.cy.jscurrently uses acy.register()API call. This test must be updated to interact with the UI by filling out the username, email, and password fields and clicking the sign-up button. -
Ensure tests are independent: In files like
settings.cy.js, the test setup (user creation) is in abefore()hook, which causes tests to depend on each other. Please move this logic into abeforeEach()hook so that each test runs with a fresh, clean state.
Addressing these points will make your test suite much more robust and fully aligned with the project requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on this task! I'm approving this submission as an educational override to help you move forward, but please review the feedback carefully for your next submission. You've done a great job using beforeEach hooks to make your tests independent.
The most critical issue to focus on is handling asynchronous code correctly. In settings.cy.js and user.cy.js, the tests try to sign in before the cy.task('generateUser') has finished, creating a race condition. To fix this, all actions that need the user data must be moved inside the .then() block that follows the task.
Additionally, remember to use data-qa for all selectors—some tests still use class selectors like .swal-title for popups. Finally, please create a SignUpPageObject to fully adhere to the Page Object Model pattern for the sign-up test. Addressing these points will make your test suite much more robust. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| signInPage.typePassword('wrongPassword123'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.get('.swal-title') |
There was a problem hiding this comment.
According to the task requirements, all elements in tests should be selected using data-qa attributes. Please replace this class selector (.swal-title) with a data-qa selector. You might need to add the attribute to the sweetalert component itself if it's not already there.
| cy.getByDataQa('signup-username').type(user.username); | ||
| cy.getByDataQa('signup-email').type(user.email); | ||
| cy.getByDataQa('signup-password').type(user.password); | ||
| cy.getByDataQa('signup-submit').click(); |
There was a problem hiding this comment.
To fully adhere to the Page Object Model pattern, these interactions with the sign-up form should be moved into a new SignUpPageObject class. This will encapsulate the page's logic and make your test more readable and maintainable.
| cy.getByDataQa('signup-password').type('123123'); | ||
| cy.getByDataQa('signup-submit').click(); | ||
|
|
||
| cy.get('.swal-title') |
There was a problem hiding this comment.
This selector uses a class name, which can be brittle. A core requirement for this task is to use data-qa attributes for all selectors to ensure test stability. Please add a data-qa attribute to this element and use cy.getByDataQa().
| signInPage.visit(); | ||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); | ||
| homePage.assertHeaderContainUsername(user.username); |
There was a problem hiding this comment.
These sign-in actions are not chained correctly with the asynchronous cy.task('generateUser'). This creates a race condition where the test attempts to sign in before the user object is defined, which will cause an error. All actions that depend on the user object must be moved inside the .then() block to ensure they execute in the correct order.
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
|
|
||
| cy.task('generateUser').then((u1) => { | ||
| target = u1; | ||
| cy.register(u1.email, u1.username, u1.password); | ||
| }); | ||
|
|
||
| cy.task('generateUser').then((u2) => { | ||
| follower = u2; | ||
| cy.register(u2.email, u2.username, u2.password); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This setup creates a race condition. The two cy.task('generateUser') calls are asynchronous and are not chained together. The test logic in the it block will try to access the target and follower variables before these tasks have completed and assigned values to them, leading to an error.
To fix this, you need to chain these operations. You can nest the second user creation inside the .then() block of the first one. Additionally, the user login is part of the setup and should also be moved into this beforeEach block and chained correctly.
| signInPage.visit(); | ||
| signInPage.typeEmail(follower.email); | ||
| signInPage.typePassword(follower.password); | ||
| signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
Because of the race condition in the beforeEach hook, the follower variable will be undefined when this code is evaluated, causing the test to fail. This login logic is part of the test setup and should be moved into the beforeEach hook, chained after the user creation is complete.
| signInPage.typePassword(follower.password); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.visit(`/#/@${target.username}`); |
There was a problem hiding this comment.
Similar to the follower variable, the target variable will be undefined here due to the race condition. This line will fail. This action should only run after all setup in beforeEach is guaranteed to be complete.
2 participants
met req