Skip to content

Add Page Object for registration test #340

Open
JoAnna578 wants to merge 11 commits into
mate-academy:mainfrom
JoAnna578:develop
Open

Add Page Object for registration test #340
JoAnna578 wants to merge 11 commits into
mate-academy:mainfrom
JoAnna578:develop

Conversation

@JoAnna578

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 made a good start structuring your tests with the Page Object Model. However, changes are needed because the current implementation has several critical issues that prevent the tests from running and miss key requirements.

Most importantly, the required test suites for sign-in, sign-up, and follow/unfollow functionality are missing. Additionally, the existing tests for articles and settings are not functional due to a few key problems:

  • Missing Authentication: Tests that require a logged-in user (articles, settings) are missing a login step in their beforeEach hooks.
  • Tests Aren't Self-Contained: In article.cy.js, the edit/delete tests will fail because they rely on the create test's data, which is cleared before each run. Each test must create its own data.
  • Undefined Methods: Your tests call methods like createArticle() and updateUsername() which haven't been implemented in the corresponding Page Object files.
  • Incorrect Selectors: SignUpPage.js must be updated to use the required data-qa attributes for selectors instead of placeholder attributes.

Please address these points to get the tests running and meet all the task requirements.


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

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread cypress/e2e/article.cy.js Outdated
before(() => {
const articlePage = new ArticlePage();
const articleData = {
title: 'Test Article ' + Math.floor(Math.random() * 1000),

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 the faker library to generate test data. Instead, Math.random() is used here. Please use faker to generate the unique article title.

Comment thread cypress/e2e/article.cy.js Outdated
Comment on lines 15 to 17
before(() => {
articlePage.visitNewArticleForm();
});

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 before hook is missing a crucial step: logging in. A user must be authenticated to create, edit, or delete an article. Additionally, the method visitNewArticleForm() is not defined in your ArticlePage.js file.

Comment thread cypress/e2e/article.cy.js

it('should be created using New Article form', () => {

articlePage.createArticle(articleData);

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 method createArticle is not defined in the ArticlePage class. Several other methods used throughout this file (e.g., assertArticleCreated, searchArticle, editArticle, deleteArticle) are also missing from ArticlePage.js and need to be implemented.

Comment thread cypress/e2e/article.cy.js Outdated
Comment on lines 28 to 33
it('should be edited using Edit button', () => {

const updatedData = { ...articleData, body: 'Updated body content' };
articlePage.searchArticle(articleData.title);
articlePage.editArticle(updatedData);
articlePage.assertArticleUpdated(updatedData.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.

This test is not self-contained. The cy.task('db:clear') command in the beforeEach hook deletes all data before each test runs. Therefore, the article from the 'create' test does not exist when this test starts. This test must create its own article before attempting to edit it.

Comment thread cypress/e2e/article.cy.js Outdated
Comment on lines 35 to 39
it('should be deleted using Delete button', () => {

articlePage.searchArticle(articleData.title);
articlePage.deleteArticle();
articlePage.assertArticleDeleted(articleData.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.

Similar to the 'edit' test, this test is not independent. It needs to create the article it intends to delete as part of its own setup, because the database is cleared before it runs.

Comment thread cypress/e2e/settings.cy.js Outdated

it('should provide an ability to update username', () => {

const newUsername = 'NewUser_' + Math.floor(Math.random() * 1000);

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 the faker library for generating test data. While using Math.random() creates a unique value, faker provides more realistic and maintainable test data. You could use faker.internet.userName() here.

Comment on lines +19 to +20
settingsPage.updateUsername(newUsername);
settingsPage.assertUsernameUpdated(newUsername);

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 test is calling high-level methods like updateUsername and assertUsernameUpdated which are not defined in the SettingsPage page object. You should either:

  1. Implement these methods in SettingsPage.js by combining the existing lower-level actions (e.g., typeUsername and clickUpdate).
  2. Call the existing lower-level methods directly in your test file.

This same issue applies to all the other tests in this file (updateBio, assertBioUpdated, etc.).

Comment on lines +3 to +47
class ArticlePage extends PageObject {
url = '/editor';

// Form fields
get titleField() {
return cy.get('[data-cy="article-title"]');
}

get descriptionField() {
return cy.get('[data-cy="article-description"]');
}

get bodyField() {
return cy.get('[data-cy="article-body"]');
}

get tagsField() {
return cy.get('[data-cy="article-tags"]');
}

get publishBtn() {
return cy.get('[data-cy="publish-article-btn"]');
}

// Actions
typeTitle(title) {
this.titleField.clear().type(title);
}

typeDescription(description) {
this.descriptionField.clear().type(description);
}

typeBody(body) {
this.bodyField.clear().type(body);
}

typeTags(tags) {
this.tagsField.clear().type(tags);
}

clickPublish() {
this.publishBtn.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 Page Object is missing the higher-level methods that are called in your test file (article.cy.js), such as createArticle, editArticle, deleteArticle, and the various assertion methods. You should define these methods within this class to encapsulate the logic for interacting with the article page. For example, a createArticle(articleData) method would call this.typeTitle(articleData.title), this.typeDescription(articleData.description), etc., and then click the publish button.

Comment on lines +7 to +21
get usernameField() {
return cy.get('input[placeholder="Username"]');
}

get emailField() {
return cy.get('input[placeholder="Email"]');
}

get passwordField() {
return cy.get('input[placeholder="Password"]');
}

get signUpButton() {
return cy.get('button[type="submit"]');
}

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 element selectors to make tests more robust and less likely to break if the UI changes. These selectors should be updated to use data-qa attributes (e.g., cy.get('[data-qa="username-input"]')). You may need to add these attributes to the application's HTML first.

Comment on lines +24 to +34
typeUsername(username) {
this.usernameField.type(username);
}

typeEmail(email) {
this.emailField.type(email);
}

typePassword(password) {
this.passwordField.type(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.

To improve the reliability of your tests, it's a good practice to clear the input field before typing. This ensures no pre-existing text interferes with your test data. Please add .clear() before .type(), like you have done in your other Page Object files (e.g., this.usernameField.clear().type(username)).

@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 job on making the article tests self-contained—that's a big improvement! However, I'm requesting a few changes to align with the project's core requirements.

The most critical issue is the test setup in article.cy.js and settings.cy.js. The login logic is in a before() hook, but since the database is cleared before each test, subsequent tests will fail. Please move the login logic into the beforeEach() hook to ensure each test has a fresh, authenticated state.

Additionally, please address the following:

  • All element selectors in your Page Objects must use data-qa attributes as required, not data-cy or placeholder.
  • Page Object files need to be moved from cypress/support to the cypress/support/pages directory.
  • The required test suites for sign-in, sign-up, and follow/unfollow functionality are missing.
  • The password update test needs an assertion to verify the change was successful.

✨ 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 Outdated
Comment on lines 14 to 18
before(() => {
cy.task('db:clear'); // czyścimy bazę danych
signInPage.visit();
signInPage.signIn(user.email, 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 login logic should be moved from this before hook into the beforeEach hook. According to the requirements, tests needing authentication must handle login in beforeEach. This is because the database is cleared before every test, so the user logged in here would be deleted after the first test runs, causing subsequent tests to fail.

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

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 faker library is imported here but is not used anywhere in this file. It's good practice to remove any unused imports.

url = '/editor';

// Form fields
get titleField() { return cy.get('[data-cy="article-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 requirements specify that all elements should be selected using data-qa attributes. This selector, and all others in this file, are using data-cy. Please update all cy.get() calls to use data-qa attributes.

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

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 faker import is not used within this file. It's good practice to remove any unused imports to keep the code clean.

Comment thread cypress/e2e/settings.cy.js Outdated
@@ -1,32 +1,52 @@
/// <reference types='cypress' />
/// <reference types='../support' />
import SettingsPage from '../support/SettingsPage';

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, Page Object Model files should be located in the cypress/support/pages directory. This file appears to be imported from cypress/support, which is not the correct location.

Comment thread cypress/e2e/settings.cy.js Outdated
Comment on lines 14 to 18
before(() => {
cy.task('db:clear');
signInPage.visit();
signInPage.signIn(user.email, 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.

This before hook has a logical flaw that will prevent the tests from running. cy.task('db:clear') removes all users, so the subsequent signInPage.signIn() call will fail because the user doesn't exist.

To meet the requirements for test independence, the setup logic (creating a user, logging in) and database clearing should be moved into a beforeEach hook. This ensures that every test starts with a fresh, consistent state.

Comment thread cypress/e2e/settings.cy.js Outdated
Comment on lines +44 to +45
settingsPage.updatePassword(newPassword);
// nie ma asercji dla hasła, można ewentualnie ponownie zalogować użytkownika

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 test is incomplete without an assertion to verify the outcome. As your comment suggests, a good way to confirm the password was updated would be to log out and then try to log back in with the new password.

@@ -0,0 +1,53 @@
import PageObject from '../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.

The requirements state that Page Object files should be in the cypress/support/pages directory. This file should be moved there. After moving it, you will likely need to update this import path to ../../PageObject.


// Pola formularza
get usernameField() {
return cy.get('input[placeholder="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 requirements state that all element selectors must use data-qa attributes for test stability. This selector uses a placeholder attribute instead. Please update this and all other selectors in this file to use the correct data-qa attributes.

@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 job moving the sign-in logic into the beforeEach hook! That's a fantastic improvement for test reliability. I am approving this submission as an educational step to help you move forward, but there are a few important things to address in your next iteration.

Please focus on these key areas:

  • File Structure: Make sure to move all your Page Object files (like ArticlePage.js and SettingsPage.js) into the cypress/support/pages directory as required.
  • Selectors: Update all your element selectors in the Page Objects to use data-qa attributes instead of data-cy or placeholders. This is crucial for stable tests.
  • Test Implementation: The password update test is incomplete because the assertPasswordUpdated method needs to be implemented. A good approach is to log out and log back in with the new password to verify the change.

Keep up the great work, and I look forward to seeing your next submission with these fixes!


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

Footnotes

  1. Rate AI review example

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

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 faker import is not used within this file. It's a good practice to remove any unused imports to keep the code clean.

Comment on lines +8 to +13
get usernameField() { return cy.get('[data-cy="username-input"]'); }
get bioField() { return cy.get('[data-cy="bio-input"]'); }
get emailField() { return cy.get('[data-cy="email-input"]'); }
get passwordField() { return cy.get('[data-cy="password-input"]'); }
get saveBtn() { return cy.get('[data-cy="save-settings-btn"]'); }
get logoutBtn() { return cy.get('[data-cy="logout-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.

According to the task requirements, all element selectors must use data-qa attributes. Please update this selector and all others in this file to use data-qa instead of data-cy.


assertEmailUpdated(email) {
cy.get('[data-cy="email-display"]').should('contain', 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 test for updating a password requires an assertion to confirm that the change was successful. Please implement the assertPasswordUpdated method here. A common way to verify this is to log out and then successfully log back in with the new password.

Comment on lines +4 to +5
import SettingsPage from '../support/pages/SettingsPage';
import SignInPage from '../support/pages/SignInPage';

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 import paths point to the cypress/support/pages directory, but the Page Object files are still located in cypress/support. As per the task requirements and previous feedback, please move your Page Object files (like SettingsPage.js and SignInPage.js) into the cypress/support/pages directory to make these imports work.

it('should update password', () => {
const newPassword = 'NewPass@123';
settingsPage.updatePassword(newPassword);
settingsPage.assertPasswordUpdated(newPassword); // np. poprzez wylogowanie i ponowne logowanie

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 will fail because the assertPasswordUpdated method is not implemented in the SettingsPage.js file. A crucial part of this test is verifying that the password was actually changed. You could implement this by logging out and then trying to log back in with the new password.

url = '/editor';

// Form fields
get titleField() { return cy.get('[data-cy="article-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 requirements state that all element selectors must use data-qa attributes. Please update this selector and all others in this file (e.g., descriptionField, bodyField, deleteBtn) to use data-qa instead of data-cy.

}

// Assertions
assertArticleCreated(articleData) {

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 appears to be a mismatch between this method's signature and how it's called in article.cy.js. The test calls assertArticleCreated with two string arguments (title, body), but this method is defined to accept a single object articleData. This will cause a runtime error. Please update the method signature and implementation to match how it is being called.

Comment thread cypress/e2e/article.cy.js
Comment on lines +4 to +5
import ArticlePage from '../support/pages/ArticlePage';
import SignInPage from '../support/pages/SignInPage';

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 requirements and previous feedback, all Page Object files must be placed in the cypress/support/pages directory. While these import paths point to the correct location, the actual files seem to still be in cypress/support. Please move ArticlePage.js, SignInPage.js, and all other Page Object files to cypress/support/pages to match these imports.

@@ -0,0 +1,53 @@
import PageObject from '../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.

According to the task requirements, all Page Object files, including this one, must be located in the cypress/support/pages directory. Please move this file to the correct folder. The import path will be correct once the file is moved, assuming PageObject.js resides in cypress/support.

Comment on lines +8 to +20
return cy.get('input[placeholder="Username"]');
}

get emailField() {
return cy.get('input[placeholder="Email"]');
}

get passwordField() {
return cy.get('input[placeholder="Password"]');
}

get signUpButton() {
return cy.get('button[type="submit"]');

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 project requirements explicitly state that all element selectors in tests must use data-qa attributes for reliability. Using selectors based on placeholder or type attributes is not stable for end-to-end testing. Please update these selectors to use the appropriate data-qa attributes. You may need to add these attributes to the application's source code if they don't exist.

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