Solution#344
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job getting the E2E tests set up and adding data-cy attributes to the Vue components! The overall structure using the Page Object Model is on the right track. However, there are a few critical issues that are preventing some of the tests from running correctly, so I am requesting changes before this can be approved.
Here are the main points to address:
- Article Test Structure: Your test suite for articles is located in
cypress/support/pages/article.pageObject.js, which is meant for Page Object classes, not tests. This has also created a circular dependency error because the file tries to import itself. Please move thedescribeanditblocks to a new file in thecypress/e2e/directory and create a properArticlePageclass in the existing file. - Incomplete Settings Page Object: The
cypress/support/pages/settings.pageObject.jsfile is missing several methods (e.g.,updateUsername,updateBio,logout) that are called by your tests insettings.cy.js. Additionally, the selector for the update button is incorrect ([data-cy=update-settings-btn]should be[data-cy=settings-submit]). These issues will cause all settings tests to fail. - Incorrect Test Selector: In
cypress/e2e/signIn.cy.js, you are using a class selector (.login-failed) for the error message. Per the task requirements, all selectors must usedata-cyattributes for stability.
Please fix these issues to get the tests passing. You're very close!
✨ 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
| /// <reference types='../support' /> | ||
|
|
||
| import SettingsPage from '../support/pages/settings.pageObject'; | ||
| import SignUpPage from '../support/pages/signUp.pageObject'; |
There was a problem hiding this comment.
There seems to be a typo in the import path. The file you are trying to import is named signUp.page.Object.js, but here you've written signUp.pageObject. Please make sure the import path exactly matches the filename to avoid any issues with module resolution.
| it('should provide an ability to update bio', () => { | ||
| const newBio = faker.lorem.sentence(); | ||
|
|
||
| settingsPage.updateBio(newBio, user.password); |
There was a problem hiding this comment.
Passing the user's password to a function that updates the bio makes the test's purpose unclear. A test should ideally focus on a single piece of functionality. This implementation suggests that the password might also be updated, which is not what the test description it('should provide an ability to update bio', ...) implies.
| it('should provide an ability to update password', () => { | ||
| const newPass = faker.internet.password(12, true); | ||
|
|
||
| settingsPage.visit(); |
There was a problem hiding this comment.
This visit() call is redundant. The beforeEach hook (line 25) already navigates to the settings page before each test in this suite.
| signInPage.typePassword(nonRegisteredUser.password); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.get('.login-failed').should('be.visible'); |
There was a problem hiding this comment.
Using class names for test selectors is not recommended because they are tied to styling and can change. According to the task requirements, you should use data-qa (or data-cy as used elsewhere in the project) attributes for selectors to make your tests more stable.
| /// <reference types='cypress' /> | ||
| /// <reference types='../settings' /> | ||
|
|
||
| import ArticlePage from '../support/pages/article.pageObject'; |
There was a problem hiding this comment.
This file is located in the support/pages directory, which is for Page Object classes, but it contains a test suite (describe, it). Test suites should be in the e2e directory. Furthermore, this line imports the file itself, which will cause a circular dependency error and prevent the tests from running.
| import SignInPage from '../support/pages/signIn.pageObject'; | ||
| import { faker } from '@faker-js/faker'; | ||
|
|
||
| const articlePage = new ArticlePage(); |
There was a problem hiding this comment.
You're attempting to instantiate ArticlePage from the same file where the test suite is defined. Due to the recursive import on line 4, the ArticlePage class is not available here. A separate ArticlePage class should be defined in this file, and the test suite logic should be moved to a file in the cypress/e2e directory.
| url = '/settings'; | ||
|
|
||
| get updateSettingsButton() { | ||
| return cy.get('[data-cy="update-settings-btn"]'); |
There was a problem hiding this comment.
The selector for the update settings button is incorrect. If you check the src/vue/routes/Settings.vue file, the button has a data-cy attribute of settings-submit, not update-settings-btn.
| class SettingsPageObject extends PageObject { | ||
| url = '/settings'; | ||
|
|
||
| get updateSettingsButton() { | ||
| return cy.get('[data-cy="update-settings-btn"]'); | ||
| } | ||
|
|
||
| changeItem(dataCY, value) { | ||
| cy.get(`[data-cy="${dataCY}"]`).as('inputField'); | ||
| cy.get('@inputField').clear(); | ||
| cy.get('@inputField').type(value); | ||
| } | ||
|
|
||
| checkInput (dataCY, value) { | ||
| cy.get(`[data-cy="${dataCY}"]`).as('newInputField'); | ||
| cy.get('@newInputField').should('have.value', value); | ||
| } | ||
|
|
||
| clickUpdateSettingsBtn() { | ||
| this.updateSettingsButton.click(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This Page Object is missing several methods that are called in cypress/e2e/settings.cy.js. To properly follow the POM pattern, you should implement methods here for all the actions performed on the settings page, such as:
updateUsername(username)updateBio(bio)updateEmail(email)updatePassword(password)logout()- A getter for the email input field (
emailInput())
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on the E2E tests! You've addressed most of the previous feedback and built out a solid test suite. However, a couple of critical changes are needed before this can be approved.
-
Test Isolation: In
settings.cy.jsanduser.cy.js, the test setup logic is in abefore()hook. This creates a shared state for all tests in the file, which breaks test isolation and can lead to unreliable, order-dependent tests. Please move this setup logic into abeforeEach()hook to ensure each test runs independently with a fresh state. -
Selector Strategy: The tests in
signUp.cy.jsusecy.contains()to find error messages. Per the task requirements, all selectors should usedata-cyattributes for stability. Please add adata-cyattribute to the error list insrc/vue/routes/Register.vueand update the test to usecy.get()with the new selector.
Once these two issues are resolved, your solution will be in great shape. 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
| signUpPage.visit(); | ||
| signUpPage.typeUsername(user.username); | ||
| signUpPage.typeEmail(user.email); | ||
| signUpPage.typePassword(user.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| settingsPage.visit(); | ||
| settingsPage.logout(); |
There was a problem hiding this comment.
This setup block performs a full user registration and logout via the UI just to create a user for the test precondition. It's better practice to use the cy.register() custom command for this setup, as it's faster and less prone to flakiness. The test should focus on verifying the UI behavior for the second registration attempt.
| signUpPage.typePassword(user2.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| cy.contains('This email is taken.').should('exist'); |
There was a problem hiding this comment.
Per the task requirements, all elements interacted with in tests should use a data-* attribute. Please add a data-cy attribute to the error message list in Register.vue and change this assertion to something like cy.get('[data-cy=signup-errors]').should('contain', 'email is taken'). This makes your test selector more robust.
| signUpPage.typePassword(user2.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| cy.contains('This username is taken.').should('exist'); |
There was a problem hiding this comment.
Similar to the previous test, please use a data-cy selector for the error message container instead of cy.contains(). This ensures your tests are stable and adhere to the project's testing strategy.
| before(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generated) => { | ||
| user = generated; | ||
|
|
||
| cy.register(user.email, user.username, user.password); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test setup (clearing the database, creating, and registering a user) is in a before() hook. This causes all tests in this suite to share and mutate the same user object. This makes the tests dependent on each other's execution order and hard to maintain.
To ensure proper test isolation, please move the logic from this before() hook into the beforeEach() hook. This will guarantee that every test starts with a fresh, predictable user state.
| before(() => { | ||
| cy.task('db:clear'); | ||
|
|
||
| cy.task('generateUser').then((u) => { | ||
| userA = u; | ||
| return u; | ||
| }); | ||
| cy.task('generateUser').then((u) => { | ||
| userB = u; | ||
| return u; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The setup logic, including clearing the database and creating users, is inside a before() hook. This means all tests in this file will share the same initial state, which violates the principle of test isolation. If another test were added, it could be affected by the outcome of this one.
To fix this, please move the entire setup process (clearing the DB, generating users, and registering them) into a beforeEach() hook to ensure each test runs independently with a fresh state.
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 submission! I am approving this solution as an educational override to help you move forward, but please review the feedback below for your next task.
You've done a great job improving test isolation and adding data-cy attributes. To make your tests even more robust, focus on consistency. A few tests still use cy.contains() to find elements that should be targeted with data-cy attributes, such as the error message in signUp.cy.js and the article title in article.cy.js. Also, ensure all test setup logic is moved from before() to beforeEach() hooks for better test isolation, particularly in signIn.cy.js and article.cy.js.
Finally, please add the missing data-cy attribute to the user's bio element in Profile.vue. You're doing fantastic work—keep focusing on these details!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generated) => { | ||
| user = generated; | ||
|
|
||
| cy.register(user.email, user.username, user.password); | ||
| }); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| beforeEach(function () { | ||
| signInPage.visit(); | ||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| settingsPage.visit(); | ||
| }); |
There was a problem hiding this comment.
While using two beforeEach hooks is valid, it's generally clearer to group all setup logic for a test suite in a single hook. Consider combining these two beforeEach blocks into one. This makes the setup sequence easier to follow and maintain.
| settingsPage.updateBio(newBio); | ||
|
|
||
| cy.visit(`/profile/${user.username}`); | ||
| cy.contains(newBio).should('be.visible'); |
There was a problem hiding this comment.
According to the task requirements, all elements interacted with in tests should have a data-cy attribute for stability. Please add a data-cy attribute to the bio element in the Profile.vue component and update this assertion to use cy.get() with that new selector instead of cy.contains().
| signUpPage.typePassword(user.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| cy.contains(user.username).should('exist'); |
There was a problem hiding this comment.
This assertion is a bit fragile because cy.contains() will find any element on the page with the user's name. A more stable approach is to target a specific element, like the user's profile link in the header. Consider creating a method in your HomePageObject to assert the username is present in the header, similar to what's done in other test files.
| signUpPage.typePassword(duplicateEmailUser.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| cy.contains('This email is taken.').should('exist'); |
There was a problem hiding this comment.
This line still uses cy.contains() to find the error message, which was specifically mentioned in the previous review. Please use the data-cy selector you've added to locate the error container first, and then assert that it contains the correct error text. This will make your test more robust and consistent with the other negative test case.
| before(() => { | ||
| cy.task('generateUser').then((u) => { | ||
| user = u; | ||
| return u; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
For better test isolation, it's recommended to move the user generation logic from the before() hook into the beforeEach() hook. This ensures that each test runs with a completely new and independent user, preventing any state from one test from affecting another.
|
|
||
| articlePage.editArticle(updated); | ||
|
|
||
| cy.contains(updated.title).should('exist'); |
There was a problem hiding this comment.
To make your tests more robust and consistent, it's better to use the data-cy attribute to locate the article title element, just as you did in the creation test (line 38). Using cy.contains() can sometimes lead to flaky tests if the same text appears elsewhere on the page.
Consider changing this to cy.get('[data-cy=article-title]').should('contain', updated.title);.
| cy.register(userA.email, userA.username, userA.password); | ||
| cy.register(userB.email, userB.username, userB.password); |
There was a problem hiding this comment.
The users (userA and userB) are already created in the database via the API requests in your beforeEach hook (lines 21-22). These cy.register() calls are redundant and could cause unexpected test behavior. You can safely remove them and proceed directly to the sign-in step.
| </router-link> | ||
| </p> | ||
| <ul v-if="errors" class="error-messages"> | ||
| <ul v-if="errors" class="error-messages" data-cy=signup-errors> |
There was a problem hiding this comment.
Great job adding the data-cy attribute for the error list! For consistency and to follow best practices, it's recommended to enclose attribute values in double quotes, like this: data-cy="signup-errors".
2 participants
No description provided.