Solution#335
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your suite shows good progress (coverage of key flows, DB reset, some POM and helper usage), but there are critical blockers against the explicit requirements.
Critical blockers to fix:
-
Stable selectors using
data-qa(required):- Multiple files use placeholders, text, classes, or
data-cyinstead ofdata-qa:cypress/e2e/article.cy.js(all selectors)cypress/e2e/settings.cy.js(all selectors)cypress/e2e/signUp.cy.js(all selectors)cypress/e2e/signIn.cy.jsnegative path (around lines 38–40, e.g.,ul.error-messagesand.find('li li'))cypress/e2e/user.cy.jsusesdata-cyinstead ofdata-qathroughout.
- Why: The checklist requires
data-qafor all interacted elements for stability. - How: Add
data-qaattributes in the application components and update tests/POM to usecy.get('[data-qa="..."]')or a helper likecy.getByQa('...').
- Multiple files use placeholders, text, classes, or
-
POM structure not used consistently (required):
- Tests with inline selectors/actions:
article.cy.js,settings.cy.js,signUp.cy.js. - Why: The checklist requires POM under
cypress/support/pagesand importing into specs. - How: Create page objects (e.g.,
ArticlePage,SettingsPage,AuthPage) undercypress/support/pages, move selectors and actions there, and use them in specs.
- Tests with inline selectors/actions:
-
Missing file breaks build (blocker):
cypress/e2e/user.cy.jsimport fails:../support/pages/user.pageObjectnot found.- Why: Prevents the spec from running at all.
- How: Add
cypress/support/pages/user.pageObject.js(ensure correct filename/case and export), or fix the import path/name.
-
Faker/helper usage not consistent (required):
article.cy.jsuses timestamps instead offakeror a helper.- Why: Requirement mandates
fakeror an equivalent helper for test data. - How: Replace timestamp-based values with
@faker-js/fakeror yourgenerate*helper.
-
Tests failing due to unmatched selectors/missing elements (blocker):
- Errors like:
Expected to find element: input[placeholder="Email"], but never found it.across multiple specs. - Why: App not rendering expected elements, routing not reached, or selectors are outdated.
- How: Verify the app is running at the expected URL, confirm the page under test loads before interacting, and update selectors to the new
data-qaattributes. Consider waiting for route or key element withdata-qa.
- Errors like:
Minor improvements (after the blockers):
- Standardize a shared base page object for common nav/header elements.
- Add a custom command
getByQafor selector consistency. - Ensure CI uses Node 16+ to avoid dependency/runtime issues.
You’re close on structure in signIn.cy.js and user.cy.js, and your DB reset and helper usage are solid. Address the data-qa selectors, complete POM migration, restore the missing page object, and align all specs with stable selectors to meet the checklist. Keep going—you’re nearly there!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| cy.task('db:clear'); | ||
|
|
||
| const timestamp = Date.now(); | ||
| user = { |
There was a problem hiding this comment.
You are not using faker or a custom helper for generating test data. The requirements specify that test data should be generated using faker or an equivalent helper, not just timestamps.
| cy.task('db:clear'); | ||
| cy.visit('/#/login'); | ||
|
|
||
| cy.get('input[placeholder="Email"]').type(user.email); |
There was a problem hiding this comment.
Selectors should use data-qa attributes for all elements interacted with in tests, as required by the checklist. Using placeholders and button text is not stable and does not meet the requirements.
| cy.visit('/#/login'); | ||
|
|
||
| cy.get('input[placeholder="Email"]').type(user.email); | ||
| cy.get('input[placeholder="Password"]').type(user.password); |
There was a problem hiding this comment.
Selectors should use data-qa attributes for all elements interacted with in tests, as required by the checklist. Using placeholders and button text is not stable and does not meet the requirements.
|
|
||
| cy.get('input[placeholder="Email"]').type(user.email); | ||
| cy.get('input[placeholder="Password"]').type(user.password); | ||
| cy.get('button').contains('Sign in').click(); |
There was a problem hiding this comment.
Selectors should use data-qa attributes for all elements interacted with in tests, as required by the checklist. Using placeholders and button text is not stable and does not meet the requirements.
| }); | ||
|
|
||
| it('should be created using New Article form', () => { | ||
| it('should create a new article', () => { |
There was a problem hiding this comment.
You are not using the Page Object Model (POM) structure as required. Element selectors and actions should be moved to POM classes under cypress/support/pages and imported here.
| cy.get('ul.error-messages li ul li', { timeout: 10000 }) | ||
| .should('contain.text', 'This email does not seem valid.'); | ||
|
|
||
| cy.get('ul.error-messages li ul li') |
There was a problem hiding this comment.
Selectors should use data-qa attributes for all elements interacted with in tests, as required by the checklist. Using class names and tag selectors is not stable and does not meet the requirements.
| beforeEach(() => { | ||
| cy.visit('/#/login'); | ||
| cy.register(user.email, user.username, user.password); | ||
| cy.getByDataCy('email-sign-in').type(user.email); |
There was a problem hiding this comment.
Selectors use data-cy attributes, but the requirements specify using data-qa attributes for all elements interacted with in tests. Update the application and selectors to use data-qa attributes.
| cy.visit('/#/login'); | ||
| cy.register(user.email, user.username, user.password); | ||
| cy.getByDataCy('email-sign-in').type(user.email); | ||
| cy.getByDataCy('password-sign-in').type(user.password); |
There was a problem hiding this comment.
Selectors use data-cy attributes, but the requirements specify using data-qa attributes for all elements interacted with in tests. Update the application and selectors to use data-qa attributes.
| cy.register(user.email, user.username, user.password); | ||
| cy.getByDataCy('email-sign-in').type(user.email); | ||
| cy.getByDataCy('password-sign-in').type(user.password); | ||
| cy.getByDataCy('sign-in-btn').click(); |
There was a problem hiding this comment.
Selectors use data-cy attributes, but the requirements specify using data-qa attributes for all elements interacted with in tests. Update the application and selectors to use data-qa attributes.
| cy.getByDataCy('email-sign-in').type(user.email); | ||
| cy.getByDataCy('password-sign-in').type(user.password); | ||
| cy.getByDataCy('sign-in-btn').click(); | ||
| cy.getByDataCy('username-link').should('contain', user.username); |
There was a problem hiding this comment.
Selectors use data-cy attributes, but the requirements specify using data-qa attributes for all elements interacted with in tests. Update the application and selectors to use data-qa attributes.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
Something went wrong while processing the review.
[CODE: GAE]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
You’ve made good progress with structure (shared PageObject base, helpers, Faker in places), but several critical checklist requirements are not met and will block approval.
Critical blockers to fix:
- Use of
data-qaselectors is required everywhere. Multiple files rely on placeholders/class names/text ordata-cyinstead ofdata-qa:cypress/e2e/article.cy.js(e.g., selectors for inputs/buttons and article elements) — refactor to usedata-qaand update the app to expose these attributes.cypress/e2e/signIn.cy.jsnegative test selectsul.error-messages→ add adata-qato the error element and use it.cypress/e2e/user.cy.jsandcypress/support/commands.jsusedata-cy/getByDataCy→ replace withdata-qa/getByQaand update all usages.- Page Objects
home.pageObject.js,signIn.pageObject.js,signUp.pageObject.js,user.pageObject.js— replace all non-data-qaselectors withgetByQatargetingdata-qaattributes.
- Page Object Model not enforced in
cypress/e2e/article.cy.js. Move all selectors and UI actions into POM classes undercypress/support/pagesand call only POM methods from the spec. - Missing required settings flows:
cypress/e2e/settings.cy.jscurrently tests sign-up. Implement settings tests (update bio, username, email, password) here, and keep sign-up tests insignUp.cy.js. - Hardcoded data and test coupling in article tests. Use Faker/helper-generated unique data per test and ensure isolation (each test creates its own article in a
beforeEachor within the test). Add an assertion that deleted articles are no longer reachable.
Minor improvements:
- Expand assertions for persistence and edge cases (e.g., verify settings changes persist after reload; verify article deletion 404/redirect).
Once you switch fully to data-qa, refactor to POM, add the settings flows, and ensure isolation/unique data, this suite should meet the requirements. Keep going—your structure is close; aligning selectors and coverage will get you over the line.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| cy.task('db:clear'); | ||
| cy.visit('/#/login'); | ||
|
|
||
| cy.get('input[placeholder="Email"]').type(user.email); |
There was a problem hiding this comment.
Selectors must use data-qa attributes for all interacted elements, as required by the checklist. Update the app to add data-qa attributes and update selectors here accordingly.
| cy.visit('/#/login'); | ||
|
|
||
| cy.get('input[placeholder="Email"]').type(user.email); | ||
| cy.get('input[placeholder="Password"]').type(user.password); |
There was a problem hiding this comment.
Selectors must use data-qa attributes for all interacted elements, as required by the checklist. Update the app to add data-qa attributes and update selectors here accordingly.
|
|
||
| cy.get('input[placeholder="Email"]').type(user.email); | ||
| cy.get('input[placeholder="Password"]').type(user.password); | ||
| cy.get('button').contains('Sign in').click(); |
There was a problem hiding this comment.
Selectors must use data-qa attributes for all interacted elements, as required by the checklist. Update the app to add data-qa attributes and update selectors here accordingly.
| it('should create a new article', () => { | ||
| cy.visit('/#/editor'); | ||
|
|
||
| cy.get('input[placeholder="Article Title"]').type('Test Article'); |
There was a problem hiding this comment.
Direct use of Cypress selectors and commands in the test spec violates the requirement to use the Page Object Model (POM). Refactor to use POM classes for all page interactions.
| cy.visit('/#/editor'); | ||
|
|
||
| cy.get('input[placeholder="Article Title"]').type('Test Article'); | ||
| cy.get('input[placeholder="What\'s this article about?"]') |
There was a problem hiding this comment.
Direct use of Cypress selectors and commands in the test spec violates the requirement to use the Page Object Model (POM). Refactor to use POM classes for all page interactions.
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.get('input[placeholder="Password"]'); |
There was a problem hiding this comment.
Selectors in this Page Object do not use data-qa attributes as required by the checklist. Update the app to add data-qa attributes to all relevant elements and update selectors here to use them.
| } | ||
|
|
||
| get signUpBtn() { | ||
| return cy.get('button').contains('Sign up'); |
There was a problem hiding this comment.
Selectors in this Page Object do not use data-qa attributes as required by the checklist. Update the app to add data-qa attributes to all relevant elements and update selectors here to use them.
| } | ||
|
|
||
| get errorMessages() { | ||
| return cy.get('ul.error-messages'); |
There was a problem hiding this comment.
Selectors in this Page Object do not use data-qa attributes as required by the checklist. Update the app to add data-qa attributes to all relevant elements and update selectors here to use them.
| } | ||
|
|
||
| assertSignedUp(username) { | ||
| cy.contains('a.nav-link', username).should('exist'); |
There was a problem hiding this comment.
Selectors in this Page Object do not use data-qa attributes as required by the checklist. Update the app to add data-qa attributes to all relevant elements and update selectors here to use them.
| } | ||
|
|
||
| get followButton() { | ||
| return cy.get('button.btn'); |
There was a problem hiding this comment.
Selectors in this Page Object do not use data-qa attributes as required by the checklist. Update the app to add data-qa attributes to all relevant elements and update selectors here to use them.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job structuring a comprehensive Cypress suite with clear POM usage, faker-based data, and consistent isolation/reset strategies. Approving for educational progression while noting a few learning opportunities to strengthen reliability and coverage.
What you did well:
- Solid POM architecture with
data-qaselectors and reusable helpers. - Broad scenario coverage across sign up/in, settings updates, articles, and follow/unfollow.
- Good test isolation with DB resets and unique data generation.
Key improvements to address next:
- cypress/e2e/article.cy.js: Ensure edit/delete tests don’t depend on earlier tests; create an article within each test or
beforeEach. After deletion, enhanceassertArticleNotExistto verify it’s not listed and direct URL returns 404 (Req #3.3, #3.6). - cypress/support/pages/article.pageObject.js: Implement missing methods referenced by specs using only
data-qa:createArticle,assertArticleExists,editFirstArticle,assertArticleBody,getFirstArticleTitle,deleteFirstArticle,assertArticleNotExist(Req #2.2, #3.3). - cypress/e2e/settings.cy.js and cypress/support/pages/settings.pageObject.js: Add
updateBio/Username/Email/Passwordand corresponding assertions. Verify persistence after reload/re-login; for password, confirm login works with new and fails with old (Req #3.3). - cypress/support/pages/user.pageObject.js: For follow/unfollow, also assert follower count changes if exposed (Req #3.3).
Tip: If any UI element lacks a
data-qaattribute, add it in the app and keep selectors strictlydata-qa-based.
You’re very close—tightening these assertions and completing the POM methods will make the suite robust and production-ready. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| }); | ||
|
|
||
| it('should be deleted using Delete button', () => { | ||
| it('should edit an article', () => { |
There was a problem hiding this comment.
The edit and delete tests rely on an article created in a previous test. For proper test isolation (requirement #3.6), each test should create its own article before editing or deleting. Consider creating a new article within each test or in a beforeEach specific to those tests.
| const title = articlePage.getFirstArticleTitle(); | ||
| articlePage.deleteFirstArticle(); | ||
| articlePage.assertArticleNotExist(title); | ||
| }); |
There was a problem hiding this comment.
The assertion for article deletion should confirm that the article is truly gone (not listed and/or 404 on direct access), as required by #3.3. Ensure that assertArticleNotExist checks for both absence in the list and inaccessibility by URL if possible.
| it('should update password', () => { | ||
| const newPassword = 'NewPass123!'; | ||
| settingsPage.updatePassword(newPassword); | ||
| settingsPage.assertPasswordUpdated(); |
There was a problem hiding this comment.
The assertion for password update should verify that the user can log in with the new password and not with the old one, as required by the checklist. Ensure that assertPasswordUpdated covers this persistence and re-authentication check.
| const newEmail = faker.internet.email(); | ||
| settingsPage.updateEmail(newEmail); | ||
| settingsPage.assertEmail(newEmail); | ||
| }); |
There was a problem hiding this comment.
For username and email updates, the test should verify that the changes persist after a page reload or after logging out and back in. Ensure that assertUsername and assertEmail cover this persistence check as required by the checklist.
| userPage.assertFollowed(); | ||
|
|
||
| userPage.unfollow(); | ||
| userPage.assertUnfollowed(); |
There was a problem hiding this comment.
The assertion for follow/unfollow should verify both the button state and the follower count where applicable, as required by the checklist. Ensure that assertFollowed and assertUnfollowed cover these aspects.
| clickPublish() { this.publishBtn.click(); } | ||
| clickEdit() { this.editBtn.click(); } | ||
| clickDelete() { this.deleteBtn.click(); } | ||
| } |
There was a problem hiding this comment.
This class is missing several methods referenced in the article test spec, such as createArticle, assertArticleExists, editFirstArticle, assertArticleBody, getFirstArticleTitle, deleteFirstArticle, and assertArticleNotExist. These are required for the tests to work and to meet the scenario coverage requirements. Ensure these methods are implemented here, using only data-qa selectors.
| typeEmail(email) { this.emailField.clear().type(email); } | ||
| typePassword(password) { this.passwordField.clear().type(password); } | ||
| clickUpdate() { this.updateBtn.click(); } | ||
| } |
There was a problem hiding this comment.
This class is missing several methods referenced in the settings test spec, such as updateBio, updateUsername, updateEmail, updatePassword, assertBio, assertUsername, assertEmail, and assertPasswordUpdated. These are required for the tests to work and to meet the scenario coverage requirements. Ensure these methods are implemented here, using only data-qa selectors.
| follow() { this.followButton.contains('Follow').click(); } | ||
| unfollow() { this.followButton.contains('Unfollow').click(); } | ||
| assertFollowed() { this.followButton.should('contain', 'Unfollow'); } | ||
| assertUnfollowed() { this.followButton.should('contain', 'Follow'); } |
There was a problem hiding this comment.
The assertion methods for follow/unfollow only check the button text. The requirements specify that the follower count should also be checked where applicable. If the UI exposes a follower count, add assertions to verify that the count increases/decreases appropriately.
2 participants
No description provided.