Skip to content

solution#346

Open
stanishevskyy wants to merge 3 commits into
mate-academy:mainfrom
stanishevskyy:e2e_testing
Open

solution#346
stanishevskyy wants to merge 3 commits into
mate-academy:mainfrom
stanishevskyy:e2e_testing

Conversation

@stanishevskyy

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Good work on setting up the end-to-end tests and implementing the Page Object Model, which provides a solid structure. However, several critical requirements need to be addressed before this can be approved.

The most significant issue is the use of data-cy attributes for test selectors throughout the project, while the task explicitly required data-qa. Please update all instances in your Vue components and Cypress page objects. Additionally, some tests are incomplete or missing, such as the test for the 'unfollow' action in user.cy.js and a required test for signing in with invalid credentials. The tests in settings.cy.js also use hardcoded data instead of faker for email and password updates. Lastly, please add cy.task('db:clear'); to the beforeEach block in cypress/e2e/signUp.cy.js to ensure proper test isolation.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread cypress/e2e/signUp.cy.js
before(() => {
let user;

beforeEach(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires clearing all data from the database before each test to ensure test isolation. You should add cy.task('db:clear'); at the beginning of this beforeEach block.

Comment thread cypress/e2e/signUp.cy.js
it('should register a new user successfully', () => {
signUpPage.fillSignUpForm(user);

cy.get('.swal-text')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task requirements, you should use data-qa attributes for all elements you interact with in tests. Using a class selector like .swal-text can make the test brittle if the class name changes. Please add a data-qa attribute to this element if possible.

Comment thread cypress/e2e/signUp.cy.js

signUpPage.fillSignUpForm(user);

cy.get('.swal-text')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous comment, this selector should use a data-qa attribute instead of a class name to make the test more robust and adhere to the project's requirements.

Comment thread cypress/e2e/article.cy.js
it('should be edited using Edit button', () => {
articlePageObject.navNewArticle.should('be.visible').click();
articlePageObject.typeNewArticle(article);
articlePageObject.articleEditorBtn.eq(0).click();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using .eq(0) can make the test fragile. If the page layout changes, this test might click the wrong element. It would be more robust to define a more specific selector in your ArticlePageObject file, for instance, by targeting the button within the page's banner.

Comment thread cypress/e2e/article.cy.js
articlePageObject.createdArticleTitle
.should('be.visible')
.should('have.text', article.title);
articlePageObject.articleDeleteBtn.should('be.visible').eq(0).click();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the edit button, using .eq(0) here is not ideal. A more specific selector in the Page Object would make this test more resilient to UI changes.

Comment thread cypress/e2e/settings.cy.js Outdated
});

it('should provide an ability to update an email', () => {
const newUserEmail = `test${1245}@mail.com`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task requirements, you should use faker to generate test data. This email is hardcoded. Please use faker.internet.email() to generate a random email for the test.

Comment thread cypress/e2e/settings.cy.js Outdated
});

it('should provide an ability to update password', () => {
const newUserPassword = '12345Qwert!12345';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using faker for test data generation. This password is a hardcoded value. Consider using faker.internet.password() to create a dynamic password for the test.

Comment thread cypress/e2e/user.cy.js
userPage.followBtn
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

userPage.assertFollowingText(`Follow ${secondUser.username}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is checking for the button's state before the click action. After a user clicks 'Follow', the UI should update to show an 'Unfollow' button. Your test should verify this change in state.

Additionally, the task requires testing both following and unfollowing. You should extend this test to also click the 'Unfollow' button and assert that the 'Follow' button reappears, ensuring the full functionality is covered.

url = '/#/register';

get usernameField() {
return cy.getByDataCy('username-sign-up');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirement specifies using data-qa attributes for selectors, but getByDataCy suggests you are using data-cy. Please ensure you are using data-qa attributes as requested in the task description.

}

get emailField() {
return cy.getByDataCy('email-sign-up');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirement specifies using data-qa attributes for selectors, but getByDataCy suggests you are using data-cy. Please ensure you are using data-qa attributes as requested in the task description.

}

get passwordField() {
return cy.getByDataCy('password-sign-up');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirement specifies using data-qa attributes for selectors, but getByDataCy suggests you are using data-cy. Please ensure you are using data-qa attributes as requested in the task description.

}

get signUpBtn() {
return cy.getByDataCy('sign-up-btn');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirement specifies using data-qa attributes for selectors, but getByDataCy suggests you are using data-cy. Please ensure you are using data-qa attributes as requested in the task description.

}

get updatedMessage() {
return cy.get('.swal-title');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for all selectors. Using a class selector like .swal-title is brittle and can break if the library's styling changes. Please find a more reliable selector or add a data-qa attribute to this element.

}

get swalButton() {
return cy.get('.swal-button');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the updatedMessage selector, this uses a class name instead of a data-qa attribute, which violates the task requirements. Tests should rely on stable attributes that are independent of styling.

url = '/#/login';

get emailField() {
return cy.getByDataCy('email-sign-in');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirement specifies using data-qa attributes for all test selectors. However, data-cy is used here and in other places. Please ensure you add and use data-qa attributes in the application's components and update your page objects accordingly.

Comment on lines +26 to +27
assertFollowingText(text) {
cy.contains(text).should('be.visible');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using cy.contains() here is not a robust testing practice because it can match any element on the page that contains the given text. This can lead to false positives. Assertions should be scoped to a specific element, for example, checking the text of the followBtn or unfollowBtn directly.

Comment thread src/vue/components/Header.vue Outdated
</router-link>
</li>
<li class="nav-item">
<li class="nav-item" data-cy="nav-new-article">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but data-cy is used here. Please change this to data-qa="nav-new-article" to meet the requirements.

Comment thread src/vue/components/Header.vue Outdated
</router-link>
</li>
<li class="nav-item">
<li class="nav-item" data-cy="nav-settings">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors. Please change data-cy to data-qa here.

Comment thread src/vue/routes/Article.vue Outdated
<div class="banner">
<div class="container">
<h1>{{ article.title }}</h1>
<h1 data-cy="article-h1-title">{{ article.title }}</h1>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirements specify using data-qa attributes for test selectors, but you've used data-cy here. Please change this to data-qa to be consistent with the requirements.

Comment thread src/vue/routes/ArticleEdit.vue Outdated
class="form-control form-control-lg"
v-model="article.title"
placeholder="Article Title"
data-cy='article-title-text'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/ArticleEdit.vue Outdated
class="form-control"
v-model="article.description"
placeholder="What's this article about?"
data-cy="article-bio-text"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/ArticleEdit.vue Outdated
rows="8"
v-model="article.body"
placeholder="Write your article (in markdown)"
data-cy="article-body-text"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/ArticleEdit.vue Outdated
</textarea>
</fieldset>
<fieldset class="form-group">
<fieldset class="form-group" data-cy="article-tag-item">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/ArticleEdit.vue Outdated
:disabled="publishing_article"
class="btn btn-lg pull-xs-right btn-primary"
type="submit"
data-cy="article-publish-btn"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/components/ArticleActions.vue Outdated
<span v-if="canModify">
<router-link class="btn btn-sm btn-outline-secondary" :to="editArticleLink">
<i class="ion-edit"></i> <span>&nbsp;Edit Article</span>
<i class="ion-edit"></i> <span data-cy="article-editor-btn">&nbsp;Edit Article</span>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you've used data-cy. Please change this to data-qa="article-editor-btn" to match the requirements.

Comment thread src/vue/components/ArticleActions.vue Outdated
<span>&nbsp;&nbsp;</span>
<button class="btn btn-outline-danger btn-sm" @click="deleteArticle">
<i class="ion-trash-a"></i> <span>&nbsp;Delete Article</span>
<i class="ion-trash-a"></i> <span data-cy="article-delete-btn">&nbsp;Delete Article</span>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute should be data-qa as per the task description, not data-cy. Please update it to data-qa="article-delete-btn".

Comment thread src/vue/routes/Register.vue Outdated
type="text"
v-model="username"
placeholder="Username"
data-cy="username-sign-up"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but data-cy is used here. Please change this to data-qa="username-sign-up" to meet the requirements.

Comment thread src/vue/routes/Register.vue Outdated
type="text"
v-model="email"
placeholder="Email"
data-cy="email-sign-up"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute should be data-qa as per the task description, not data-cy. Please update it to data-qa="email-sign-up".

Comment thread src/vue/routes/Register.vue Outdated
type="password"
v-model="password"
placeholder="Password"
data-cy="password-sign-up"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute should be data-qa as per the task description, not data-cy. Please update it to data-qa="password-sign-up".

Comment thread src/vue/routes/Register.vue Outdated
/>
</fieldset>
<button class="btn btn-lg btn-primary pull-xs-right">
<button class="btn btn-lg btn-primary pull-xs-right" data-cy="sign-up-btn">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute should be data-qa as per the task description, not data-cy. Please update it to data-qa="sign-up-btn".

Comment thread src/vue/routes/Profile.vue Outdated
class="btn btn-sm btn-secondary action-btn"
v-if="profile.following"
@click.prevent="unfollow()"
data-cy="unfollow-btn"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but data-cy is used here. Please change this to data-qa="unfollow-btn".

Comment thread src/vue/routes/Profile.vue Outdated
class="btn btn-sm btn-outline-secondary action-btn"
v-if="!profile.following"
@click.prevent="follow()"
data-cy="follow-btn"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but data-cy is used here. Please change this to data-qa="follow-btn".

Comment thread src/vue/routes/Settings.vue Outdated
<div class="row">
<div class="col-md-6 offset-md-3 col-xs-12">
<h1 class="text-xs-center">Your Settings</h1>
<h1 class="text-xs-center" data-cy="user-settings-title">Your Settings</h1>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you have used data-cy here and throughout the component. Please change all instances of data-cy to data-qa to meet the requirements.

Comment thread src/vue/routes/Settings.vue Outdated
type="text"
v-model="user.username"
placeholder="Your username"
data-cy='user-username'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/Settings.vue Outdated
rows="8"
v-model="user.bio"
placeholder="Short bio about you"
data-cy='user-bio'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/Settings.vue Outdated
type="text"
v-model="user.email"
placeholder="Email"
data-cy='user-email'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/Settings.vue Outdated
type="password"
v-model="user.password"
placeholder="Password"
data-cy='user-password'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/Settings.vue Outdated
/>
</fieldset>
<button class="btn btn-lg btn-primary pull-xs-right">
<button class="btn btn-lg btn-primary pull-xs-right" data-cy="user-btn-update">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

Comment thread src/vue/routes/Settings.vue Outdated
<!-- Line break for logout button -->
<hr />
<button @click="logout" class="btn btn-outline-danger">
<button @click="logout" class="btn btn-outline-danger" data-cy="user-btn-logout">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using data-qa attributes for test selectors, but you're using data-cy. Please change this to data-qa.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great progress on the E2E tests! You've addressed a lot of the previous feedback and the overall structure is looking good. However, a few critical issues need to be resolved before this can be approved.

Changes are needed for the following reasons:

  • Missing Tests: The required tests for signing in with both valid and invalid credentials have not been implemented. Please add a new test file to cover this functionality.
  • Broken Custom Command: In cypress/support/commands.js, the getByDataQa command is missing a return statement. This is critical because it prevents chaining other Cypress commands (like .click()) and will cause tests to fail.
  • Incorrect Test Logic: The follow/unfollow tests in cypress/e2e/user.cy.js have logical flaws. The assertions check the button's state before the click instead of after, and the 'unfollow' test needs to first perform a 'follow' action to set up the correct state.

Once these issues are addressed, your solution will be in great shape. Keep up the excellent 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread cypress/e2e/user.cy.js
userPage.followBtn
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

userPage.assertFollowingText(`Follow ${secondUser.username}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on adding the test for following a user. However, this assertion checks the state of the button before the click. To verify the action was successful, you should assert that the button text has changed to indicate the user is now being followed (e.g., it should contain 'Unfollow').

Comment thread cypress/e2e/user.cy.js
Comment on lines +62 to +65
userPage.unfollowBtn
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

userPage.assertFollowingText(`Unfollow ${secondUser.username}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test for unfollowing has a couple of logical issues:

  1. The test starts in a state where the first user is not following the second user. Therefore, the unfollowBtn will not be visible, and the test will fail when trying to find it.
  2. The text assertion .and('contain.text', 'Follow') is incorrect for an unfollowBtn, which should contain the text "Unfollow".

To fix this, you should first perform the 'follow' action within this test, and then verify that you can successfully 'unfollow' the user. The final assertion should confirm the button has reverted to the 'Follow' state.

Comment thread cypress/e2e/article.cy.js
.should('have.text', article.title);
articlePageObject.articleDeleteBtn.should('be.visible').eq(0).click();

articlePageObject.createdArticleTitle.should('not.be.visible');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion works, but it's indirect. A more robust approach would be to assert that the application has redirected to the home page after deletion, as this is the intended behavior. You could check the URL with something like cy.url().should('not.include', '/article');.

this.articleTitle.should('be.visible').clear().type(title);
this.articleBio.should('be.visible').clear().type(bio);
this.articleBody.should('be.visible').clear().type(body);
this.articleTag.should('be.visible').clear().type(`${tag}{enter}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The articleTag getter targets the <fieldset> element. While Cypress is often smart enough to find the typeable <input> within it, this is not a robust practice. It's better to make the selector more specific. Consider updating the getter to explicitly find the input element, for example: return cy.getByDataQa('article-tag-item').find('input');. This will make your test more resilient to future changes.

Cypress.Commands.add('getByDataCy', (selector) => {
cy.get(`[data-cy="${selector}"]`);
Cypress.Commands.add('getByDataQa', (selector) => {
cy.get(`[data-qa="${selector}"]`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This custom command is missing a return statement. Without it, you won't be able to chain other Cypress commands like .click() or .should() after using cy.getByDataQa(). Please add return before cy.get(...) to fix this.

Comment on lines +6 to +62
get emailField() {
return cy.getByDataQa('email-sign-in');
}

get passwordField() {
return cy.getByDataQa('password-sign-in');
}

get signInBtn() {
return cy.getByDataQa('sign-in-btn');
}

get navSettingsBtn() {
return cy.getByDataQa('nav-settings');
}

get userSettingsTitle() {
return cy.getByDataQa('user-settings-title');
}

get userNameField() {
return cy.getByDataQa('user-username');
}

get userBioField() {
return cy.getByDataQa('user-bio');
}

get userEmailField() {
return cy.getByDataQa('user-email');
}

get userPasswordField() {
return cy.getByDataQa('user-password');
}

get userUpdateButton() {
return cy.getByDataQa('user-btn-update');
}

get userLogoutBtn() {
return cy.getByDataQa('user-btn-logout');
}

get updatedMessage() {
return cy.get('.swal-title');
}

get swalButton() {
return cy.get('.swal-button');
}

login(email, password) {
this.emailField.type(email);
this.passwordField.type(password);
this.signInBtn.click();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The emailField, passwordField, signInBtn getters, and the login method are all related to the login page, not the settings page. Including them here violates the single responsibility principle of the Page Object Model. To improve the design, this logic should be moved to the SignInPageObject, and your tests should use that object to log in before navigating to and interacting with the settings page.

Comment on lines +6 to +34
get emailField() {
return cy.getByDataQa('email-sign-in');
}

get passwordField() {
return cy.getByDataQa('password-sign-in');
}

get signInBtn() {
return cy.getByDataQa('sign-in-btn');
}

get followBtn() {
return cy.getByDataQa('follow-btn');
}

get unfollowBtn() {
return cy.getByDataQa('unfollow-btn');
}

assertFollowingText(text) {
cy.contains(text).should('be.visible');
}

login(email, password) {
this.emailField.type(email);
this.passwordField.type(password);
this.signInBtn.click();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The emailField, passwordField, signInBtn getters and the login method are all specific to the login page. They should be removed from this UserPageObject to avoid code duplication. The test scripts should import and use the SignInPageObject to handle the login process before navigating to the user's profile page.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your submission as an educational override to help you progress, but there are a few critical issues that need your attention. You've done an excellent job adding all the data-qa attributes and structuring your tests with the Page Object Model! However, the most critical issue is that your getByDataQa custom command in cypress/support/commands.js is still missing a return statement, which prevents command chaining and causes the tests to fail. Additionally, the follow/unfollow tests in cypress/e2e/user.cy.js check the state before the click instead of verifying the outcome after the action. For example, after clicking 'Follow', the test should assert that the button text becomes 'Unfollow'. Please also remember to implement the missing sign-in tests. Addressing these points will make your test suite fully functional. Keep up the great work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread cypress/e2e/article.cy.js
.should('have.text', article.title);
articlePageObject.articleDeleteBtn.should('be.visible').eq(0).click();

articlePageObject.createdArticleTitle.should('not.be.visible');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is correct but not very robust. After deleting an article, the application redirects the user to the home page. A better test would be to verify this redirection explicitly, for example, by checking the URL with cy.url().should('not.include', '/article') or cy.url().should('eq', Cypress.config().baseUrl + '/#/'). This ensures you are testing the intended application flow.

Comment thread cypress/e2e/user.cy.js
userPage.followBtn
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

userPage.assertFollowingText(`Follow ${secondUser.username}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion should verify the state of the UI after the click action on line 50. After following a user, the button text is expected to change to 'Unfollow'.

Comment thread cypress/e2e/user.cy.js
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

userPage.unfollowBtn
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text assertion here is incorrect. After following the user (line 63), the button with data-qa="unfollow-btn" will contain the text 'Unfollow', not 'Follow'. This will cause the test to fail.

Comment thread cypress/e2e/user.cy.js
userPage.unfollowBtn
.should('exist').and('be.visible').and('contain.text', 'Follow').click();

userPage.assertFollowingText(`Unfollow ${secondUser.username}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the 'follow' test, this assertion is checking the state before the unfollow action on line 66. After a user is unfollowed, the button text should revert to 'Follow'.

url = '/#/settings';

get emailField() {
return cy.getByDataQa('email-sign-in');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getter, along with all others using getByDataQa, will not work because the custom command in cypress/support/commands.js is missing a return statement. This was a critical issue flagged in the previous review. Without the return, this getter returns undefined, and chaining commands like .type() will cause tests to fail.


get emailField() {
return cy.getByDataCy('email-sign-in');
return cy.getByDataQa('email-sign-in');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getter will not work as intended because the getByDataQa custom command in commands.js is missing a return statement. As a result, this method returns undefined instead of a Cypress chainable object, and any attempts to chain commands (like in the typeEmail method) will fail. This was a high-priority issue from the previous review that needs to be fixed.

url = '/#/register';

get usernameField() {
return cy.getByDataQa('username-sign-up');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getter will not work correctly because the getByDataQa custom command in cypress/support/commands.js is missing a return statement. This was a critical issue flagged in the previous review. Without the return, this getter returns undefined, and any attempt to chain a command like .type() will cause the test to fail.

Comment on lines +38 to +42
fillSignUpForm(user) {
this.typeUsername(user.username);
this.typeEmail(user.email);
this.typePassword(user.password);
this.clickSignUpBtn();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method will fail at runtime because it attempts to call .type() on this.usernameField, which is undefined due to the missing return statement in the getByDataQa custom command. The same issue applies to all other element interactions in this method.

url = '/#/login';

get emailField() {
return cy.getByDataQa('email-sign-in');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getter, along with all others using getByDataQa in this file, will return undefined instead of a Cypress chainable object. This is because the custom command in commands.js is still missing the required return statement, an issue that was flagged as high priority in the previous review. This will cause any subsequent actions like .click() or .type() to fail.

Comment on lines +30 to +34
login(email, password) {
this.emailField.type(email);
this.passwordField.type(password);
this.signInBtn.click();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This login method is duplicated in other page objects like ArticlePageObject and SettingsPageObject. To improve code maintainability and follow the DRY (Don't Repeat Yourself) principle, this common action should be centralized, for instance, in the base PageObject or handled by the SignInPageObject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants