Testing#353
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've done a great job setting up the project structure and covering all the required user flows. However, this submission requires changes to meet several core requirements for creating a robust and maintainable test suite.
Here are the main issues to address:
- Test Independence is Missing: Most test suites (e.g.,
article.cy.js,signUp.cy.js,user.cy.js) are missing thecy.task('db:seed')call in abeforeEachhook. Additionally, thedb:seedtask incypress.config.jsis only a placeholder and needs to be implemented to actually clear the database. This is critical for ensuring tests are isolated and not dependent on each other. - Incomplete Page Object Model (POM) Implementation: The advanced requirement to use the POM pattern has not been fully met. Files like
article.cy.js,settings.cy.js, andsignUp.cy.jsstill interact directly with the DOM. Please abstract these selectors and actions into their corresponding page object classes. - Missing Assertions: Some tests perform actions without verifying the outcome. For example, in
settings.cy.js, the tests for updating email and password should include an assertion to confirm the settings were successfully updated.
Addressing these points will bring your solution in line with the task requirements. I look forward to reviewing your updated 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
| 'db:seed'() { | ||
| seed(); | ||
|
|
||
| console.log('Reset database'); | ||
| return null; | ||
| } | ||
| }, |
There was a problem hiding this comment.
This db:seed task is a good start, but it currently only logs a message. According to the task requirements, it needs to "Clear all data from the database". You'll need to add the actual logic here to reset your database, for example, by running a seeding script or calling a specific API endpoint to clear the data.
| before(() => { | ||
| import { faker } from '@faker-js/faker'; | ||
|
|
||
| describe('Settings', () => { |
There was a problem hiding this comment.
While the tests are functional, they directly interact with the DOM using cy.get(). The task's advanced requirement is to use the Page Object Model (POM) pattern. You should create a settings.pageObject.js file in cypress/support/pages to encapsulate the page's selectors and methods, and then use that Page Object in this test file.
|
|
||
| cy.get('[data-qa="settings-email"]').clear().type(email); | ||
|
|
||
| cy.get('[data-qa="settings-submit"]').click(); |
There was a problem hiding this comment.
This test performs the 'click' action but doesn't verify the outcome. To ensure the test is reliable, you should add an assertion to confirm that the email update was successful. For example, you could check for the success message 'Your settings have been updated', just like in the 'update bio' test.
|
|
||
| cy.get('[data-qa="settings-password"]').type(password); | ||
|
|
||
| cy.get('[data-qa="settings-submit"]').click(); |
There was a problem hiding this comment.
This test is also missing an assertion. After clicking the submit button, you should verify that the password was updated successfully. Adding an assertion will make the test more robust and meaningful.
| describe('Article flow', () => { | ||
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.login('test@test.com', '123456'); |
There was a problem hiding this comment.
To ensure a clean state for each test run, the requirements state that you should clear the database before the test. Consider adding cy.task('db:seed'); at the beginning of this beforeEach hook.
| Cypress.Commands.add('login', (email, password) => { | ||
| cy.visit('/login'); | ||
|
|
||
| cy.get('[data-qa="login-email"]').type(email); | ||
| cy.get('[data-qa="login-password"]').type(password); | ||
|
|
||
| Cypress.Commands.add('register', (email = 'riot@qa.team', username = 'riot', password = '12345Qwert!') => { | ||
| cy.request('POST', '/users', { | ||
| email, | ||
| username, | ||
| password | ||
| }); | ||
| cy.get('[data-qa="login-submit"]').click(); | ||
| }); |
There was a problem hiding this comment.
While this custom command works, performing a full UI login before each test makes your test suite slow and less reliable. A best practice is to handle prerequisite actions like logging in programmatically.
Consider replacing the UI steps (cy.visit, cy.get, .type, .click) with a single cy.request() call that sends a POST request to your application's login endpoint. This will log the user in instantly and make your tests much faster and more robust.
|
|
||
| describe('User', () => { | ||
| before(() => { | ||
| describe('Follow / Unfollow user', () => { |
There was a problem hiding this comment.
To meet the advanced task requirements, all page interactions should be handled through a Page Object. Consider creating a profile.pageObject.js to abstract away the selectors and actions for this page.
| describe('User', () => { | ||
| before(() => { | ||
| describe('Follow / Unfollow user', () => { | ||
| beforeEach(() => { |
There was a problem hiding this comment.
The task requires clearing the database before each test to ensure they are independent. You should add cy.task('db:seed'); inside this beforeEach block to reset the application state before each test run.
|
|
||
| cy.get('[data-qa="follow-btn"]').click(); | ||
|
|
||
| cy.contains('Unfollow').should('exist'); |
There was a problem hiding this comment.
While cy.contains() works, a more robust assertion would be to target the specific element by its data-qa attribute and then check its text. This ensures you are verifying the text of the correct button.
Example:
cy.get('[data-qa="follow-btn"]').should('contain', 'Unfollow');
| }); | ||
|
|
||
| it.skip('should be able to follow the another user', () => { | ||
| it('unfollow user', () => { |
There was a problem hiding this comment.
This test has flawed logic. It performs the same action as the 'follow user' test but expects a different outcome. This makes it dependent on the initial state of the application. A better approach would be to ensure the user is already followed (perhaps by clicking the follow button first), and then test the unfollow functionality.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've made excellent progress by implementing database seeding and starting to adopt the Page Object Model (POM), which is great to see! However, this requires changes as a few critical requirements are not yet met. First, two required test cases are missing: one for editing an article in cypress/e2e/article.cy.js and another for updating a username in cypress/e2e/settings.cy.js. Additionally, the positive sign-in test in cypress/e2e/signIn.cy.js will fail because it attempts to log in a user that doesn't exist after the database is cleared; please create the user with cy.register() first. Finally, several test files, including article.cy.js, signUp.cy.js, and user.cy.js, still need to be refactored to use the POM pattern by moving DOM interactions into page object classes.
✨ 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
| import { faker } from '@faker-js/faker'; | ||
| import settingsPage from '../support/pages/settings.pageObject'; | ||
|
|
||
| describe('Settings', () => { |
There was a problem hiding this comment.
This test suite is missing a required test case. According to the task description, you also need to cover the functionality for "updating username". Please add a test for it.
| }); | ||
|
|
||
| it('should provide an ability to log in with existing credentials', () => { | ||
| it('should login with valid credentials', () => { |
There was a problem hiding this comment.
This test will fail because the database is cleared in beforeEach, but this user is never created. For a positive test case, you need to ensure the user exists before attempting to log in. Please use the cy.register() command to create the user first.
| @@ -1,24 +1,36 @@ | |||
| /// <reference types='cypress' /> | |||
| /// <reference types='../support' /> | |||
| describe('Article flow', () => { | |||
There was a problem hiding this comment.
The test suite is missing a test case for editing an article, which is a required functionality to cover according to the task description.
| cy.contains('New Article').click(); | ||
|
|
||
| it('should be created using New Article form', () => { | ||
| cy.get('[data-qa="article-title"]').type('Test Article'); | ||
| cy.get('[data-qa="article-description"]').type('Test Description'); | ||
| cy.get('[data-qa="article-body"]').type('Test Body'); | ||
|
|
||
| cy.get('[data-qa="article-publish"]').click(); | ||
|
|
||
| cy.contains('Test Article').should('exist'); |
There was a problem hiding this comment.
This test interacts directly with the DOM using cy.contains() and cy.get(). According to the 'Advanced level' requirements and previous feedback, these interactions should be abstracted into a Page Object Model (POM) class. For example, you could create an ArticlePage class with methods like createArticle(title, description, body).
| it('should register a new user with valid credentials', () => { | ||
| const username = faker.internet.userName(); | ||
| const email = faker.internet.email(); | ||
| const password = '12345Qwert!'; | ||
|
|
||
| cy.get('[data-qa="signup-username"]').type(username); | ||
| cy.get('[data-qa="signup-email"]').type(email); | ||
| cy.get('[data-qa="signup-password"]').type(password); | ||
|
|
||
| cy.get('[data-qa="signup-submit"]').click(); | ||
|
|
||
| cy.contains('Your Feed').should('exist'); | ||
| }); | ||
|
|
||
| it('should ...', () => { | ||
| it('should show validation error with invalid email', () => { | ||
| const username = faker.internet.userName(); | ||
| const password = '12345Qwert!'; | ||
|
|
||
| cy.get('[data-qa="signup-username"]').type(username); | ||
| cy.get('[data-qa="signup-email"]').type('invalid-email'); | ||
| cy.get('[data-qa="signup-password"]').type(password); | ||
|
|
||
| cy.get('[data-qa="signup-submit"]').click(); | ||
|
|
||
| cy.contains('email').should('exist'); | ||
| }); | ||
|
|
||
| it('should show validation error when password is too short', () => { | ||
| const username = faker.internet.userName(); | ||
| const email = faker.internet.email(); | ||
|
|
||
| cy.get('[data-qa="signup-username"]').type(username); | ||
| cy.get('[data-qa="signup-email"]').type(email); | ||
| cy.get('[data-qa="signup-password"]').type('123'); | ||
|
|
||
| cy.get('[data-qa="signup-submit"]').click(); | ||
|
|
||
| cy.contains('password').should('exist'); | ||
| }); |
There was a problem hiding this comment.
These tests interact directly with the DOM using cy.get(). According to the 'Advanced level' requirements and the previous review feedback, you should abstract these interactions into a Page Object class. Create a signUp.pageObject.js file and define methods like fillUsername(username), fillEmail(email), and submit() to encapsulate the element selection and actions.
| it('should follow user', () => { | ||
| cy.get('[data-qa="follow-btn"]').click(); | ||
|
|
||
| cy.get('[data-qa="follow-btn"]').should('contain', 'Unfollow'); | ||
| }); | ||
|
|
||
| it.skip('should be able to follow the another user', () => { | ||
| it('should unfollow user', () => { | ||
| cy.get('[data-qa="follow-btn"]').click(); | ||
|
|
||
| cy.get('[data-qa="follow-btn"]').should('contain', 'Unfollow'); | ||
|
|
||
| cy.get('[data-qa="follow-btn"]').click(); | ||
|
|
||
| cy.get('[data-qa="follow-btn"]').should('contain', 'Follow'); |
There was a problem hiding this comment.
These tests directly interact with the DOM using cy.get(). According to the advanced task requirements and previous review feedback, this logic should be moved into a Page Object Model (POM) class. For example, you could create a ProfilePage object with a followButton() element and a clickFollow() method.
| @@ -0,0 +1,44 @@ | |||
| import PageObject from '../PageObject'; | |||
|
|
|||
| class SettingsPage extends PageObject { | |||
There was a problem hiding this comment.
This page object is missing the necessary selectors and methods to support the 'update username' test case, which is a requirement. Please add elements and actions for the username field, similar to how you've handled email and password.
| emailInput() { | ||
| return cy.get('[data-qa="settings-email"]'); | ||
| } | ||
|
|
||
| passwordInput() { | ||
| return cy.get('[data-qa="settings-password"]'); | ||
| } | ||
|
|
||
| bioInput() { | ||
| return cy.get('[data-qa="settings-bio"]'); | ||
| } | ||
|
|
||
| submitButton() { | ||
| return cy.get('[data-qa="settings-submit"]'); |
There was a problem hiding this comment.
While this works, your base PageObject class provides a getByQa() helper method. For consistency with the pattern you've established in other page objects (like signIn.pageObject.js), you should use the inherited method here. For example: return this.getByQa('settings-email');.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your submission as an educational override to help you progress, but it's important to address the feedback below for your next steps. You've done a great job adding the missing test for editing an article and starting to implement the Page Object Model!
To bring your solution fully in line with the requirements, please focus on these key areas:
- Use
data-qaattributes for all selectors. This is a core requirement for test stability. Currently, your tests rely on text and placeholders (e.g., incypress/support/pages/article.pageObject.js). - Implement the missing tests. The tests for updating user settings (bio, username, email, password) are still missing from
cypress/e2e/settings.cy.js, and the user test needs to cover both following and unfollowing. - Test through the UI. The positive sign-up test in
cypress/e2e/signUp.cy.jsmust interact with the form fields instead of using thecy.register()API command.
Keep up the great effort! Focusing on these points will significantly improve the quality and reliability of your tests.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| }); | ||
|
|
||
| it('should be created using New Article form', () => { | ||
| cy.contains('a', 'New Article').click(); |
There was a problem hiding this comment.
Interacting with elements directly in the test file (e.g., using cy.contains) doesn't fully follow the Page Object Model pattern. This logic should be moved to a method in the appropriate page object. Additionally, the task requires using data-qa attributes for selectors to make tests more robust, rather than relying on text content.
|
|
||
| cy.contains('a', 'Edit Article').click(); | ||
|
|
||
| articlePage.typeTitle('updated'); |
There was a problem hiding this comment.
The .type() command appends text to an input field. To properly edit the title, you should clear the existing content before typing the new value. You can do this by chaining .clear().type('updated').
| it('should not provide an ability to log in with wrong credentials', () => { | ||
| signInPage.visit(); | ||
|
|
||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password + '123'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.contains('div[class="swal-title"]', 'Login failed!').should( | ||
| 'be.visible', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test for invalid credentials depends on the previous test ('should provide an ability to log in with existing credentials') to register the user. If this test is run alone, it will fail because the user doesn't exist. Tests should be independent. Consider moving the user registration into a beforeEach hook so that a fresh user is created and registered before each test in this suite.
| let email; | ||
| let password; | ||
|
|
||
| before(() => {}); |
There was a problem hiding this comment.
This before hook is empty and can be removed.
| it('should ...', () => { | ||
| it('should sign up succefully', () => { | ||
| signInPage.visit(); | ||
| cy.register(email, username, password); |
There was a problem hiding this comment.
This test should validate the sign-up functionality through the user interface. Instead of using the cy.register() command which makes a direct API call, the test should navigate to the sign-up page, fill out the form fields with the generated user data, and submit the form.
| describe('Sign In page', () => { | ||
| let user; | ||
|
|
||
| before(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generateUser) => { | ||
| user = generateUser; | ||
| }); | ||
| }); | ||
|
|
||
| it('should provide an ability to update an email', () => { | ||
|
|
||
| it('should provide an ability to log in with existing credentials', () => { | ||
| signInPage.visit(); | ||
| cy.register(user.email, user.username, user.password); | ||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); | ||
| homePage.assertHeaderContainUsername(user.username); | ||
| }); | ||
|
|
||
| it('should provide an ability to update password', () => { | ||
|
|
||
| }); | ||
| it('should not provide an ability to log in with wrong credentials', () => { | ||
| signInPage.visit(); | ||
|
|
||
| it('should provide an ability to log out', () => { | ||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password + 'wrong111'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.contains('div[class="swal-title"]', 'Login failed!').should( | ||
| 'be.visible', | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This file is intended for testing the settings page, but it currently contains tests for signing in. The task requires you to implement tests for updating the user's bio, username, email, and password here. Please replace the existing content with the correct tests for the settings functionality.
| return cy.get('input[placeholder="Article Title"]'); | ||
| } | ||
|
|
||
| get aboutField() { | ||
| return cy.get(`input[placeholder="What's this article about?"]`); | ||
| } | ||
|
|
||
| get textField() { | ||
| return cy.get('textarea[placeholder="Write your article (in markdown)"]'); | ||
| } | ||
|
|
||
| get tagsField() { | ||
| return cy.get('input[placeholder="Enter tags"]'); | ||
| } | ||
|
|
||
| get publishArticleButton() { | ||
| return cy.contains('button', 'Publish Article'); |
There was a problem hiding this comment.
The task explicitly requires using data-qa attributes for all element selectors to ensure test stability. Relying on placeholder text or visible text like cy.contains makes tests fragile, as these attributes can change frequently. Please update all selectors in this file to use data-qa attributes.
| it('should be able to follow another user', () => { | ||
| signInPage.visit(); | ||
| signInPage.typeEmail(userFollower.email); | ||
| signInPage.typePassword(userFollower.password); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.wait(1000); | ||
| cy.visit(`/#/@${userTarget.username}`); | ||
| cy.contains('button', `Follow ${userTarget.username}`).click(); | ||
| }); |
There was a problem hiding this comment.
The task requires testing both following and unfollowing a user. This test only covers the 'follow' action. You should also add an assertion to verify the follow was successful (e.g., the button text changes) and then test the 'unfollow' functionality.
| signInPage.typePassword(userFollower.password); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.wait(1000); |
There was a problem hiding this comment.
Using cy.wait() with a fixed duration is discouraged as it can lead to flaky tests that are either too slow or fail intermittently. It's better to wait for a specific element to appear or for a network request to complete.
| cy.visit(`/#/@${userTarget.username}`); | ||
| cy.contains('button', `Follow ${userTarget.username}`).click(); |
There was a problem hiding this comment.
To fully adhere to the Page Object Model (POM) pattern, these interactions should be moved into a new UserProfilePageObject. Additionally, the task requires using data-qa attributes for selectors, not text content via cy.contains.
2 participants
No description provided.