Skip to content

cypress_project#347

Open
robsza1122 wants to merge 3 commits into
mate-academy:mainfrom
robsza1122:develop
Open

cypress_project#347
robsza1122 wants to merge 3 commits into
mate-academy:mainfrom
robsza1122:develop

Conversation

@robsza1122

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

You've done a great job building out the E2E test suite and implementing the Page Object Model. The structure is well-organized. However, I'm requesting a few changes to address some critical issues.

The most important issue is that the task required data-qa for test selectors, but data-cy was used project-wide. This needs to be corrected in all Vue components and tests. Additionally, please add a database reset command (e.g., cy.task('db:clear')) to a beforeEach hook in each test file to ensure tests run in a clean environment.

Finally, there are a few functional issues to resolve:

  • The follow/unfollow test logic in user.cy.js is incorrect; it should use two separate users.
  • The Settings.vue component has a fatal error due to a duplicate mounted hook.
  • Several router-links are broken because they are missing the : binding for the to prop.

Once these key items are addressed, your solution will be in great shape!


✨ 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

const homePage = new HomePageObject();
const settingsPage = new SettingsPageObject();

describe('Settings page', () => {

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 suite is missing a beforeEach hook to handle common setup tasks.

  1. DRY Principle: The setup logic for registering and logging in a user is repeated in every test. This common setup should be extracted into a beforeEach hook to avoid code duplication and improve maintainability.
  2. Database Reset: A key requirement is to clear the database before each test to ensure test isolation. A call to cy.task('db:clear') should be included in the beforeEach hook.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typeUserName('NewUser123!');

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 new username is hardcoded. According to the task requirements, you should use faker or a custom utility to generate dynamic test data. This will make your tests more robust.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typeBio('New 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 bio text is hardcoded. Please use faker to generate this data dynamically to comply with the task requirement of using fake data in tests.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typeEmail('updateMail@example.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.

The new email address is hardcoded. It's best practice to generate this dynamically using faker to avoid potential conflicts and ensure the test is reliable.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typePassword('UpdatePassword1');

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 new password is hardcoded. Please generate a dynamic password for the update, as required by the task.

Comment thread cypress/e2e/article.cy.js
const articlePage = new ArticlePageObject();

describe('Article', () => {
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.

A critical requirement is to 'Clear all data from the database before the test'. This step is missing from your beforeEach hook. To ensure each test runs independently and in a clean environment, please add a call to reset the database here. It looks like you have a cy.resetBase() custom command that can be used for this.

Comment thread cypress/e2e/article.cy.js
Comment on lines +43 to +45
articlePage.typeEdittedTitle('New Editted Title');
articlePage.typeEdittedDescription('New Editted Description');
articlePage.typeEdittedBody('New Editted Body!!!');

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 hardcoded values for test data is not ideal. The task requires using faker to generate data. Please use your generateArticle() utility to create dynamic content for the updated article, just as you do for the initial creation. This will make your test more robust.

Comment thread cypress/e2e/article.cy.js Outdated
articlePage.clickDeleteArticleBtn();
homePage.clickHomeLink();
cy.scrollTo('bottom');
articlePage.assertionDeletedArticle(title, description);

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 for a deleted article is not reliable. If there are other articles on the page, should('not.have.text', title) will pass even if the deletion failed. A better way to check for non-existence is cy.contains(selector, text).should('not.exist').

Furthermore, since this check is performed on the home page, the assertion logic should be handled by the homePage object to maintain a clean Page Object Model structure. It seems homePage already has a better implementation for this.

Comment thread .gitignore Outdated
.idea
node_modules/
src/public/js/app.js
src/webpack.config.js

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 webpack configuration file should not be ignored. It's a source file required to build the project, and other developers will need it to run the application and tests. Please remove this line.

Comment thread cypress/e2e/signIn.cy.js
cy.task('generateUser').then((generateUser) => {
user = generateUser;
});
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 that you 'Clear all data from the database before the test.' This beforeEach hook is the perfect place to do that, but the step is missing. Please add a command here to reset the database to ensure that each test starts with a clean state and is not affected by previous tests.

Comment thread cypress/e2e/user.cy.js
const signUpPage = new SignUpPageObject();

describe('Follow/unfollow button', () => {
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 the database before each test to ensure test isolation. Please add a cy.task('db:clear') call within this beforeEach hook.

Comment thread cypress/e2e/user.cy.js Outdated
Comment on lines +19 to +37
it('should provide an ability to follow the another user', () => {
const { username, email, password } = generateUser();
const { title, description, body, tags } = generateArticle();

signUpPage.typeUserName(username);
signUpPage.typeEmail(email);
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
articleInPage.clickArticleBtn();
articleInPage.typeTitle(title);
articleInPage.typeDescription(description);
articleInPage.typeBody(body);
articleInPage.typeTags(tags);
articleInPage.handleAddingArticle();
homeInPage.clickHomeLink();
homeInPage.clickYourFeedBtn();
homeInPage.clickFollowBtn();
homeInPage.assertFollowedUser();

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 has two main issues:

  1. Logical Flaw: A user cannot follow themselves. To properly test this feature, you need to simulate two different users. One user should create an article, and then a second user should log in to find that article and follow the original author.
  2. Code Duplication: The entire setup of creating a user and an article is repeated in the next test. This common logic should be extracted into the beforeEach hook to keep your tests DRY (Don't Repeat Yourself).

Comment thread cypress/e2e/user.cy.js
Comment on lines +40 to +59
it('should provide an ability to unfollow the another user', () => {
const { username, email, password } = generateUser();
const { title, description, body, tags } = generateArticle();

signUpPage.typeUserName(username);
signUpPage.typeEmail(email);
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
articleInPage.clickArticleBtn();
articleInPage.typeTitle(title);
articleInPage.typeDescription(description);
articleInPage.typeBody(body);
articleInPage.typeTags(tags);
articleInPage.handleAddingArticle();
homeInPage.clickHomeLink();
homeInPage.clickYourFeedBtn();
homeInPage.clickFollowBtn();
homeInPage.clickFollowBtn();
homeInPage.assertUnfollowedUser();

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 case repeats the same flawed setup logic from the previous test. By moving the common user and article creation into a beforeEach hook and correcting the test logic to use two separate users, you will fix the duplication and logical issues in both tests.

Comment thread cypress/e2e/signUp.cy.js
const signInPage = new SignInPageObject();

describe('Sign Up page', () => {
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 that you 'Clear all data from the database before the test'. This beforeEach hook is missing a call to cy.task('db:clear') to ensure that each test starts with a clean database state. Please add it here.

Comment thread cypress/e2e/signUp.cy.js
homePage.assertHeaderContainUsername(username);
});

it(`should not accept email without dot, '@' and spaces`, () => {

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 case validates multiple scenarios (different invalid email formats) within a single it block. It is a best practice to create separate, focused test cases for each scenario. This improves readability, makes failures easier to diagnose, and reduces code repetition within the test.

Comment thread cypress/e2e/signUp.cy.js
signUpPage.typeEmail('Cierra@examplecom');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You are using an assertion method from signInPage within a test for the sign-up page. This creates an unnecessary dependency between page objects. The logic for asserting an error on the sign-up page should be part of the SignUpPageObject to maintain a clear and logical structure.

Comment thread cypress/support/commands.js Outdated
Comment on lines +31 to +33
Cypress.Commands.add('login',
// eslint-disable-next-line max-len
(email = 'user32@hotmail.com', username = 'user32', password = 'Userpass1') => {

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 command is named 'login', but it actually registers a new user by sending a POST request to the /users endpoint, which can be confusing. Additionally, it uses hardcoded default values. Custom commands should be generic and receive all necessary data as arguments from the test, aligning with the requirement to use faker for test data.

Comment thread cypress/support/commands.js Outdated
Comment on lines +59 to +60
Cypress.Commands.add('register',
(email = 'riot@qa.team', username = 'riot', password = '12345Qwert!') => {

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 command uses hardcoded default values for registration. To comply with the requirement of using dynamically generated fake data, please remove these defaults and ensure that the user data is passed in from the test that calls this command.


class SettingsPageObject extends PageObject {
get usernameInSettings() {
return cy.getByDataCy('update-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 description requires using data-qa attributes for all test selectors. This implementation uses data-cy. Please update the custom command and all selectors in your Page Objects and Vue components to use data-qa as specified in the requirements.

}

get swalBtn() {
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.

Selecting elements by CSS class names like .swal-button makes tests fragile because these classes are often tied to styling and can change. It's a best practice to use test-specific attributes (like data-qa) for selectors. If you can modify the component that generates this button, please add a data-qa attribute to it.

Comment on lines +46 to +59
get editorArtTitle() {
return cy.getByDataCy('editor-art-title');
}

get editorArtDescription() {
return cy.getByDataCy('editor-art-description');
}

get editorArtBody() {
return cy.getByDataCy('editor-art-body');
}

get editorArtTags() {
return cy.getByDataCy('editor-art-tags');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These getters (editorArtTitle, editorArtDescription, etc.) are duplicates of the ones defined earlier in the file (e.g., titleInput, descriptionInput). To avoid redundancy and make the code cleaner and easier to maintain, you should define each element selector only once.

Comment on lines +149 to +154
assertionDeletedArticle(title, description) {
this.previewTitle
.should('not.have.text', title);
this.previewDescription
.should('not.have.text', description);
}

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 for asserting deletion is unreliable. should('not.have.text', ...) can pass if any other article preview is on the page, leading to a false positive. A better approach is to verify that the specific article preview element does not exist, for example, using cy.contains('[data-cy="preview-title"]', title).should('not.exist').

Additionally, since this assertion is run on the home page (after navigating back), this logic should be moved to the HomePageObject.


get usernameLink() {
get homeLink() {
return cy.getByDataCy('home');

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 was to use data-qa attributes for selectors. While data-cy is functional, it's important to adhere to the project's specific guidelines. Please update the custom command and the attributes in the Vue components from data-cy to data-qa.

.should('not.exist');
}

checkProfileLinkForLogout() {

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, checkProfileLinkForLogout, is an exact duplicate of assertLoggingOut. To adhere to the DRY (Don't Repeat Yourself) principle and avoid code redundancy, please remove this method and use assertLoggingOut in its place.

.click();
};

clickInDeletionOfArticle() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's a best practice in the Page Object Model to define all selectors as getters at the top of the class. Here, you are calling cy.getByDataCy directly inside the method. To improve maintainability, you should create a getter for the 'delete-article-btn' and then reference it here (e.g., this.deleteArticleBtn.first().click();). The same applies to the clickInEditionOfArticle method.

.click();
}

assertFollowedUser() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Asserting against CSS classes like btn-primary can make tests fragile, as class names may change for purely stylistic reasons. A more robust approach is to check for something the user would see, like the button's text (e.g., 'Unfollow User'), or to use a dedicated data attribute that reflects the state. The same applies to the assertUnfollowedUser method.

Comment thread src/vue/components/ArticleActions.vue Outdated
<!-- Used when user is also author -->
<span v-if="canModify">
<router-link class="btn btn-sm btn-outline-secondary" :to="editArticleLink">
<router-link class="btn btn-sm btn-outline-secondary" :to="editArticleLink" data-cy="edit-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 update this and all other data-cy attributes in the project to data-qa to meet the requirements.

Comment thread src/vue/components/ArticleActions.vue Outdated
class="btn btn-sm"
@click="toggleFavorite"
:class="toggleFavoriteButtonClasses"
data-cy="follow-user-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 selector name follow-user-btn is misleading as this button handles the 'favorite' action (@click="toggleFavorite"), not following a user. Please rename this selector to something more descriptive, like favorite-article-btn, to avoid confusion.

@@ -0,0 +1,52 @@
import PageObject from '../PageObject';

class SignUpPageObject extends PageObject {

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 Page Object is missing a crucial method: one for asserting sign-up errors. Each Page Object should be self-contained. The corresponding test file currently calls an assertion method from SignInPageObject, which breaks encapsulation. Please add an assertSignUpError() method to this class to handle error validation for the sign-up page.

url = '/#/register';

get usernameInput() {
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 description requires using data-qa attributes for all test selectors. This implementation uses data-cy. Please ensure this is corrected throughout the project by renaming the custom command and updating the attributes in the Vue components.

}

get swalBtn() {
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.

Using CSS class names for selectors is not a robust practice, as class names are prone to change for styling reasons. The task requires adding data-qa attributes to all elements used in tests. Please add a data-qa attribute to this sweet alert button in the application's source code and update the selector here accordingly.

}

get errorScreen() {
return cy.get('.swal-modal');

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 a CSS class like .swal-modal for selectors can make your tests fragile, as class names are often tied to styling and can change. The task requires adding data-qa attributes for all elements you interact with in tests. Please add a data-qa attribute to this modal and update the selector accordingly for better test stability.

Comment on lines +22 to +23
get headerHomeBtn() {
return cy.getByDataCy('home');

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 header home button is a common element that appears on multiple pages, not just the sign-in page. According to the task requirements, common elements should be defined in the base PageObject.js file to avoid duplication and improve maintainability.

Comment on lines +41 to +44
clickHeaderHomeBtn() {
this.headerHomeBtn
.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.

Since the headerHomeBtn element should be moved to a base Page Object, this corresponding action method should also be moved there to keep related logic together.

Comment thread src/vue/routes/Article.vue Outdated
<div class="banner">
<div class="container">
<h1>{{ article.title }}</h1>
<h1 data-cy="article-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 requires using data-qa for test selectors, but this element uses data-cy. Please update this attribute to data-qa="article-title" to align with the requirements.

Comment thread src/vue/routes/Article.vue Outdated
<div class="row article-content">
<div class="col-xs-12">
<div v-html="parseMarkdown(article.body)"></div>
<div v-html="parseMarkdown(article.body)" data-cy="article-body"></div>

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 element uses data-cy for its test selector. Please change it to data-qa as specified in the task description.

Comment thread src/vue/routes/Article.vue Outdated
<Tag
:name="tag"
className="tag-default tag-pill tag-outline"
data-cy="article-tag"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please change this attribute from data-cy to data-qa to follow the project's testing requirements.

Comment thread src/vue/components/Header.vue Outdated
active-class="active"
exact
:to="{ name: 'home' }"
data-cy="header-home-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 explicitly requires using data-qa attributes for all test selectors. Please change data-cy to data-qa here and for all other selectors in this component to meet the requirement.

<div class="article-meta">
<router-link
:to="{ name: 'profile', params: { username: authorUsername() } }"
to="{ name: 'profile', params: { username: authorUsername() } }"

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 router-link's to prop is being treated as a static string because it's missing the : binding prefix. To make this a dynamic binding that correctly navigates to the author's profile, it should be written as :to="{ name: 'profile', params: { username: authorUsername() } }".

Comment thread src/vue/components/ArticleMeta.vue Outdated
<router-link
:to="{ name: 'profile', params: { username: authorUsername() } }"
class="author"
data-cy="author-link"

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 explicitly requires using data-qa attributes for all test selectors. This should be changed from data-cy to data-qa="author-link" to meet the requirements.

Comment thread src/vue/components/ArticleMeta.vue Outdated
'btn-primary': article && article.favorited,
'btn-outline-primary': article && !article.favorited
}"
data-cy="follow-user-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.

This selector has two issues:

  1. It should use data-qa instead of data-cy as per the task requirements.
  2. The name follow-user-btn is misleading because this button's @click handler calls toggleFavorite. It should be renamed to something like favorite-article-btn to accurately reflect its function.

Comment thread src/vue/components/ArticlePreview.vue Outdated
Comment on lines +5 to +6
<h1 v-text="article.title" data-cy="preview-title"/>
<p v-text="article.description" data-cy="preview-description" />

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 explicitly requires using data-qa attributes for all test selectors. Please rename these data-cy attributes to data-qa to comply with the project's requirements.

Comment thread src/vue/routes/Login.vue Outdated
Comment on lines +71 to +82
setTimeout(() => {
const swalModal = document.querySelector('.swal-modal');
if (swalModal) {
swalModal.setAttribute('data-cy', 'login-loading-modal');
}


const swalText = document.querySelector('.swal-text');
if (swalText) {
swalText.setAttribute('data-cy', 'login-status-text');
}
}, 50);

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 code dynamically adds test selectors to the sweet alert modals, which is a good approach for testability. However, the task requires using data-qa attributes, not data-cy. Please update these attributes to data-qa to meet the project's requirements.

Comment thread src/vue/routes/Login.vue Outdated
Comment on lines +95 to +99
}).then(() => {
const errorModal = document.querySelector('.swal-modal');
if (errorModal) {
errorModal.setAttribute('data-cy', 'login-error-modal');
}

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 loading modal, the test attribute for the error modal should be data-qa instead of data-cy to comply with the task requirements.

Comment thread src/vue/routes/Home.vue Outdated
:to="{ name: 'home-my-feed' }"
class="nav-link"
active-class="active"
data-cy="your-feed-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 has been used here. This needs to be corrected to data-qa="your-feed-btn" to align with the project requirements.

Comment thread src/vue/routes/Home.vue Outdated
<li

class="nav-item"
data-cy="global-feed-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.

This data-cy attribute on the <li> element is redundant because the same selector is already present on the router-link inside it. You should only have one test selector for a single interactive element.

Comment thread src/vue/routes/Home.vue
>
<router-link
:to="{ name: 'home' }"
to="{ name: 'home' }"

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 to prop for this router-link is being passed as a static string because it's missing the : binding prefix. This will cause the link to not work as expected. It should be :to="{ name: 'home' }" to be treated as a JavaScript object.

Comment thread src/vue/routes/Home.vue
<li class="nav-item" v-if="tag">
<router-link
:to="{ name: 'home-tag', params: { tag } }"
to="{ name: 'home-tag', params: { tag } }"

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 link, this to prop is missing the : binding prefix, so it's being interpreted as a static string instead of a dynamic route object. It should be corrected to :to="{ name: 'home-tag', params: { tag } }".

Comment thread src/vue/routes/Home.vue Outdated
<p>Popular Tags</p>
<div class="tag-list">
<Tag v-for="(tag, index) in tags" :name="tag" :key="index">
<Tag v-for="(tag, index) in tags" name="tag" key="index">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's a bug in how you are passing the name prop to the Tag component. name="tag" passes the literal string "tag" for every iteration of the loop. You need to bind it to the tag variable from the v-for loop like this: :name="tag".

@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 this task! You've successfully implemented the Page Object Model and addressed several key issues from the last review. However, there are a few critical issues that need to be resolved before this can be approved.

Several parts of the site navigation are broken due to missing : bindings on router-links in src/vue/routes/Home.vue and src/vue/components/ArticleMeta.vue. The follow/unfollow test in cypress/e2e/user.cy.js is unreliable as it navigates to the wrong feed and targets the wrong element; the data-qa="follow-user-btn" attribute is incorrectly placed on the 'favorite article' button in ArticleMeta.vue. Additionally, the requirement to use data-qa for all selectors is not fully met, as the modals in src/vue/routes/Login.vue and src/vue/routes/Register.vue are missing these attributes.

Please address these points to ensure the application is fully functional and the tests are reliable.


✨ 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/article.cy.js
Comment on lines +18 to +23
signInPage.visit();
cy.register(email, username, password);
signInPage.typeEmail(email);
signInPage.typePassword(password);
signInPage.clickSignInBtn();
homePage.clickArticleBtn();

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 cy.register() custom command already handles user creation and programmatic login by setting the session token. The subsequent UI actions to log in (visiting the sign-in page, typing credentials, and clicking the button) are redundant. For faster and more stable tests, it's best to rely on programmatic login for setup and navigate directly to the page under test afterward.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typeUserName('NewUser123!');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's a good practice to use faker or a similar library to generate test data instead of hardcoding values like 'NewUser123!'. This aligns with the task requirement and makes the test more robust by using unique data for each run.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typeBio('New 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.

This hardcoded bio text should be dynamically generated using faker, for example with faker.lorem.sentence(). This ensures the test is not dependent on static data.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typeEmail('updateMail@example.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.

This hardcoded email should be generated dynamically. You can define a new variable for the updated email at the start of the test and use it here and again when you log in with the new credentials.

signInPage.clickSignInBtn();
signInPage.clickHeaderHomeBtn();
homePage.clickSettingsLink();
settingsPage.typePassword('UpdatePassword1');

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 other tests in this file, the new password should be generated dynamically using faker instead of being hardcoded. This improves test reliability.

Comment thread cypress/e2e/signUp.cy.js
Comment on lines +28 to 47
it(`should not accept email without dot, '@' and spaces`, () => {
const { username, password } = generateUser();
signUpPage.typeUserName(username);
signUpPage.typeEmail('Cierra@examplecom');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();
signUpPage.clickSwalBtn();
signUpPage.typeUserName(username);
signUpPage.typeEmail('Cierra@example com');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();
signUpPage.clickSwalBtn();
signUpPage.typeUserName(username);
signUpPage.typeEmail('Cierraexample.com');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();
});

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 block attempts to check multiple invalid scenarios sequentially. However, the type... methods in your page objects append text without clearing the input fields first. After the first failed submission, the form fields are not cleared, causing subsequent type... calls to append to the existing text. This results in unintended values like usernameusername being submitted.

To fix this, consider either clearing the fields before each attempt within this test or, for better test atomicity, splitting this into separate it blocks for each validation case.

Comment thread cypress/e2e/user.cy.js
Comment on lines +15 to 31
beforeEach(() => {
signUpPage.visit();
const { username, email, password } = generateUser();
const { title, description, body, tags } = generateArticle();

signUpPage.typeUserName(username);
signUpPage.typeEmail(email);
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
articleInPage.clickArticleBtn();
articleInPage.typeTitle(title);
articleInPage.typeDescription(description);
articleInPage.typeBody(body);
articleInPage.typeTags(tags);
articleInPage.handleAddingArticle();
});

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 beforeEach block is missing cy.task('db:clear'), which is required to ensure tests are isolated from each other. Additionally, the setup logic of creating a user and an article via the UI is inefficient and can make tests flaky. It's better to use programmatic commands (like cy.register() and an API call to create an article) for test setup.

Comment thread cypress/e2e/user.cy.js Outdated
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
homeInPage.clickHomeLink();
homeInPage.clickYourFeedBtn();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Navigating to 'Your Feed' is incorrect here. 'Your Feed' only shows articles from users that the current user is following. Since this is a newly created user, their feed will be empty. The test should instead navigate to the 'Global Feed' to find the article posted by the first user.

Comment thread cypress/e2e/user.cy.js Outdated
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
homeInPage.clickHomeLink();
homeInPage.clickYourFeedBtn();

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 navigation step is incorrect. The test should go to the 'Global Feed' to find the other user's article before attempting to follow and then unfollow them.

Comment on lines +31 to +59
Cypress.Commands.add(
'login',
// eslint-disable-next-line max-len
(
email = 'user32@hotmail.com',
username = 'user32',
password = 'Userpass1'
) => {
cy.request('POST', '/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);
});
}
);

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 command appears to be incorrect. It's sending a request to the user registration endpoint (/users) rather than a login endpoint. Since it's also not used in any of the provided tests, it would be best to remove it to avoid confusion and potential misuse.


Cypress.Commands.add('getByDataCy', (selector) => {
cy.get(`[data-cy="${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.

The name of this command, getByDataCy, is now misleading since it selects elements using the data-qa attribute. To maintain consistency and clarity across the project, please rename this command to getByDataQa. You'll also need to update its usage in all Page Object files and its type definition in cypress/support/index.d.ts.

Comment on lines +68 to +71
cy.request('POST', '/users', {
email,
username,
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 payload for this registration request may be incorrect. Typically, APIs like this expect the user data to be nested within a user object, like so: { user: { email, username, password } }. While the current implementation might work, it's worth double-checking the expected API contract to ensure robustness and consistency.

url = '/editor';

get titleInput() {
return cy.getByDataCy('editor-art-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.

While this works, the custom command is named getByDataCy but it's actually selecting by data-qa. This can be confusing. It's better to rename the command to getByDataQa in commands.js and update its usage here for clarity and consistency.

Comment on lines +46 to +60
get editorArtTitle() {
return cy.getByDataCy('editor-art-title');
}

get editorArtDescription() {
return cy.getByDataCy('editor-art-description');
}

get editorArtBody() {
return cy.getByDataCy('editor-art-body');
}

get editorArtTags() {
return cy.getByDataCy('editor-art-tags');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These getters (editorArtTitle, editorArtDescription, editorArtBody, editorArtTags) are duplicates of the getters defined above (titleInput, descriptionInput, bodyInput, tagInput). They all select the same elements. To reduce redundancy and improve maintainability, you should remove these duplicate getters and update the typeEditted... methods to use the original ones (e.g., this.titleInput.clear().type(title)).


get usernameLink() {
get homeLink() {
return cy.getByDataCy('home');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As highlighted in the review for commands.js, the custom command getByDataCy should be renamed to getByDataQa to accurately reflect its function. Please update this and all other occurrences throughout the file for consistency and clarity.

Comment on lines +102 to +108
clickInDeletionOfArticle() {
cy.getByDataCy('delete-article-btn').first().click();
}

clickInEditionOfArticle() {
cy.getByDataCy('edit-article-btn').first().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.

These two methods, clickInDeletionOfArticle and clickInEditionOfArticle, are not used in any of the tests and appear to target elements that belong on the article page, not the home page. To keep the page object focused and clean, these unused methods should be removed.

Comment thread cypress/support/index.d.ts Outdated
@@ -4,5 +4,6 @@ declare namespace Cypress {
interface Chainable<Subject> {
getByDataCy(selector: string): Chainable<any>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In line with the project-wide change from data-cy to data-qa, this type definition should be renamed from getByDataCy to getByDataQa. This will ensure the type definition accurately reflects the custom command's implementation in commands.js.


class SettingsPageObject extends PageObject {
get usernameInSettings() {
return cy.getByDataCy('update-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.

As mentioned in the review for commands.js, this custom command should be renamed from getByDataCy to getByDataQa to accurately reflect that it's selecting elements using the data-qa attribute. This change should be applied project-wide for consistency.

Comment on lines +18 to +20
get errorScreen() {
return cy.get('.swal-modal');
}

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 elements in tests. This selector uses a CSS class (.swal-modal), which is less stable. In the src/vue/routes/Login.vue file, there's already logic to dynamically add a data-cy attribute to this modal. Please update that logic to add a data-qa attribute instead, and then use that attribute here for selection.

Comment on lines +22 to +24
get headerHomeBtn() {
return cy.getByDataCy('home');
}

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 headerHomeBtn and its corresponding click method are for a common header element. According to the task requirements, elements common to the whole app should be in a shared location, like a base PageObject or a dedicated HeaderPageObject. While this works, consider refactoring to avoid duplicating header elements in multiple page objects.

url = '/#/register';

get usernameInput() {
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.

As noted in the reviews for commands.js and other page objects, the custom command getByDataCy should be renamed to getByDataQa to accurately reflect its function of selecting elements by the data-qa attribute. This change should be applied consistently across all files.

<div class="article-meta">
<router-link
:to="{ name: 'profile', params: { username: authorUsername() } }"
to="{ name: 'profile', params: { username: authorUsername() } }"

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 router-link is missing the : before the to prop. Without the binding, the to value is treated as a static string instead of a dynamic route object, which will break navigation. This was a specific issue that needed to be fixed from the previous review.

'btn-outline-primary': article && !article.favorited
'btn-outline-primary': article && !article.favorited,
}"
data-qa="follow-user-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 data-qa attribute here is named follow-user-btn, but the button's action is to favorite an article (@click="toggleFavorite"). This is misleading and causes the follow/unfollow tests to interact with the wrong element. The attribute should be renamed to accurately reflect its purpose, for example, favourite-article-btn.

Comment thread src/vue/routes/Home.vue
<li class="nav-item" data-qa="global-feed-btn">
<router-link
:to="{ name: 'home' }"
to="{ name: 'home' }"

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 router-link is missing the : before the to prop. Without the dynamic binding, the link will not function correctly, as the to value is treated as a static string instead of a route object. This was a critical fix requested in the previous review.

Comment thread src/vue/routes/Home.vue
<li class="nav-item" v-if="tag">
<router-link
:to="{ name: 'home-tag', params: { tag } }"
to="{ name: 'home-tag', params: { tag } }"

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 'Global Feed' link, this router-link is missing the : binding for the to prop. This will prevent the tag-based navigation from working as intended.

<h1 class="text-xs-center">Sign up</h1>
<p class="text-xs-center">
<router-link :to="{ name: 'login' }">
<router-link to="login">

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 router-link should use a dynamic :to binding to ensure it resolves to the correct route object. It should be :to="{ name: 'login' }" instead of to="login" for consistency and best practice with named routes.

@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 solution so you can continue to make progress, but please review the following feedback carefully for your next submission.

You've done an excellent job building out the test suite and using the Page Object Model to structure your tests. However, there are a few critical issues to address. The most important is that several navigation links in the application are broken because they are missing the : binding on the to prop (e.g., in src/vue/routes/Home.vue). Additionally, the follow/unfollow test is failing because the data-qa="follow-user-btn" attribute is incorrectly placed on the 'favorite article' button instead of the actual follow button. Please also ensure all test data, including updates, is generated dynamically with faker instead of being hardcoded.

Keep up the great work! Addressing these key points will significantly improve the reliability of both the application and your tests.


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

Footnotes

  1. Rate AI review example

Comment on lines +20 to +24
signInPage.visit();
cy.register(email, username, password);
signInPage.typeEmail(email);
signInPage.typePassword(password);
signInPage.clickSignInBtn();

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 cy.register() custom command already logs the user in by setting the authentication token. The subsequent UI login steps (visiting the sign-in page, typing credentials, and clicking the button) are redundant. You can navigate directly to the settings page after cy.register().

Comment on lines +27 to +29
settingsPage.typeUserName('NewUser123!');
settingsPage.clickUpdateSettingsBtn();
homePage.assertHeaderContainUsername('NewUser123!');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of hardcoding the new username, you should generate it using faker or another user from generateUser(). Store it in a variable and use that variable for both typing the new username and for the assertion. This makes the test more dynamic and less prone to errors.

Comment on lines +41 to +45
settingsPage.typeBio('New Bio Text!!!');
settingsPage.clickUpdateSettingsBtn();
homePage.clickSettingsLink();
settingsPage.clickSwalBtn();
settingsPage.assertUpdatedBio('New 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.

Hardcoded values should be avoided. Please use faker to generate a bio text. Store it in a variable to use it for both typing and assertion.

Comment on lines +57 to +63
settingsPage.typeEmail('updateMail@example.com');
settingsPage.clickUpdateSettingsBtn();
homePage.clickSettingsLink();
settingsPage.clickSwalBtn();
settingsPage.clickLogoutBtn();
homePage.clickSignInNav();
signInPage.typeEmail('updateMail@example.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.

This test uses a hardcoded email address. It's better to generate a new email using faker, store it in a variable, and use that variable here and in the login step.

Comment on lines +78 to +84
settingsPage.typePassword('UpdatePassword1');
settingsPage.clickUpdateSettingsBtn();
settingsPage.clickSwalBtn();
settingsPage.clickLogoutBtn();
homePage.clickSignInNav();
signInPage.typeEmail(email);
signInPage.typePassword('UpdatePassword1');

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 new password is hardcoded. Please generate a new password, perhaps using faker or just a different string stored in a variable, and use it for both updating and logging in.

Comment thread cypress/e2e/article.cy.js
Comment on lines +18 to +22
signInPage.visit();
cy.register(email, username, password);
signInPage.typeEmail(email);
signInPage.typePassword(password);
signInPage.clickSignInBtn();

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 cy.register() custom command already handles user registration and logs the user in by setting the authentication token. The subsequent UI login steps (typing email/password and clicking sign-in) are redundant and make the test setup slower. You can remove lines 18-22 and navigate directly to the 'new article' page after cy.register completes.

Comment thread cypress/e2e/article.cy.js
Comment on lines +44 to +46
articlePage.typeEdittedTitle('New Editted Title');
articlePage.typeEdittedDescription('New Editted Description');
articlePage.typeEdittedBody('New Editted Body!!!');

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 to generate test data. Here, you are using hardcoded strings for the edited article's title, description, and body. You should use generateArticle() again to create new, dynamic data for the update.

Comment on lines +31 to +59
Cypress.Commands.add(
'login',
// eslint-disable-next-line max-len
(
email = 'user32@hotmail.com',
username = 'user32',
password = 'Userpass1'
) => {
cy.request('POST', '/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);
});
}
);

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 command appears to be unused across the test suite, as all tests use cy.register() instead. Furthermore, its name is misleading because it's calling the user registration endpoint (POST /users), not a login endpoint. It's best to remove this unused command to avoid confusion.

Comment on lines +68 to +71
cy.request('POST', '/users', {
email,
username,
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 payload for this API request is incorrect. The backend API expects the user data to be nested inside a user object. The correct structure should be { user: { email, username, password } }. Without this change, the registration will fail.

Comment on lines +88 to 90
Cypress.Commands.add('resetBase', () => {
return cy.request('POST', 'http://localhost:1667/api/testing/reset');
});

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 resetBase command is not being used in any of the test files. The tests consistently use cy.task('db:clear') for resetting the database state. To keep the codebase clean, you should remove this unused command.

Comment thread cypress/e2e/user.cy.js
Comment on lines +14 to 61
describe('Follow/unfollow button', () => {
beforeEach(() => {
cy.task('db:clear');
signUpPage.visit();
const { username, email, password } = generateUser();
const { title, description, body, tags } = generateArticle();

signUpPage.typeUserName(username);
signUpPage.typeEmail(email);
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
articleInPage.clickArticleBtn();
articleInPage.typeTitle(title);
articleInPage.typeDescription(description);
articleInPage.typeBody(body);
articleInPage.typeTags(tags);
articleInPage.handleAddingArticle();
});

it.skip('should be able to follow the another user', () => {
it('should provide an ability to follow the another user', () => {
signUpPage.visit();
const { username, email, password } = generateUser();
signUpPage.typeUserName(username);
signUpPage.typeEmail(email);
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
homeInPage.clickHomeLink();
homeInPage.clickGlobalFeedBtn();
homeInPage.clickFollowBtn();
homeInPage.assertFollowedUser();
});

it('should provide an ability to unfollow the another user', () => {
signUpPage.visit();
const { username, email, password } = generateUser();
signUpPage.typeUserName(username);
signUpPage.typeEmail(email);
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signUpPage.clickSwalBtn();
homeInPage.clickHomeLink();
homeInPage.clickGlobalFeedBtn();
homeInPage.clickFollowBtn();
homeInPage.clickFollowBtn();
homeInPage.assertUnfollowedUser();
});

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 suite has a critical flaw: it doesn't test the follow/unfollow functionality. It clicks the 'Favorite Article' button instead. This is because the data-qa="follow-user-btn" selector is incorrectly placed on the favorite button in the ArticleMeta.vue component, an issue that was highlighted in the previous review.

To fix this, you should:

  1. Correct the data-qa attribute placement in the Vue components. The follow button is available on the user's profile page (Profile.vue).
  2. Update the test flow to navigate to the author's profile page from the global feed and then click the correct follow button.

Additionally, the test setup is inefficient. Creating users and articles via the UI is slow and brittle. A better approach is to use cy.register() and cy.request() to set up the necessary data (author, article, follower) via API calls in a beforeEach hook. This makes the tests much faster and more reliable.

Comment thread cypress/e2e/signUp.cy.js
Comment on lines +30 to +46
signUpPage.typeUserName(username);
signUpPage.typeEmail('Cierra@examplecom');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();
signUpPage.clickSwalBtn();
signUpPage.typeUserName(username);
signUpPage.typeEmail('Cierra@example com');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();
signUpPage.clickSwalBtn();
signUpPage.typeUserName(username);
signUpPage.typeEmail('Cierraexample.com');
signUpPage.typePassword(password);
signUpPage.clickSignUpBtn();
signInPage.assertSignInError();

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 case contains a lot of repeated code for checking different invalid email formats. The sequence of typing user info, clicking sign-up, asserting an error, and closing the modal is duplicated three times. A better approach would be to store the invalid emails in an array and loop through them, performing the check for each one. This would make your test more concise and easier to extend.

}

get swalBtn() {
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.

The task requirements and previous review feedback explicitly state that all selectors must use data-qa attributes for stability. This getter uses a class selector (.swal-button), which is brittle and violates the requirement. You will need to add a data-qa attribute to the sweetalert modal/button in the application code and update this selector to use it.

Comment on lines +30 to +118
get followBtn() {
return cy.getByDataQa('follow-user-btn').first();
}

get feedBtn() {
return cy.getByDataQa('global-feed-btn');
}

get newTitleInBanner() {
return cy.getByDataQa('banner-title');
}

get bodyUnderBanner() {
return cy.getByDataQa('article-page-body');
}

get previewTitle() {
return cy.getByDataQa('preview-title');
}

get previewDescription() {
return cy.getByDataQa('preview-description');
}

get author() {
return cy.getByDataQa('author-link');
}

get globalFeedBtn() {
return cy.getByDataQa('your-feed-btn');
}

assertHeaderContainUsername(username) {
this.usernameLink
.should('contain', username);
this.profileLink.should('contain', username);
}

assertLoggingOut() {
this.profileLink.should('not.exist');
}

clickHomeLink() {
this.homeLink.click();
}

clickSettingsLink() {
this.settingsLink.click();
}

clickSignInNav() {
this.signInNavigation.click();
}

clickArticleBtn() {
this.articleLink.click();
}

clickGlobalFeedBtn() {
this.feedBtn.click();
}

clickProfileLink() {
this.profileLink.click();
}

clickFollowBtn() {
this.followBtn.click();
}

clickAuthor() {
this.author.click();
}

clickInDeletionOfArticle() {
cy.getByDataQa('delete-article-btn').first().click();
}

clickInEditionOfArticle() {
cy.getByDataQa('edit-article-btn').first().click();
}

assertFollowedUser() {
this.followBtn.should('have.class', 'btn btn-sm pull-xs-right btn-primary');
}

assertUnfollowedUser() {
this.followBtn.should(
'have.class',
'btn btn-sm pull-xs-right btn-outline-primary'
);

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 the assertFollowedUser and assertUnfollowedUser methods, is incorrectly implemented. The data-qa="follow-user-btn" selector currently points to the 'favorite article' button in the article feed (ArticleMeta.vue), not the user follow button. The assertions also check the classes of the favorite button. This was a key issue from the previous review. The actual follow button is on the user's profile page (Profile.vue). This implementation must be corrected to target the right element on the correct page.

Comment on lines +38 to +132
get newTitleInBanner() {
return cy.getByDataQa('banner-title');
}

get bodyUnderBanner() {
return cy.getByDataQa('article-page-body');
}

get previewTitle() {
return cy.getByDataQa('preview-title');
}

get previewDescription() {
return cy.getByDataQa('preview-description');
}

get author() {
return cy.getByDataQa('author-link');
}

get globalFeedBtn() {
return cy.getByDataQa('your-feed-btn');
}

assertHeaderContainUsername(username) {
this.usernameLink
.should('contain', username);
this.profileLink.should('contain', username);
}

assertLoggingOut() {
this.profileLink.should('not.exist');
}

clickHomeLink() {
this.homeLink.click();
}

clickSettingsLink() {
this.settingsLink.click();
}

clickSignInNav() {
this.signInNavigation.click();
}

clickArticleBtn() {
this.articleLink.click();
}

clickGlobalFeedBtn() {
this.feedBtn.click();
}

clickProfileLink() {
this.profileLink.click();
}

clickFollowBtn() {
this.followBtn.click();
}

clickAuthor() {
this.author.click();
}

clickInDeletionOfArticle() {
cy.getByDataQa('delete-article-btn').first().click();
}

clickInEditionOfArticle() {
cy.getByDataQa('edit-article-btn').first().click();
}

assertFollowedUser() {
this.followBtn.should('have.class', 'btn btn-sm pull-xs-right btn-primary');
}

assertUnfollowedUser() {
this.followBtn.should(
'have.class',
'btn btn-sm pull-xs-right btn-outline-primary'
);
}

assertNewArticle(title, description) {
this.newTitleInBanner.should('contain.text', title);
this.bodyUnderBanner.should('contain.text', description);
}

assertTitleInBanner(title) {
this.newTitleInBanner.should('contain.text', title);
}

assertBodyUnderBanner(body) {
this.bodyUnderBanner.should('contain.text', body);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These elements and the assertion methods that use them (assertNewArticle, assertTitleInBanner, assertBodyUnderBanner) belong to the article detail page, not the home page. They should be moved to Article.pageObject.js. Additionally, the selectors banner-title and article-page-body do not appear to be present in the application code provided.

Comment on lines +58 to 60
get globalFeedBtn() {
return cy.getByDataQa('your-feed-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.

This getter is named globalFeedBtn but it selects your-feed-btn. This is misleading and there is already another getter, feedBtn, that correctly selects global-feed-btn. This confusing duplicate should be removed to improve clarity.

Comment on lines +102 to +108
clickInDeletionOfArticle() {
cy.getByDataQa('delete-article-btn').first().click();
}

clickInEditionOfArticle() {
cy.getByDataQa('edit-article-btn').first().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.

These methods for editing and deleting an article belong on the article details page, so they should be moved to Article.pageObject.js. Furthermore, the selectors they use (delete-article-btn, edit-article-btn) are incorrect as the -btn suffix is not in the application code. Correct implementations already exist in Article.pageObject.js, so these duplicates should be removed.

register(email: string, username: string, password: string): Chainable<any>
getByDataQa(selector: string): Chainable<any>;
register(email: string, username: string, password: string): Chainable<any>;
resetBase(): Chainable<any>;

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 resetBase command is not used in any of the test files; instead, cy.task('db:clear') is used for resetting the database. Since the command itself should be removed from commands.js for code cleanliness, this type declaration should also be removed.

Comment on lines +46 to +59
get editorArtTitle() {
return cy.getByDataQa('editor-art-title');
}

get editorArtDescription() {
return cy.getByDataQa('editor-art-description');
}

get editorArtBody() {
return cy.getByDataQa('editor-art-body');
}

get editorArtTags() {
return cy.getByDataQa('editor-art-tags');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These getters (editorArtTitle, editorArtDescription, editorArtBody, editorArtTags) are duplicates of titleInput, descriptionInput, bodyInput, and tagInput defined earlier in this class. To improve code maintainability and reduce redundancy, you should remove these duplicates and update the typeEditted... methods to use the original getters.

}

get swalBtn() {
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.

The task requirements and previous review feedback explicitly state that all selectors must use data-qa attributes for stability. This getter uses a class selector (.swal-button), which is brittle and violates the requirement. You will need to add a data-qa attribute to the sweetalert modal/button in the application code and update this selector to use it.

}

get errorScreen() {
return cy.get('.swal-modal');

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 and previous review feedback state that all selectors must use data-qa attributes. This selector uses a class name, which is not ideal for test stability. Please add a data-qa attribute to the error modal in src/vue/routes/Login.vue and update this getter to use cy.getByDataQa().

<div class="article-meta">
<router-link
:to="{ name: 'profile', params: { username: authorUsername() } }"
to="{ name: 'profile', params: { username: authorUsername() } }"

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 router-link is missing the required : binding for the to prop. Without it, the to attribute is treated as a static string instead of a route object, which will break the navigation to the author's profile page.

'btn-outline-primary': article && !article.favorited
'btn-outline-primary': article && !article.favorited,
}"
data-qa="follow-user-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.

This data-qa attribute is incorrect. This button is for favoriting an article (it has a heart icon and calls toggleFavorite), but the data-qa attribute is follow-user-btn. This was a high-priority issue in the last review and is the reason the follow/unfollow tests are failing or testing the wrong functionality. The follow-user-btn selector should be on the actual follow/unfollow button, which is located on the user's profile page (Profile.vue).

Comment thread src/vue/routes/Home.vue
<li class="nav-item" data-qa="global-feed-btn">
<router-link
:to="{ name: 'home' }"
to="{ name: 'home' }"

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 router-link is missing the required : before the to prop. Without the binding, the to attribute is treated as a literal string "{ name: 'home' }" instead of a route object, which will break navigation. This was a high-priority issue from the previous review that still needs to be addressed.

Comment thread src/vue/routes/Home.vue
<li class="nav-item" v-if="tag">
<router-link
:to="{ name: 'home-tag', params: { tag } }"
to="{ name: 'home-tag', params: { tag } }"

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 global feed link, this router-link is also missing the : binding for its to prop. It should be :to to correctly bind to the route object.

<h1 class="text-xs-center">Sign up</h1>
<p class="text-xs-center">
<router-link :to="{ name: 'login' }">
<router-link to="login">

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 router-link should use a bound :to attribute for consistency with the rest of the application. While to="login" may work, using :to="{ name: 'login' }" is the standard practice for named routes and is more robust.

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