solution#341
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a great start, and your tests are well-structured using the Page Object Model. However, some critical requirements are missing, so changes are needed before this can be approved.
The most important issue is that the tests do not use data-qa attributes for element selection, which was a core requirement for ensuring test stability. Instead, the page objects rely on fragile selectors like placeholders, text, and CSS classes. Please update your application to include these attributes and modify your page objects to use them.
Additionally, a few other key items need to be addressed:
- The required negative test case for the sign-up flow is missing.
- The custom commands in
cypress/support/commands.jsare missingreturnstatements, which will prevent command chaining and cause tests to fail. - The
cy.logincommand incorrectly uses the registration endpoint (/api/users) instead of the login endpoint (/api/users/login).
Once these issues are resolved, your test suite will be much more robust and will meet all the task requirements.
✨ 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
|
|
||
| cy.task('generateArticle').then((article) => { | ||
| articlePage.visit(); | ||
| articlePage.typeArticleTitle(article.title); |
There was a problem hiding this comment.
A key requirement for this task is to use data-qa attributes for element selection to ensure test stability. The selectors in your article.pageObject.js file currently rely on placeholders, text content, and HTML tags. Please update the application to include data-qa attributes on the elements you interact with and then update your Page Object methods to use these attributes for selection.
| title: Math.random().toString().slice(2, 6), | ||
| description: Math.random().toString().slice(2, 8), | ||
| body: Math.random().toString().slice(2, 20), |
There was a problem hiding this comment.
The task requires using faker to generate all test data. Here, you are using Math.random() to generate the title, description, and body. Please use the appropriate methods from the faker library instead.
| }; | ||
|
|
||
| cy.createArticle().then(() => { | ||
| articlePage.clickInArticle(); |
There was a problem hiding this comment.
The method clickInArticle() is likely to fail because its selector seems to be static. After an article is created, you are on the user's profile page, which lists articles. To select the correct one to edit, you should use a dynamic selector that finds the article by its unique title. The current implementation in your Page Object appears to be looking for a static text which will not work.
| cy.createArticle(); | ||
| cy.get('@response').then((response) => { | ||
| const slug = response.article.slug; | ||
| articlePage.clickInArticle(); |
There was a problem hiding this comment.
Similar to the edit test, this step relies on the clickInArticle() method, which uses a fragile selector. This makes the test unreliable. Please ensure the selector can dynamically find the newly created article on the user's profile page before attempting to delete it.
| const homePage = new HomePageObject(); | ||
| const settingsPage = new SettingsPageObject(); | ||
|
|
||
| describe('Settings page', () => { |
There was a problem hiding this comment.
The tests in this suite rely on page object methods that use selectors based on placeholders and text content. A core requirement of the task is to add and use data-qa attributes for selectors to ensure test stability. Please update the corresponding page objects to use data-qa selectors.
| Cypress.Commands.add('getByDataCy', (selector) => { | ||
| cy.get(`[data-cy="${selector}"]`); |
There was a problem hiding this comment.
This custom command is missing a return statement. Without returning the result of cy.get(), you cannot chain other Cypress commands like .click() or .type() to it. The same issue exists in your getByPlaceholder command on line 35.
| cy.request('POST', '/api/users', { | ||
| user: { | ||
| email, | ||
| username, | ||
| password | ||
| } |
There was a problem hiding this comment.
The login command is using the /api/users endpoint, which is for creating a new user (registration). The correct endpoint for logging in an existing user is /api/users/login. While your current approach works for creating a new, logged-in user for each test, the command's name is misleading. Consider renaming it to something like registerAndLogin or using the correct login endpoint if you intend to log in with a pre-existing user.
| const settingsPage = new SettingsPageObject(); | ||
| const homePage = new HomePageObject(); | ||
|
|
||
| describe('User', () => { |
There was a problem hiding this comment.
The tests cover the required functionality, but they rely on Page Object methods that use fragile selectors like class names (.author) and text content ('Global Feed'). A key requirement of this task is to add and use data-qa attributes for all element interactions to ensure your tests are stable and maintainable. Please update your application code to include these attributes and modify your Page Objects accordingly.
| it.skip('should be able to follow the another user', () => { | ||
| it('should be able to follow the another user', () => { | ||
| homePage.clickInGlobalFeed(); | ||
| homePage.clickInAuthor(); |
There was a problem hiding this comment.
This step likely relies on clicking the first author link found on the page. While this might work after clearing the database, it's a fragile approach. A more robust test would first locate the specific article created in the setup step (e.g., by its unique title) and then click the author link within that article's component. This ensures you are always interacting with the correct user, making the test more reliable.
| const signUpPage = new SignUpPageObject(); | ||
| const homePage = new HomePageObject(); | ||
|
|
||
| describe('Sign Up page', () => { |
There was a problem hiding this comment.
The task requirements specify that you should cover both positive and negative scenarios for sign-up. This file only contains the positive case. You need to add another test case that verifies that a user cannot sign up with invalid or already existing data.
| before(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generateUser) => { | ||
| user = generateUser; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Using a before hook here means the user data is generated only once for all tests in this suite. If you add the required negative test case, this could cause conflicts or test dependency issues. It's better practice to use a beforeEach hook to ensure each test runs independently with a fresh set of data.
|
|
||
| it('should ...', () => { | ||
|
|
||
| it('should allow register with non-existing data', () => { |
There was a problem hiding this comment.
A core requirement of this task is to use data-qa attributes for element selectors to make the tests more robust. The signUpPage methods currently rely on placeholder text, which is not ideal. Please ensure you add data-qa attributes to the application's HTML and update your Page Object to use them.
| url = '/editor'; | ||
|
|
||
| get articleTitleField() { | ||
| return cy.getByPlaceholder('Article Title'); |
There was a problem hiding this comment.
The task explicitly requires using data-qa attributes for all element selectors to ensure test stability. Using getByPlaceholder is not as robust. Please add a data-qa attribute to the corresponding element in the application's code and update this selector.
| } | ||
|
|
||
| get publishButton() { | ||
| return cy.contains('.btn', 'Publish Article'); |
There was a problem hiding this comment.
Relying on button text with cy.contains makes tests fragile. If the text changes (e.g., for a different language or a UI update), the test will fail. A data-qa attribute is the required and more stable approach.
| } | ||
|
|
||
| assertContainsArticleTitle(title) { | ||
| return cy.get('h1').should('contain', title); |
There was a problem hiding this comment.
Selectors based on generic HTML tags like h1 are not reliable, as there could be multiple h1 elements on the page. Please use a data-qa attribute to uniquely identify the article title element.
| clickInArticle() { | ||
| return cy.contains('h1', 'Article title:') | ||
| .click(); |
There was a problem hiding this comment.
This selector appears to be incorrect for its intended purpose. It looks for a static text 'Article title:', which will not match the dynamically generated titles of your test articles. To make this work for editing or deleting a specific article, this method should likely accept the article's unique title as a parameter and use it to build a dynamic selector to find the correct article link on the page.
|
|
||
| get emailField() { | ||
| return cy.getByDataCy('email-sign-in'); | ||
| return cy.getByPlaceholder('Email'); |
There was a problem hiding this comment.
A key requirement for this task is to use data-qa attributes for element selectors to ensure test stability. Selecting by placeholder text is not a robust method. Please add a data-qa attribute to the email input field in the application and update this selector to use it.
|
|
||
| get signInBtn() { | ||
| return cy.getByDataCy('sign-in-btn'); | ||
| return cy.contains('.btn', 'Sign in'); |
There was a problem hiding this comment.
Relying on button text with cy.contains makes tests brittle. If the text changes (e.g., for localization or a UI copy update), the test will break. The task requires using a data-qa attribute for a more stable selector.
| return cy.contains('li', 'email or password:') | ||
| .contains('li', 'is invalid') | ||
| .should('exist'); |
There was a problem hiding this comment.
This selector is very fragile as it depends on the exact error message text. A much more reliable approach, and one that aligns with the task requirements, would be to add a data-qa attribute to the error message container in the application's code and select it using that attribute.
|
|
||
| get usernameLink() { | ||
| return cy.getByDataCy('username-link'); | ||
| return cy.get('a[href*="/profile/"]'); |
There was a problem hiding this comment.
This selector is not stable as there could be multiple profile links on the page (e.g., in the header and in each article preview). It's also not compliant with the task requirement to use data-qa attributes. Please use a specific data-qa attribute to uniquely identify the username link in the header.
| } | ||
|
|
||
| clickInGlobalFeed() { | ||
| return cy.contains('a', 'Global Feed') |
There was a problem hiding this comment.
Using cy.contains to find elements based on their text content is not a robust practice. If the text changes, the test will break. The task requires using data-qa attributes for selectors, so please add one to the 'Global Feed' link and use it here.
| } | ||
|
|
||
| clickInAuthor() { | ||
| return cy.get('.author') |
There was a problem hiding this comment.
This is a very fragile selector. Using .first() on a generic class like .author makes your test non-deterministic, as it depends on the order of articles in the feed. A reliable test should locate the specific article created during setup and then find the author link within that article's scope. This approach must also be implemented using data-qa attributes.
| } | ||
|
|
||
| followUser() { | ||
| return cy.contains('.btn', 'Follow') |
There was a problem hiding this comment.
Relying on button text like 'Follow' or 'Unfollow' makes tests brittle. These buttons should have their own data-qa attributes to ensure the selectors are stable and independent of the display text. This applies to followUser, assertFollowedUser, unfollowUser, and assertUnfollowedUser methods.
| url = 'user/register'; | ||
|
|
||
| get usernameField() { | ||
| return cy.getByPlaceholder('Username'); |
There was a problem hiding this comment.
The task explicitly requires the use of data-qa attributes for selectors to ensure test stability. Using getByPlaceholder does not meet this requirement. Please add a data-qa attribute to the username input field in the application and update this selector accordingly. This comment also applies to the emailField and passwordField getters.
| } | ||
|
|
||
| get SignUpButton() { | ||
| return cy.contains('.btn', 'Sign up'); |
There was a problem hiding this comment.
Relying on button text with cy.contains is a fragile practice, as the text could change. The task requires using data-qa attributes for all selectors. Please add a data-qa attribute to the 'Sign up' button and update this getter to use it.
| url = '/settings'; | ||
|
|
||
| get usernameField() { | ||
| return cy.getByPlaceholder('Username'); |
There was a problem hiding this comment.
The task requires using data-qa attributes for element selection. Using getByPlaceholder is not a robust solution, as placeholder text can change. Please add a data-qa attribute to the username input field in the application and update this selector.
| } | ||
|
|
||
| clickInUpdateSettings() { | ||
| return cy.contains('.btn', 'Update Settings') |
There was a problem hiding this comment.
Relying on button text with cy.contains makes tests brittle. If the button text is updated, this selector will break. A data-qa attribute on the button is the required and more stable approach.
| } | ||
|
|
||
| assertContainBio(bio) { | ||
| return cy.get('p').should('contain.text', bio); |
There was a problem hiding this comment.
Using a generic HTML tag like p to find the bio is very unreliable, as there could be many <p> elements on the page. Please use a unique data-qa attribute to identify the element containing the user's bio.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the negative test case for the sign-up flow and structuring your tests well with the Page Object Model. However, some critical changes are needed before this can be approved.
The most important issue is that the tests still rely on fragile selectors (like placeholders and text) instead of the required data-qa attributes. This was a core requirement for test stability. Additionally, there are two key problems in cypress/support/commands.js:
- The
cy.logincommand is using the incorrect registration endpoint (/api/users) instead of the correct login endpoint (/api/users/login). - All custom commands are missing
returnstatements, which is necessary to make them chainable and prevent flaky tests.
Please focus on updating your page objects to use data-qa attributes and correcting the custom commands.
✨ 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
| title: Math.random().toString().slice(2, 6), | ||
| description: Math.random().toString().slice(2, 8), | ||
| body: Math.random().toString().slice(2, 20), |
There was a problem hiding this comment.
While using Math.random() works, the task requirements specify using faker for generating test data. Using faker for all fields (e.g., faker.lorem.word() for the title, faker.lorem.sentence() for the description) would better align with the requirements and produce more realistic data.
| class ArticlePageObject extends PageObject { | ||
| url = '/editor'; | ||
|
|
||
| get articleTitleField() { | ||
| return cy.getByPlaceholder('Article Title'); | ||
| } | ||
|
|
||
| typeArticleTitle(articleTitle) { | ||
| this.articleTitleField.type(articleTitle); | ||
| } | ||
|
|
||
| get articleDescriptionField() { | ||
| return cy.getByPlaceholder(`What's this article about?`); | ||
| } | ||
|
|
||
| typeArticleDescription(description) { | ||
| this.articleDescriptionField.type(description); | ||
| } | ||
|
|
||
| get articleBodyField() { | ||
| return cy.getByPlaceholder('Write your article (in markdown)'); | ||
| } | ||
|
|
||
| typeArticleBody(body) { | ||
| this.articleBodyField.type(body); | ||
| } | ||
|
|
||
| get articleTagField() { | ||
| return cy.getByPlaceholder('Enter tags'); | ||
| } | ||
|
|
||
| typeArticleTag(tag) { | ||
| this.articleTagField.type(`${tag}{enter}`); | ||
| } | ||
|
|
||
| get publishButton() { | ||
| return cy.contains('.btn', 'Publish Article'); | ||
| } | ||
|
|
||
| assertContainsPublishButton() { | ||
| this.publishButton.should('be.visible'); | ||
| } | ||
|
|
||
| clickInPublishButton() { | ||
| this.publishButton.click(); | ||
| return cy.url().then((url) => { | ||
| const slug = url.split('/article/')[1]; | ||
| return { | ||
| article: { | ||
| slug | ||
| } | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| assertContainsArticleTitle(title) { | ||
| return cy.get('h1').should('contain', title); | ||
| } | ||
|
|
||
| assertContainsBody(body) { | ||
| return cy.get('p').should('contain', body); | ||
| } | ||
|
|
||
| clickInProfileLink() { | ||
| cy.get('@user').then((user) => { | ||
| return cy.contains('a', user.username.toLowerCase()).click(); | ||
| }); | ||
| } | ||
|
|
||
| assertContainsDescription(description) { | ||
| return cy.contains('p', 'Article description:') | ||
| .should('contain', description); | ||
| } | ||
|
|
||
| assertContainsTag(tag) { | ||
| return cy.get('li').should('contain', tag); | ||
| } | ||
|
|
||
| clickInNewArticle() { | ||
| return cy.contains('h1', 'Article title:') | ||
| .click(); | ||
| } | ||
|
|
||
| clickInEditButton() { | ||
| return cy.contains('a', ' Edit Article') | ||
| .first() | ||
| .click(); | ||
| } | ||
|
|
||
| get updateArticleButton() { | ||
| return cy.contains('.btn', 'Update Article'); | ||
| } | ||
|
|
||
| clickInUpdateArticle() { | ||
| this.updateArticleButton.click(); | ||
| } | ||
|
|
||
| clickInDeleteButton() { | ||
| return cy.contains('.btn', ' Delete Article') | ||
| .first() | ||
| .click(); | ||
| } | ||
|
|
||
| clickInArticle() { | ||
| return cy.contains('h1', 'Article title:') | ||
| .click(); | ||
| } |
There was a problem hiding this comment.
All element selectors in this file must be updated to use data-qa attributes instead of placeholders, text content, or generic HTML tags. This was a critical requirement from the task description and previous review to ensure test stability.
| Cypress.Commands.add('getByPlaceholder', (placeholder) => { | ||
| return cy.get(`[placeholder="${placeholder}"]`); | ||
| }); |
There was a problem hiding this comment.
While this custom command is a good idea for reusability, it's based on a placeholder, which is considered a fragile selector. The task requires using data-qa attributes for stability. You should create a similar command for data-qa selectors, like cy.getByQa('some-selector'), and update your Page Objects to use that instead.
| cy.request('POST', '/api/users', { | ||
| user: { | ||
| email, | ||
| username, | ||
| password | ||
| } | ||
| }); |
There was a problem hiding this comment.
This command is missing a return statement. For Cypress commands to be chainable (e.g., to use .then() after them), you must return the Cypress chain. This should be return cy.request(...).
| cy.request('POST', '/api/users', { | ||
| user: { | ||
| email, | ||
| username, | ||
| password | ||
| } | ||
| }).then((response) => { | ||
| const user = { | ||
| bio: response.body.user.bio, | ||
| effectiveImage: `https://static.productionready.io/images/smiley-cyrus.jpg`, | ||
| email: response.body.user.email, | ||
| image: response.body.user.image, | ||
| token: response.body.user.token, | ||
| username: response.body.user.username | ||
| }; | ||
| window.localStorage.setItem('user', JSON.stringify(user)); | ||
| cy.setCookie('auth', response.body.user.token); | ||
| }); |
There was a problem hiding this comment.
This command has two critical issues that were mentioned in the previous review:
- Incorrect Endpoint: You are sending a
POSTrequest to/api/users, which is the registration endpoint. For logging in, you must use the/api/users/loginendpoint. - Missing
return: The command does not return the Cypress chain, so it cannot be chained. It should start withreturn cy.request(...).
| cy.task('generateArticle').then((article) => { | ||
| articlePage.visit(); | ||
| articlePage.typeArticleTitle(article.title); | ||
| articlePage.typeArticleDescription(article.description); | ||
| articlePage.typeArticleBody(article.body); | ||
| articlePage.typeArticleTag(article.tag); | ||
| articlePage.assertContainsPublishButton(); | ||
| articlePage.clickInPublishButton().then((response) => { | ||
| articlePage.assertContainsArticleTitle(article.title); | ||
| articlePage.assertContainsBody(article.body); | ||
| articlePage.clickInProfileLink(); | ||
| articlePage.assertContainsDescription(article.description); | ||
| articlePage.assertContainsTag(article.tag); | ||
| cy.wrap(response).as('response'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This custom command is also missing a return statement. To ensure it's chainable and Cypress waits for it to complete before moving on, you should return the cy.task() call.
| homePage.clickInGlobalFeed(); | ||
| homePage.clickInAuthor(); | ||
| homePage.followUser(); | ||
| homePage.assertFollowedUser(); |
There was a problem hiding this comment.
This command is missing a return statement. You should return the chain of Cypress commands to ensure it's properly queued and chainable.
| homePage.assertHeaderContainUsername(user.username); | ||
| }); | ||
|
|
||
| it.only('should not allow register with existing data', () => { |
There was a problem hiding this comment.
Using .only is a useful tool for debugging, but it should be removed before submitting your work. It causes Cypress to skip all other tests in this file, which can hide failures.
| class SettingsPageObject extends PageObject { | ||
| url = '/settings'; | ||
|
|
||
| get usernameField() { | ||
| return cy.getByPlaceholder('Username'); | ||
| } | ||
|
|
||
| typeUsername(username) { | ||
| this.usernameField.type(username); | ||
| } | ||
|
|
||
| clearUsername() { | ||
| this.usernameField.clear(); | ||
| } | ||
|
|
||
| clickInUpdateSettings() { | ||
| return cy.contains('.btn', 'Update Settings') | ||
| .click(); | ||
| } | ||
|
|
||
| get bioField() { | ||
| return cy.getByPlaceholder('Short bio about you'); | ||
| } | ||
|
|
||
| clearBio() { | ||
| this.bioField.clear(); | ||
| } | ||
|
|
||
| typeBio(bio) { | ||
| this.bioField.type(bio); | ||
| } | ||
|
|
||
| assertUpdatedSettings() { | ||
| return cy.url().should('include', '/profile'); | ||
| } | ||
|
|
||
| assertContainBio(bio) { | ||
| return cy.get('p').should('contain.text', bio); | ||
| } | ||
|
|
||
| get emailField() { | ||
| return cy.getByPlaceholder('Email'); | ||
| } | ||
|
|
||
| typeEmail(email) { | ||
| this.emailField.type(email); | ||
| } | ||
|
|
||
| clearEmail() { | ||
| this.emailField.clear(); | ||
| } | ||
|
|
||
| assertContainEmail(email) { | ||
| this.emailField.should('have.value', email); | ||
| } | ||
|
|
||
| clickInLogout() { | ||
| cy.contains('.btn', 'Or click here to logout.') | ||
| .click(); | ||
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.getByPlaceholder('New Password'); | ||
| } | ||
|
|
||
| typeNewPassword(password) { | ||
| this.passwordField.type(password); | ||
| } | ||
|
|
||
| clearPassword() { | ||
| this.passwordField.clear(); | ||
| } | ||
|
|
||
| assertLoggedOutUser() { | ||
| cy.url().should('eq', 'https://conduit.mate.academy/'); | ||
| } | ||
| } |
There was a problem hiding this comment.
All selectors in this file need to be updated to use data-qa attributes as per the task requirements. Using placeholders, button text, and generic tags like <p> is not a stable or reliable approach for testing.
For example:
cy.getByPlaceholder('Username')should become something likecy.get('[data-qa="username-input"]').cy.contains('.btn', 'Update Settings')should becy.get('[data-qa="update-settings-button"]').
| class SignUpPageObject extends PageObject { | ||
| url = 'user/register'; | ||
|
|
||
| get usernameField() { | ||
| return cy.getByPlaceholder('Username'); | ||
| } | ||
|
|
||
| typeUsername(username) { | ||
| this.usernameField.type(username); | ||
| } | ||
|
|
||
| get emailField() { | ||
| return cy.getByPlaceholder('Email'); | ||
| } | ||
|
|
||
| typeEmail(email) { | ||
| this.emailField.type(email); | ||
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.getByPlaceholder('Password'); | ||
| } | ||
|
|
||
| typePassword(password) { | ||
| this.passwordField.type(password); | ||
| } | ||
|
|
||
| get SignUpButton() { | ||
| return cy.contains('.btn', 'Sign up'); | ||
| } | ||
|
|
||
| clickSignUpButton() { | ||
| this.SignUpButton.click(); | ||
| } | ||
|
|
||
| assertErrorMessage() { | ||
| return cy.contains('li', 'This email is taken.') | ||
| .should('exist'); | ||
| } | ||
| } |
There was a problem hiding this comment.
All selectors in this file must be updated to use data-qa attributes. Using placeholders and text content for selectors is not a stable practice and goes against the core requirements of the task. For example, cy.getByPlaceholder('Username') should be changed to something like cy.get('[data-qa="username-input"]').
No description provided.