solution#339
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are critical blockers that will prevent the tests from running reliably and violate core checklist items.
Critical issues to fix:
- cypress/support/pages/signIn.pageObject.js: the
visit()method callsthis.visit(this.url)(infinite recursion). Replace withsuper.visit(this.url)orcy.visit(this.url)so sign-in tests can execute. This is a runtime blocker. - cypress/e2e/signIn.cy.js: user creation happens inside the first test, making the second test order-dependent. Move registration to
before/beforeEachor generate/register a user per test to ensure independence and determinism. - cypress/e2e/article.cy.js: registering the same user in
beforeEachafter only clearing DB once inbeforewill cause duplicate user failures. Either move registration tobefore, clear DB inbeforeEach, or generate a fresh unique user per test. Also remove redundant double navigation (cy.visit('/')followed bysignInPage.visit()), and verify whethercy.registeralready logs in to avoid duplicate sign-in steps. - cypress/e2e/signUp.cy.js: after
clickSignUpBtn()the test immediately uses SignInPage inputs without navigating to/login. Either navigate withsignInPage.visit()or remove the sign-in steps and assert logged-in state after sign-up. - cypress/e2e/user.cy.js: follow/unfollow tests do not create the target profile or authenticate the acting user. Create both users (faker or a task), register/login the actor in setup, then visit the target profile before clicking follow.
Minor improvements (non-blocking): standardize visitPage() usage to avoid overriding visit, rename assertAllert to assertAlert, clear inputs in Article.create, improve absence assertion in Article POM, and add assertions for settings bio/email/password updates. You’ve set up a solid POM structure and data-qa usage—once the above blockers are addressed, the suite should be stable. Keep going; you’re close to full compliance.
✨ 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
| let article; | ||
|
|
||
| before(() => { | ||
| cy.task('db:clear'); |
There was a problem hiding this comment.
You clear the DB once at the start (good), but it’s only called in the suite-level before(). The current test registers the same user in every beforeEach which will likely fail on the second test due to duplicate user/email constraints. The checklist requires "Database must be cleared before tests start to ensure a clean state for test runs", but to avoid duplicate-registration errors you must either:
- Move user registration into before() so registration runs only once, or
- Clear the DB beforeEach instead of only before(), or
- Generate & register a new unique user for each test (using faker) so registration won’t collide.
Please update the lifecycle to ensure registration won’t fail on subsequent tests.
| cy.task('db:clear'); | ||
| }); | ||
| cy.visit('/'); | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
Registering the same user inside beforeEach will attempt to create the same account before every test. This will likely cause failures (duplicate account/email) on test 2+. Per the requirements you must ensure deterministic runs. Consider moving this registration into the suite-level before() (so user is created once after the DB clear), or generate a fresh user for each test and register that user, or call db:clear beforeEach instead.
| cy.visit('/'); | ||
| cy.register(user.email, user.username, user.password); | ||
|
|
||
| it('should be created using New Article form', () => { | ||
| signInPage.visit(); |
There was a problem hiding this comment.
You call cy.visit('/') and immediately after signInPage.visit(). This causes two navigations in a row — redundant and can slow or confuse tests. Either remove the initial cy.visit('/') or the subsequent signInPage.visit() depending on what signInPage.visit() does. Also verify whether cy.register(...) already authenticates the user; if it does, the explicit sign-in steps may be unnecessary.
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
You perform an explicit sign-in sequence (signInPage.typeEmail, typePassword, clickSignInBtn) after calling cy.register. If cy.register already creates and signs the user in (common pattern), these steps are redundant and risk race conditions. Confirm the behavior of cy.register and remove redundant sign-in if register handles authentication, or keep sign-in and stop registering every test (see earlier comments).
| /// <reference types='../support' /> | ||
| /// <reference types="cypress" /> | ||
|
|
||
| import ArticlePageObject from '../support/pages/article.pageObject'; |
There was a problem hiding this comment.
You import and instantiate page objects which is correct and aligns with the POM requirement. Make sure the project also includes the centralized PageObject.js (shared elements) and that the POM classes live under cypress/support/pages (the task requires this structure). If these files are missing or not used by other tests, update the project structure to match the checklist.
| let user; | ||
|
|
||
| before(() => { | ||
| cy.task('db:clear'); |
There was a problem hiding this comment.
The tests call cy.task('db:clear') only once in before(). The task requirements ask to "Clear all application data from the database before running tests" to avoid flaky results. Consider running DB clear in beforeEach() (or otherwise ensuring a clean state before each test) so tests are fully isolated and deterministic. See the task description for the requirement to clear data before tests .
| it('should update bio', () => { | ||
| const newBio = faker.lorem.sentence(); | ||
| settingsPage.visitPage(); | ||
| settingsPage.updateBio(newBio); |
There was a problem hiding this comment.
The should update bio test updates the bio but does not assert that the change was applied in the UI. You should add an assertion (for example, a method like settingsPage.assertBio(newBio) or check the profile page) to verify the update is visible to the user. The current settings page object (cypress/support/pages/settings.pageObject.js) does not provide an assertBio helper, so either extend the POM or add an explicit assertion here .
| it('should update email', () => { | ||
| const newEmail = faker.internet.email(); | ||
| settingsPage.visitPage(); | ||
| settingsPage.updateEmail(newEmail); |
There was a problem hiding this comment.
The should update email test changes the email but doesn't verify it. Add an assertion to confirm the email was updated (for example, reloading the settings page and checking the email input value, or re-authenticating with the new email). The SettingsPageObject doesn't include an assertEmail — consider adding one to the POM to keep tests readable and consistent with the username assertion pattern .
| it('should update password', () => { | ||
| const newPassword = faker.internet.password(8); | ||
| settingsPage.visitPage(); | ||
| settingsPage.updatePassword(newPassword); |
There was a problem hiding this comment.
The should update password test updates the password but lacks verification. Updating password should be validated by re-authenticating with the new password (log out and log in using user.email and the new password) or by asserting any visible confirmation. Add an explicit verification step or a POM helper for this. The task list explicitly includes "updating password" which implies you must verify that the new password works in the UI flow .
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generateUser) => { | ||
| user = generateUser; | ||
| cy.task('generateUser').then((generatedUser) => { |
There was a problem hiding this comment.
The test uses cy.task('generateUser') here to obtain user data. Ensure the plugin task 'generateUser' is implemented in Cypress plugins and that it returns an object with email, username, and password fields. Also note: generating fake user data does not necessarily create the user in the app DB — you still need to register/create the user before trying to sign in. As an alternative pattern, see how other tests set up user state (for example signUp.cy.js uses an explicit registration flow / setup).
|
|
||
| it('should provide an ability to log in with existing credentials', () => { | ||
| signInPage.visit(); | ||
| it('should log in with correct credentials', () => { |
There was a problem hiding this comment.
Be careful with test setup here: the first test performs cy.register(...) (inside the test body). That means the second test relies on the first test having run and created the user — this introduces a test-order dependency. Move user registration into the before or beforeEach hook so each test has the required user state and tests can be run independently.
| it('should log in with correct credentials', () => { | ||
| cy.register(user.email, user.username, user.password); | ||
|
|
||
| signInPage.visit(); |
There was a problem hiding this comment.
Calling signInPage.visit() will invoke the SignInPageObject.visit() method. The current SignInPageObject defines visit() as this.visit(this.url) which recursively calls itself and will cause a stack overflow / runtime error. Update the page object to call the base implementation (e.g. super.visit(this.url) or cy.visit(this.url)) or rename the method (e.g. visitPage) to avoid overriding the base visit method. See the SignInPageObject implementation for the problematic code.
|
|
||
| it('should not provide an ability to log in with wrong credentials', () => { | ||
| it('should not log in with wrong password', () => { | ||
| signInPage.visit(); |
There was a problem hiding this comment.
Same issue as above: this second call to signInPage.visit() (in the negative test) will also hit the recursive visit() in the page object. Fix the page object before running these tests. After fixing, consider using a consistent method name across page objects (for example visitPage() used in other POMs) to avoid accidental overrides.
| signInPage.clickSignInBtn(); | ||
|
|
||
| homePage.assertHeaderContainUsername(user.username); | ||
| cy.contains(user.username).should('be.visible'); |
There was a problem hiding this comment.
This assertion checks for the user's name by text. It's better to target a stable UI selector (the task requires the use of data-qa attributes). If the app provides a data-qa for the profile link (used in other POMs as [data-qa="profile-link"]), prefer cy.get('[data-qa="profile-link"]').contains(user.username).should('be.visible') or use the page object method for asserting username presence for more robust checks.
| signInPage.typePassword('wrongPassword'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.contains('email or password is invalid').should('be.visible'); |
There was a problem hiding this comment.
The negative-path assertion checks the visible error message by text. The task requires adding data-qa attributes to elements used in tests — prefer asserting error messages via a data-qa selector (or a POM method that checks the error container) to make the check less fragile and to align with task requirements. See the project description for the requirement to add data-qa attributes.
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
After signUpPage.clickSignUpBtn() you immediately call SignInPageObject methods without navigating to the login page. signInPage.typeEmail / typePassword / clickSignInBtn will try to find elements that exist only on the /login page and will fail if the app stayed on another page after registration. Fix options:
- If you intend to re-login via the login page, call
signInPage.visit()before typing (same pattern used in thesignIn.cy.jstest). See howsignInPage.visit()is used in the project's sign-in test for reference . - If the app auto-logs-in after successful sign-up (common behaviour), remove the explicit sign-in steps and assert the logged-in state directly (for example assert the username is visible). This keeps the test focused on verifying the sign-up UI flow.
| signUpPage.typeUsername(user.username); | ||
| signUpPage.typeEmail(user.email); | ||
| signUpPage.typePassword(user.password); | ||
| signUpPage.clickSignUpBtn(); |
There was a problem hiding this comment.
This line triggers the sign-up action in the UI. Right after this you attempt a sign-in flow (without navigating). Either (1) replace the subsequent sign-in steps with a direct assertion that the user is logged in (e.g. assert the username/profile link is visible), or (2) explicitly navigate to the login page before using the SignInPageObject. Leaving both actions (signup + implicit signin commands) as-is is likely to cause a failing/flaky test and does not match the UI-driven flow expectations in the task description.
| describe('Follow/unfollow button', () => { | ||
| before(() => { | ||
|
|
||
| cy.task('db:clear'); |
There was a problem hiding this comment.
You clear the DB here which is required (good). However, clearing alone is not sufficient: you must create the users required for the follow/unfollow flows and sign in the acting user before interacting with the follow button. Add tasks or API calls to create (a) the user who will perform follow/unfollow and (b) the target user (JaneDoe) to be followed. Alternatively use the existing cy.task('generateUser') in other tests and cy.register(...) to ensure the actor user is registered and logged in. This is missing from the test setup and is required by the task (follow/unfollow must be verifiable in the UI).
| it.skip('should be able to follow the another user', () => { | ||
| it('should provide an ability to follow another user', () => { | ||
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); |
There was a problem hiding this comment.
You visit the profile page for JaneDoe but there is no code creating JaneDoe on the backend. The test assumes the profile exists. Ensure the target user is created (via a plugin task or API request) before visiting /profile/JaneDoe. Also prefer generating usernames via faker to avoid hard-coding and conflicts.
| it('should provide an ability to follow another user', () => { | ||
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); | ||
| userPage.clickFollow(); |
There was a problem hiding this comment.
Clicking the follow button requires the test to be authenticated. There is no registration or sign-in step for the acting user in this test. Add registration (e.g. cy.register(...)) and sign-in steps (or perform sign-in via API) in a before/beforeEach hook so the actor is logged in before calling clickFollow(); otherwise the click may have no effect or the UI may redirect to login.
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); | ||
| userPage.clickFollow(); | ||
| userPage.assertFollowButtonText('Unfollow'); |
There was a problem hiding this comment.
The assertion assertFollowButtonText('Unfollow') is fine conceptually, but it's fragile if the follow flow hasn't completed or if the button text has whitespace. Consider asserting with .should('contain.text', 'Unfollow') or, better, expose a POM method that waits for the API call / UI state change and then asserts the text. Also ensure the button uses the data-qa attribute as required by the task (your POM uses it which is good).
|
|
||
| it('should provide an ability to unfollow the user', () => { | ||
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); |
There was a problem hiding this comment.
The second test (unfollow) repeats visiting JaneDoe without ensuring JaneDoe exists or the test user is signed in. Tests should be independent and deterministic. Either create the required users and log in the actor in a beforeEach or at least in before register the actor and create the target. Right now the test may pass/fail depending on external state.
| it('should provide an ability to unfollow the user', () => { | ||
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); | ||
| userPage.clickFollow(); // jeśli jest już "Unfollow", klikamy, żeby odfollowować |
There was a problem hiding this comment.
This line contains an inline Polish comment — it's fine to leave comments in your language, but keep test code and comments clear for reviewers. Functionally: clicking clickFollow() again to toggle follow/unfollow is okay only if you first ensure the user is currently followed; prefer to make the initial state explicit (e.g., assert initial text is 'Follow', click, assert 'Unfollow', click, assert 'Follow'). That makes the test intent explicit and deterministic.
| visit(url) { | ||
| cy.visit(url || this.url); |
There was a problem hiding this comment.
The visit method implementation is correct (it calls cy.visit(url || this.url)) and has no syntax errors. However, because subclasses may implement their own visit() names (for example visit() in SignInPageObject), calling this.visit(this.url) from a subclass will recursively call the subclass method and cause a stack overflow. To avoid this: either (a) instruct subclasses to call the base method using super.visit(this.url), or (b) rename this method to something less likely to be overridden (for example visitPage(url) or goTo(url)). This change will prevent the recursion problem observed when POM subclasses implement their own visit methods.
| assertAllert(alertMessage) { | ||
| cy.on('window:alert', (alert) => { | ||
| expect(alert).to.eq(alertMessage); |
There was a problem hiding this comment.
Method name assertAllert appears to have a typo (should be assertAlert). Rename it for clarity and to avoid confusion. Also note that cy.on('window:alert', ...) registers a global event handler — document this behaviour or ensure tests that rely on it expect that it will fire for the whole test run. If you want a chainable Cypress-style assertion, consider returning the chain or creating a Cypress custom command that asserts alert text in a more Cypress-native way.
| this.titleField.type(title); | ||
| this.descriptionField.type(description); | ||
| this.bodyField.type(body); | ||
| this.publishBtn.click(); |
There was a problem hiding this comment.
createArticle types into inputs without clearing them first. Other page objects (and best practices) use .clear().type(...) to ensure deterministic input state. Consider using this.titleField.clear().type(title) (and same for description/body) to avoid flakiness if inputs are pre-filled or retain previous values.
| editArticle({ title, description, body }) { | ||
| this.editBtn.click(); | ||
| if (title) { | ||
| this.titleField.clear().type(title); | ||
| } | ||
| if (description) { | ||
| this.descriptionField.clear().type(description); | ||
| } | ||
| if (body) { | ||
| this.bodyField.clear().type(body); | ||
| } | ||
| this.publishBtn.click(); |
There was a problem hiding this comment.
editArticle already clears fields before typing which is good for determinism — keep that pattern consistent across create/edit flows. If create flow intentionally avoids .clear() because inputs are empty on /new-article, still consider adding .clear() to make the method robust to UI changes.
| assertArticleNotPresent(title) { | ||
| cy.contains('[data-qa="article-title-display"]', title).should('not.exist'); |
There was a problem hiding this comment.
assertArticleNotPresent uses cy.contains('[data-qa="article-title-display"]', title).should('not.exist'). This is fragile because cy.contains() will fail if it cannot find a matching element (it times out trying to find it) before the should('not.exist') assertion can run. To assert absence reliably, use a pattern that checks container does not contain the text (for example cy.get('body').should('not.contain', title) or inspect the collection of title elements and assert the list does not include the title). This will avoid false failures when the element is absent from the DOM.
| @@ -0,0 +1,63 @@ | |||
| import PageObject from './PageObject'; | |||
There was a problem hiding this comment.
Import is correct and available — PageObject is used as the shared base class as required by the advanced checklist. No change required here.
| visitPage() { | ||
| this.visit(this.url); |
There was a problem hiding this comment.
visitPage() calls this.visit(this.url) which relies on the base PageObject.visit implementation. This is fine, but consider naming the visit method consistently across POMs (many other POMs use visitPage() while the SignIn POM incorrectly overrides visit() causing recursion). Using visitPage() prevents accidental overriding of the base visit method and avoids the recursion problem seen elsewhere.
| updateUsername(username) { | ||
| this.usernameField.clear().type(username); | ||
| this.updateBtn.click(); | ||
| } | ||
|
|
||
| updateBio(bio) { | ||
| this.bioField.clear().type(bio); | ||
| this.updateBtn.click(); | ||
| } | ||
|
|
||
| updateEmail(email) { | ||
| this.emailField.clear().type(email); | ||
| this.updateBtn.click(); |
There was a problem hiding this comment.
Update helper methods (updateUsername/updateBio/updateEmail/updatePassword) perform the action but do not provide an assertion or return a promise for chaining. Consider adding helper assertion methods (for example assertEmailUpdated, assertBioUpdated) or returning this/the resulting element so tests can immediately assert success from the POM. The task requires tests that validate updates — adding assertion helpers here will make tests clearer and less brittle.
| assertUsername(username) { | ||
| cy.contains('[data-qa="profile-link"]', username).should('be.visible'); |
There was a problem hiding this comment.
assertUsername uses cy.contains('[data-qa="profile-link"]', username) which is acceptable. For a more precise assertion you can use cy.get('[data-qa="profile-link"]').should('contain', username) or add an assertProfileLink helper in POM to centralize the check. This helps avoid false positives if other elements contain the same text.
| visit() { | ||
| this.visit(this.url); |
There was a problem hiding this comment.
This visit() implementation calls itself (this.visit(this.url)), causing infinite recursion at runtime. Replace with a call to the base class or to Cypress directly. For example:
super.visit(this.url);// calls PageObject.visit(url)
orcy.visit(this.url);// direct Cypress call
Usingsuper.visit(this.url)is preferable sincePageObjectalready centralizes visit behavior. After changing this, re-run tests because several test files callsignInPage.visit()and depend on correct behaviour.
| visitPage() { | ||
| this.visit(this.url); |
There was a problem hiding this comment.
visitPage() correctly delegates to the shared visit method via this.visit(this.url). This is fine, but other page objects in the project sometimes use visit() directly (rather than visitPage()). Consider standardizing the navigation method name across POM classes (either visit() or visitPage()) to avoid confusion and accidental overrides like the recursive visit() issue seen in other POMs.
| clickSignUpBtn() { | ||
| this.signUpBtn.click(); |
There was a problem hiding this comment.
Before clicking the sign-up button it’s more robust to assert the button is visible/enabled. I suggest using a short guard like this.signUpBtn.should('be.visible').click() to reduce flakiness if the element is not yet ready in the UI.
| this.usernameField.clear().type(username); | ||
| } | ||
|
|
||
| typeEmail(email) { | ||
| this.emailField.clear().type(email); | ||
| } | ||
|
|
||
| typePassword(password) { | ||
| this.passwordField.clear().type(password); |
There was a problem hiding this comment.
Good use of .clear().type(...) for all input fields which makes typing deterministic. Keep this pattern — it prevents issues when fields are prefilled or stale values remain.
| } | ||
|
|
||
| export default SignUpPageObject; |
There was a problem hiding this comment.
Consider adding small helper assertion methods to this POM for common verifications (for example assertSignedUp() or assertSignUpError(message)) so the test files stay concise and checks can use data-qa selectors consistently rather than raw cy.contains(...) calls in tests. This will improve readability and align with the task expectation to use POMs for UI interactions.
| visitProfile(username) { | ||
| this.visit(`/profile/${username}`); |
There was a problem hiding this comment.
visitProfile constructs a profile URL and calls the shared visit method. This is fine and will navigate to /profile/<username>. Consider documenting whether the app requires the user to be logged-in before visiting a profile (tests should ensure the necessary auth state elsewhere).
| clickFollow() { | ||
| this.followBtn.should('be.visible').click(); |
There was a problem hiding this comment.
clickFollow() uses .should('be.visible').click() which is fine for interacting with the button. If the follow/unfollow action is asynchronous (server call), consider adding a subsequent assertion or waiting for a specific UI change (e.g., toggled button text or a network alias) to ensure the click completed before continuing the test. This will reduce flakiness in tests that rely on immediate follow state changes.
| assertFollowButtonText(expectedText) { | ||
| this.followBtn.should('have.text', expectedText); |
There was a problem hiding this comment.
assertFollowButtonText uses this.followBtn.should('have.text', expectedText). Using have.text requires an exact text match (including whitespace and child elements). For a more robust assertion prefer one of:
this.followBtn.should('contain.text', expectedText)(partial match, ignores extra whitespace/children)this.followBtn.invoke('text').then(t => expect(t.trim()).to.eq(expectedText))(exact trimmed match)
This change will make the assertion less likely to fail due to minor markup/whitespace differences and aligns with stable UI checks required by the task.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because there are several critical blockers that will make the suite fail or be flaky and violate the determinism requirements.
Critical issues to fix:
- Duplicate user registration and non-deterministic setup: in
cypress/e2e/article.cy.jsandcypress/e2e/settings.cy.js,cy.task('db:clear')runs only inbefore, but the same user is registered inbeforeEach, causing duplicate-user errors and order dependence. Fix by either moving registration tobefore, clearing DB inbeforeEach, or generating a unique user per test with faker (align this across specs). - Order-dependent tests in
cypress/e2e/article.cy.js: edit/delete assume a previously created article. Make each test create its own article (or group create→edit→delete into one flow test) to ensure independence. - Missing/incorrect auth flow:
cypress/e2e/user.cy.jsdoes not create an actor or target user and attempts to follow while unauthenticated, plus uses a hard-coded username that may not exist. Create both users with faker, sign in the actor inbeforeEach, ensure the target exists, and set up the unfollow precondition. - Runtime breakages:
cypress/support/pages/settings.pageObject.jslackslogout()but tests callsettingsPage.logout(); add this method. Also,signIn.pageObjectexposesvisitPage()while tests callsignInPage.visit(); add avisit()alias. - Functional flow issues:
cypress/e2e/signUp.cy.jsuses sign-in steps immediately after clicking Sign Up without navigating or confirming auto-login. Either assert logged-in state directly after sign-up or navigate to/loginbefore typing. - Missing persistence assertions in settings: after updating bio/email/password in
cypress/e2e/settings.cy.js, add verifications (e.g., bio visible on profile, re-login with new password, sign in with new email).
Minor improvements (nice-to-have): add defensive .clear() in article creation, add convenience helpers (e.g., signIn(email, password)), strengthen error-message assertions to be less brittle. You’ve got solid POM structure and faker usage—addressing the above will make the suite reliable and compliant. 👍
✨ 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
| before(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| }); |
There was a problem hiding this comment.
You call cy.task('db:clear') only once in the before hook. However, the suite registers the same user inside beforeEach which will cause duplicate user registration errors when tests run in the same process. This violates the requirement that tests be deterministic and prepared/isolated. Fix options (pick one):
- Move user registration to the
beforehook (register once after db:clear), or - Clear the DB in
beforeEachso each test starts from a clean state, or - Generate and register a unique user per test (use faker) so registrations don't collide.
Reference: checklist requires clearing DB and deterministic setup.
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| }); | ||
| cy.visit('/'); | ||
| cy.register(user.email, user.username, user.password); | ||
|
|
||
| it('should be created using New Article form', () => { | ||
| signInPage.visit(); | ||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
The beforeEach flow performs redundant navigation and possibly redundant sign-in steps:
- You call
cy.visit('/')thensignInPage.visit()(duplicate navigation). - After
cy.register(user...)you still navigate to sign-in and call sign-in methods. Ifcy.registeralready auto-logs-in the user, these sign-in steps are unnecessary and will add flakiness. Confirm the behavior ofcy.registerand either remove the extra sign-in steps or the register call accordingly.
Also, registering the same user inbeforeEach(see previous comment) will produce duplicate user errors; combine the recommendations to fix both problems.
Required by checklist: ensure setup is deterministic and avoid redundant UI interactions.
| it('should create an article', () => { | ||
| articlePage.createArticle(article); | ||
| articlePage.assertArticlePresent(article.title); | ||
| }); | ||
|
|
||
| it('should be deleted using Delete button', () => { | ||
| it('should edit an article', () => { | ||
| const updatedArticle = { | ||
| title: article.title + ' Updated', | ||
| description: article.description + ' Updated', | ||
| body: article.body + ' Updated' | ||
| }; | ||
| articlePage.editArticle(updatedArticle); | ||
| articlePage.assertArticlePresent(updatedArticle.title); | ||
| article = updatedArticle; // update reference for delete test | ||
| }); | ||
|
|
||
| it('should delete an article', () => { | ||
| articlePage.deleteArticle(); | ||
| articlePage.assertArticleNotPresent(article.title); |
There was a problem hiding this comment.
The three tests for article creation, editing and deletion are order-dependent: the edit test assumes the article created in the first test exists, and the delete test assumes the edited article exists. This makes tests brittle if run individually or re-ordered. To comply with the checklist for deterministic e2e tests, make each test independent by either:
- Creating the article in the test itself or in a
beforeEachsetup (ensuring unique article data), or - Group the create->edit->delete steps into a single test that represents the full flow (if you intentionally want a flow test).
Also ensure the POM implementation clears inputs when editing to avoid concatenation (covered by the generalized checklist).
| before(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| }); |
There was a problem hiding this comment.
Issue: DB is cleared only once (before) but tests register the same user in beforeEach. This will cause duplicate user creation errors or order-dependent failures.
Checklist reference: tests must run deterministically and data must be prepared/isolated (clear DB or generate unique data per test).
Recommendation: either clear the DB in beforeEach(), or register the user once in before() and only sign in in beforeEach(), or generate a unique user per test (faker).
| }); | ||
|
|
||
| beforeEach(() => { | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
Problematic call: registering the same user inside beforeEach will attempt to create the same user repeatedly (duplicate user conflicts) because the DB is cleared only once in before().
Actionable options:
- Move
cy.register(...)to the before() (register once) and keep sign-in in beforeEach(), or - Clear DB in beforeEach() instead of before(), or
- Generate and register a fresh unique user per test using faker/helpers.
This is a HIGH-severity stability issue.
| signInPage.visit(); | ||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
Potential redundancy/fragility: after cy.register(...) the test navigates to and performs an explicit sign-in (signInPage.visit() + typeEmail/typePassword/clickSignInBtn). Confirm the behaviour of cy.register:
- If
cy.registeralready logs in the user, these sign-in steps are redundant and may cause unexpected navigation. - If it does not auto-login, keep them but consider moving sign-in into a single helper (or ensure POM visit method works reliably).
Also, note that if the SignIn page object has a known visit() bug (infinite recursion), these calls will fail — validate the SignIn POM implementation.
| settingsPage.visitPage(); | ||
| settingsPage.updateUsername(newUsername); | ||
| settingsPage.assertUsername(newUsername); | ||
| user.username = newUsername; |
There was a problem hiding this comment.
Risk: the test mutates the shared user object inside tests (user.username = newUsername). Mutating this shared object makes subsequent test setup (which re-uses user) order-dependent and brittle.
Recommendation: avoid mutating the original user used for registration unless you also reset the DB and re-register after mutation. Prefer generating a new user for flows that require a changed identity, or register once and then perform updates without re-registering.
| it('should update bio', () => { | ||
| const newBio = faker.lorem.sentence(); | ||
| settingsPage.visitPage(); | ||
| settingsPage.updateBio(newBio); |
There was a problem hiding this comment.
Missing verification: the bio update test calls settingsPage.updateBio(newBio) but does not assert that the bio persisted. The checklist requires asserting updates persisted (e.g., visible on the profile/settings page).
Action: add an assertion after update (for example, check that the settings form shows the updated bio or visit the user's profile and assert the bio is displayed).
| settingsPage.updateEmail(newEmail); | ||
| user.email = newEmail; |
There was a problem hiding this comment.
Missing verification: the email update test updates newEmail and mutates user.email, but there is no assertion that the new email persisted or that authentication behaves accordingly.
Action: after changing email, assert persistence (e.g., sign out and sign back in using the new email, or check the account settings/profile). Also be aware that changing the shared user.email affects subsequent registration/sign-in flows — ensure the test setup accounts for that.
| settingsPage.updatePassword(newPassword); | ||
| user.password = newPassword; |
There was a problem hiding this comment.
Missing verification (important): the password update test calls settingsPage.updatePassword(newPassword) but does not validate the change. The checklist explicitly expects updating password to be verified (e.g., re-login with the new password).
Action: after changing the password, explicitly log out, then attempt to sign in using the updated credentials to confirm the change. Update the shared user.password only if you plan to use it for subsequent sign-ins and registration flows.
| @@ -12,23 +13,27 @@ describe('Sign In page', () => { | |||
|
|
|||
| before(() => { | |||
| cy.task('db:clear'); | |||
There was a problem hiding this comment.
You call cy.task('db:clear') once in before() which clears data before the spec run — that satisfies the “clear DB before tests” requirement, but because you only clear once, later specs or tests that create users may still cause collisions if they run in the same Cypress process. Consider one of the following to make test runs deterministic:
- Clear DB in a
beforeEach()so every test starts from a clean DB, or - Continue clearing once but ensure every test registers unique users (e.g., generate a fresh user per test), or
- Use a dedicated reset task that is executed once per spec run at the CI level.
Which approach you choose should be consistent across the suite. This is relevant to checklist items about deterministic setup and data isolation.
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
You generate and register the user inside before() (good to have a pre-created user), however this introduces a risk if cy.register(...) automatically logs the user in. If the user is logged-in after registration the first test may be affected (or redundant), and the second test (negative sign-in) may not be able to assert an error because the app might redirect away from the sign-in page. To ensure test independence either:
- Register a new unique user per test in
beforeEach(), or - Ensure you explicitly log out (clear cookies/localStorage or call a
cy.task('logout')) before each test (e.g., inbeforeEach()), or - Modify
cy.registerto not auto-login and return only after the user is created.
Also avoid relying on shared mutableuserbetween tests if you plan to run tests in parallel or reorder them.
| cy.register(user.email, user.username, user.password); | ||
|
|
||
| it('should log in with correct credentials', () => { | ||
| signInPage.visitPage(); |
There was a problem hiding this comment.
Both tests call signInPage.visitPage() but there is no guarantee the app is logged out before this call. If the app is already authenticated it may redirect from the sign-in URL to the home page and your test actions/assertions will fail. Add a step to ensure the app is logged out before visiting the sign-in page (for example, cy.clearCookies()/cy.clearLocalStorage() or a cy.task('logout')) or move registration into beforeEach() so each test controls auth state explicitly.
| signInPage.typePassword('wrongPassword'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.get('[data-qa="error-messages"]').should('contain.text', 'email or password is invalid'); |
There was a problem hiding this comment.
The negative test assertion uses the exact text 'email or password is invalid'. This is brittle to small wording or casing changes in the app. Prefer one of the following to make the assertion more robust:
- Assert the element exists and contains a substring (e.g.,
.should('contain.text', 'invalid')), - Use a case-insensitive regex, or
- Prefer checking the presence of the error container (
[data-qa="error-messages"]) and that it is visible, then assert the important part of the message.
Also ensure the negative case is exercised from a logged-out state (see previous comments).
| signUpPage.typeUsername(user.username); | ||
| signUpPage.typeEmail(user.email); | ||
| signUpPage.typePassword(user.password); | ||
| signUpPage.clickSignUpBtn(); |
There was a problem hiding this comment.
After clicking the Sign Up button you immediately call sign-in methods without navigating to the login page or asserting the post-sign-up state. If your app auto-logs-in after a successful sign-up, the sign-in inputs won’t exist and the test will fail. Checklist relevance: tests must assert the expected outcome of the flow (sign-up should either log in the user or redirect to login). Consider one of the following fixes:
- If sign-up auto-logs-in: remove the explicit sign-in steps (lines 29–31) and assert the user is logged in (e.g.,
cy.contains(user.username).should('be.visible')— which you already have at line 33). - If sign-up redirects to the login page: call
signInPage.visitPage()(orcy.visit('/login')) beforesignInPage.typeEmail(...)so you interact with the login form on the correct page.
This is a HIGH severity functional issue that will make the test flaky/failing.
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
These lines type into the sign-in form but there is no navigation to the login page beforehand. Either navigate explicitly to the login page (e.g., signInPage.visitPage()), or remove these steps and assert that the user is already logged in after sign-up. Also double-check whether signUpPage.clickSignUpBtn() triggers an automatic login (if so, this block is redundant and fragile).
| signInPage.typePassword(user.password); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.contains(user.username).should('be.visible'); |
There was a problem hiding this comment.
You assert the username visibility here which is the correct way to verify that the user is logged in. If the app auto-logs-in after sign-up, keep this assertion and remove the sign-in steps above. If the app requires explicit login after sign-up, add a navigation step to the login page before typing credentials. Make the test reflect the actual app behavior to avoid order-dependent failures.
| }); | ||
|
|
||
| it('should show error for existing email', () => { | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
You use cy.register(...) in the second test to create a pre-existing user which is fine given cy.task('db:clear') runs in beforeEach. Please confirm the behavior of cy.register — does it create the user only or also log in the user? If cy.register logs in automatically and your intent is only to create the user, either ensure it just creates the record or explicitly log out afterwards. This prevents hidden state from affecting the sign-up negative case.
| describe('Follow/unfollow button', () => { | ||
| before(() => { | ||
|
|
||
| cy.task('db:clear'); |
There was a problem hiding this comment.
Clearing the DB only once in before() and then not creating any users will still leave tests without required data. Per the checklist, tests must prepare/isolate data deterministically. Either clear the DB in beforeEach() or (preferred) generate unique users per test and register them as part of setup.
Suggested approach: use cy.task('db:clear') in a beforeEach or generate/register users in beforeEach so each test starts with a known state.
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); |
There was a problem hiding this comment.
The test visits a profile for a hard-coded username 'JaneDoe' but there is no guarantee such a user exists after the DB clear. Tests must create the target profile as part of setup (e.g., cy.task('generateUser') or cy.register) using faker to avoid collisions. Hard-coded usernames make the suite non-deterministic and brittle (violates requirement to prepare/ isolate data).
| userPage.visitProfile(usernameToFollow); | ||
| userPage.clickFollow(); | ||
| userPage.assertFollowButtonText('Unfollow'); |
There was a problem hiding this comment.
There is no acting user created or authenticated before clicking follow. The checklist requires authenticating the acting user (user A) before visiting user B's profile and clicking follow. Add setup to create an actor user and perform sign-in (or generate a token and set auth) prior to calling userPage.visitProfile and userPage.clickFollow().
Example flow to implement in beforeEach (not code to paste, but steps):
- generate/register actor user (faker) and target user;
- sign in actor user (use POM signIn or API/token);
- visit target profile and perform follow/unfollow actions.
This is a HIGH-severity issue — tests will likely fail because unauthenticated users cannot follow others.
| it('should provide an ability to unfollow the user', () => { | ||
| const usernameToFollow = 'JaneDoe'; | ||
| userPage.visitProfile(usernameToFollow); | ||
| userPage.clickFollow(); | ||
| userPage.assertFollowButtonText('Follow'); |
There was a problem hiding this comment.
The unfollow test's logic/assertion appears incorrect or incomplete. It visits the profile and immediately calls clickFollow() and then asserts assertFollowButtonText('Follow'). Given a cleared DB and no prior follow relationship, clicking once will typically switch from Follow to Unfollow. To test unfollow you must ensure the actor is already following the target (either by performing a follow action in setup or by calling follow then click again to unfollow). Then click to unfollow and assert the button shows Follow.
Fix options:
- Setup: make the actor follow the target before the test then call
clickFollow()once and assertFollow, or - In the test: call
clickFollow()twice (first to follow, second to unfollow) and assert after the second click.
As written this test is order-dependent and will produce false negatives — HIGH severity.
| /// <reference types='cypress' /> | ||
| /// <reference types='../support' /> | ||
| import UserPageObject from '../support/pages/user.pageObject'; | ||
| const userPage = new UserPageObject(); |
There was a problem hiding this comment.
Recommendation: use faker to generate both the actor's and target's usernames/emails to avoid collisions and make tests independent. Use cy.task('generateUser') or a custom helper cy.register(email, username, password) so you can control creation and ensure the target profile exists. Also ensure userPage.visitProfile() accepts dynamic usernames rather than relying on hard-coded strings.
This ties back to checklist items: faker/custom helpers, deterministic data setup, and POM usage.
| visit(url) { | ||
| cy.visit(url || this.url); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a standardized visitPage() wrapper in the base class so page objects and tests can use a single method name (for example visitPage()), which avoids inconsistent overrides of visit() and the recursion pitfalls mentioned in the checklist. This helps satisfy the POM standardization requirement (see generalized task requirements). A simple implementation could call this.visit() internally.
| cy.visit(url || this.url); | ||
| } | ||
|
|
||
| assertAlert(alertMessage) { // poprawiona literówka |
There was a problem hiding this comment.
assertAlert currently listens for window:alert with cy.on. That works, but it can be flaky depending on timing. Consider using cy.window().then(win => cy.stub(win, 'alert').as('alert')) before the action that triggers the alert, then assert the stub was called with the expected message. This makes the assertion more deterministic and fits the requirement to have reliable checks in e2e tests.
| createArticle({ title, description, body }) { | ||
| this.visit(); | ||
| this.titleField.type(title); | ||
| this.descriptionField.type(description); | ||
| this.bodyField.type(body); |
There was a problem hiding this comment.
Be defensive when creating an article: clear fields before typing to avoid accidental concatenation if the page is re-used or if tests pre-fill fields. This makes the method more robust and reduces flakiness.
Suggestion: call .clear() on each field before .type(...) in createArticle.
| assertArticlePresent(title) { | ||
| cy.contains('[data-qa="article-title-display"]', title).should('be.visible'); |
There was a problem hiding this comment.
Consider making the assertion more explicit and robust. cy.contains(selector, title) works, but using cy.get(selector).contains(title) makes the intent clearer (get the element set first, then assert it contains the title). Also consider asserting URL or other article metadata after publish to ensure navigation succeeded and the correct article is shown.
| assertArticleNotPresent(title) { | ||
| cy.contains('[data-qa="article-title-display"]', title).should('not.exist'); |
There was a problem hiding this comment.
assertArticleNotPresent currently uses cy.contains(...).should('not.exist'). That is valid, but it can be flaky if the element is still in the DOM but hidden. Depending on the app behavior, consider asserting should('not.exist') (for removed nodes) or should('not.be.visible') (for hidden nodes). If you expect the article to be removed from the list, not.exist is fine — otherwise refine the assertion to match the UI behaviour.
| deleteArticle() { | ||
| this.deleteBtn.click(); |
There was a problem hiding this comment.
After calling deleteArticle() the method only clicks the delete button. Tests that use this POM should explicitly verify that the article was deleted (for example by calling assertArticleNotPresent or checking URL/navigation). Consider adding an optional parameter or returning a chainable Cypress command so callers can assert deletion immediately after the action.
| visitPage() { | ||
| super.visit(this.url); |
There was a problem hiding this comment.
Good: visitPage() uses super.visit(this.url), which avoids the infinite recursion problem (ensures the base PageObject visit implementation is used). Ensure PageObject.visit is implemented correctly to perform cy.visit() and any common wait logic.
| updateUsername(username) { | ||
| this.usernameField().clear().type(username); | ||
| this.updateBtn().click(); |
There was a problem hiding this comment.
The update methods correctly clear() then type() before clicking the update button — this addresses the checklist item about clearing inputs when editing (prevents concatenated values). Good practice here.
| assertEmail(email) { | ||
| this.emailField().should('have.value', email); | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: This Page Object is missing a logout() method. The test file cypress/e2e/settings.cy.js calls settingsPage.logout() (so the suite will throw TypeError: settingsPage.logout is not a function at runtime). Add a logout() method that clicks the logout control (use the app's data-qa selector, e.g. cy.get('[data-qa="logout-btn"]').click() if that matches your markup).
| updatePassword(password) { | ||
| this.passwordField().clear().type(password); | ||
| this.updateBtn().click(); | ||
| } |
There was a problem hiding this comment.
Recommendation: there is no helper to verify password updates. The checklist requires updating password and asserting it persisted. You should either add a POM helper to support the re-login flow (e.g., logout() + use SignIn POM) or add an assertPasswordChanged() helper. At minimum, tests must log out and re-login with the new password to confirm persistence.
|
|
||
| updateBio(bio) { | ||
| this.bioField().clear().type(bio); | ||
| this.updateBtn().click(); |
There was a problem hiding this comment.
Suggestion: add an optional assertSaved() or success-notification assertion so update methods can explicitly wait for and verify the server response (this makes tests less flaky than relying on implicit navigation/DOM state). For example, look up the data-qa attr for the success alert and assert it appears after updateBtn().click().
| visitPage() { | ||
| super.visit(this.url); |
There was a problem hiding this comment.
Blocking mismatch: this class exposes visitPage() but many tests in the suite call signInPage.visit() (for example cypress/e2e/settings.cy.js uses signInPage.visit()), so those tests will fail with TypeError: signInPage.visit is not a function at runtime.
Recommendation: add a visit() method (either as an alias that calls super.visit(this.url) or rename visitPage() to visit()) to standardize the visit API across all page objects and tests.
| clickSignInBtn() { | ||
| this.signInBtn | ||
| .click(); | ||
| this.signInBtn().should('be.visible').click(); |
There was a problem hiding this comment.
Suggestion: add a convenience method that performs the full sign-in flow (visit page, type email, type password, click). Tests currently repeat these steps. Example helper name: signIn(email, password) which would internally call this.visit()/this.typeEmail(email)/this.typePassword(password)/this.clickSignInBtn().
This reduces duplication in tests and centralizes any future changes to the sign-in flow in one place.
| clickSignInBtn() { | ||
| this.signInBtn | ||
| .click(); | ||
| this.signInBtn().should('be.visible').click(); |
There was a problem hiding this comment.
Small improvement: clickSignInBtn() asserts visibility before clicking which is good. Consider also adding a post-signin assertion helper (or returning a chainable) so tests can assert successful login state in a consistent way (e.g., check nav/profile link). This helps reduce flakiness when login triggers redirects or async loads.
| visitPage() { | ||
| super.visit(this.url); |
There was a problem hiding this comment.
Good: visitPage() delegates to super.visit(this.url), avoiding recursion and following the standardized POM visit pattern. This makes navigation consistent with other page objects and avoids the earlier visit() recursion issue.
| assertSignedUp(username) { | ||
| cy.get('[data-qa="profile-link"]').should('contain.text', username); | ||
| } |
There was a problem hiding this comment.
Suggestion: There is no helper to assert sign-up failures (errors). The test suite requires a negative sign-up test (sign up with invalid/duplicate data). Add a method like assertSignUpError(message) that checks the error alert or error list (using the app's data-qa selector for errors). This will keep tests readable and consistent with the POM pattern.
| typeUsername(username) { | ||
| this.usernameField().clear().type(username); | ||
| } | ||
|
|
||
| typeEmail(email) { | ||
| this.emailField().clear().type(email); | ||
| } | ||
|
|
||
| typePassword(password) { | ||
| this.passwordField().clear().type(password); | ||
| } | ||
|
|
||
| clickSignUpBtn() { | ||
| this.signUpBtn().should('be.visible').click(); |
There was a problem hiding this comment.
Suggestion (convenience): The POM exposes typeUsername, typeEmail, typePassword, and clickSignUpBtn individually. Consider adding a register(username, email, password) helper that calls these in sequence and submits — this reduces duplication in tests and makes setup clearer.
| this.usernameField = () => cy.get('[data-qa="signup-username"]'); | ||
| this.emailField = () => cy.get('[data-qa="signup-email"]'); | ||
| this.passwordField = () => cy.get('[data-qa="signup-password"]'); | ||
| this.signUpBtn = () => cy.get('[data-qa="sign-up-btn"]'); |
There was a problem hiding this comment.
Check consistency: the button selector uses data-qa="sign-up-btn". Ensure the tests (and the application markup) use the exact same data-qa value — mismatches between sign-up-btn and something like signup-btn will cause selectors to fail at runtime. If tests currently reference a different attribute, align them.
| class UserPageObject extends PageObject { | ||
| constructor() { | ||
| super(); | ||
| this.followBtn = () => cy.get('[data-qa="follow-btn"]'); |
There was a problem hiding this comment.
Line defines the follow button selector. Consider scoping this selector to a region (for example, the main/profile container) if pages have multiple similar buttons to avoid ambiguous matches. Also confirm the app exposes a data-qa specifically for the logout/follow control; if there is a separate selector for follow/unfollow states or a follower count, add those here.
Example guidance (do not paste literal code unless you confirm markup): () => cy.get('main').find('[data-qa="follow-btn"]') — this reduces risk of selecting the wrong element.
| visitProfile(username) { | ||
| super.visit(`/profile/${username}`); |
There was a problem hiding this comment.
visitProfile correctly builds the profile URL, but the method should also assert navigation succeeded (helps tests fail fast and makes flows deterministic). Consider adding an assertion such as cy.url().should('include', /profile/${username}) or verifying a profile heading is visible after navigation.
| clickFollow() { | ||
| this.followBtn().should('be.visible').click(); |
There was a problem hiding this comment.
clickFollow currently clicks the button — good. To better express intent consider renaming to toggleFollow() if the button toggles state, or add explicit clickUnfollow() if you want clear test code for follow vs unfollow flows. Also consider asserting the button is enabled before clicking: this.followBtn().should('be.visible').and('not.be.disabled').click() to be more robust.
| assertFollowButtonText(expectedText) { | ||
| this.followBtn().should('contain.text', expectedText); |
There was a problem hiding this comment.
assertFollowButtonText uses contain.text, which is generally tolerant. If tests expect exact text use should('have.text', expectedText). Additionally, the checklist suggests asserting follower counts or state changes — add a method like assertFollowerCount(expected) that targets a data-qa for follower count (if present) so tests can verify counts after follow/unfollow actions.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Here is my approval under an educational override so you can keep progressing while learning. You’ve done well setting up a solid POM structure, using Faker for unique data, and clearing the DB for determinism. The most critical fixes to address next are:
- Sign-in helper missing: add
signIn(email, password)incypress/support/pages/signIn.pageObject.jsso calls in settings/user/article specs don’t fail. - Selector mismatch: fix
assertBioincypress/support/pages/settings.pageObject.jsto use the same[data-qa="settings-bio"]selector asbioField()and verify persistence on the public profile. - Navigation and flow: ensure
article.pageObject.js’seditArticlenavigates to the editor (or provideopenArticleByTitle/clickEdithelpers), and correct sign-up flow so you don’t use sign-in actions without visiting the login page. - Missing coverage: add the required “update username” test in
settings.cy.js. - Determinism: chain
cy.task('generateUser')→cy.register(...)→ sign-in by returning/then-chaining commands to avoid race conditions; prefer resilient assertions (e.g.,contain.text).
Nice progress overall—once you apply these targeted fixes, the suite should be stable and fully compliant. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| let user; | ||
|
|
||
| beforeEach(() => { | ||
| cy.task('db:clear'); |
There was a problem hiding this comment.
You clear the DB and then generate/register a user in beforeEach which is good for determinism. Ensure the task db:clear is implemented in plugins and actually clears all relevant tables/collections before tests run. If db:clear is slow you may opt to run it once in a before hook, but clearing beforeEach is correct for determinism.
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
This block depends on two external helpers: cy.task('generateUser') and cy.register(...). Confirm that:
generateUserreturns a unique user object (email, username, password) each call (to avoid collisions).cy.registeris defined incypress/support/commands.js(or similar) and returns a Cypress chainable that resolves when registration completes.
If either is missing or not deterministic the tests will fail. Ifcy.registeruses the UI to sign up, consider using an API call to be faster and less flaky.
| body: faker.lorem.paragraph() | ||
| }; | ||
|
|
||
| signInPage.signIn(user.email, user.password); |
There was a problem hiding this comment.
You call signInPage.signIn(user.email, user.password) in each test. Make sure signInPage.signIn() performs a full, self-contained sign-in flow (visits the login page, fills, submits and waits for login to complete). If it relies on a visit() alias that doesn't exist, the test will fail. Also consider adding a shared helper signIn(email,password) to reduce repetition across specs if not already present.
|
|
||
| signInPage.signIn(user.email, user.password); | ||
|
|
||
| articlePage.createArticle(article); |
There was a problem hiding this comment.
articlePage.createArticle(article) must reliably navigate to the editor, clear inputs, type values and submit, then wait for the created-article page to load before returning control. Ensure the implementation uses data-qa attributes for selectors (per task requirements) and calls .clear() on inputs before typing to avoid flaky behavior when reusing the editor.
| articlePage.createArticle(originalArticle); | ||
|
|
||
| articlePage.editArticle(updatedArticle); |
There was a problem hiding this comment.
In the edit test you create an article then call articlePage.editArticle(updatedArticle). Confirm createArticle finishes navigation to the article page before editArticle runs — the POM should internally wait on UI/network events. Also ensure editArticle locates the correct article (e.g. by using the article title) and uses data-qa selectors. If editArticle assumes the app is already on the article page, make that explicit in its implementation or in the test flow.
| articlePage.assertArticlePresent(article.title); | ||
| }); | ||
|
|
||
| it('should be edited using Edit button', () => { | ||
| it('should edit an article', () => { | ||
| const originalArticle = { | ||
| title: faker.lorem.words(3), | ||
| description: faker.lorem.sentence(), | ||
| body: faker.lorem.paragraph() | ||
| }; | ||
| const updatedArticle = { | ||
| title: faker.lorem.words(3) + ' Updated', | ||
| description: faker.lorem.sentence() + ' Updated', | ||
| body: faker.lorem.paragraph() + ' Updated' | ||
| }; | ||
|
|
||
| signInPage.signIn(user.email, user.password); | ||
|
|
||
| articlePage.createArticle(originalArticle); | ||
|
|
||
| articlePage.editArticle(updatedArticle); | ||
| articlePage.assertArticlePresent(updatedArticle.title); | ||
| }); | ||
|
|
||
| it('should be deleted using Delete button', () => { | ||
| it('should delete an article', () => { | ||
| const article = { | ||
| title: faker.lorem.words(3), | ||
| description: faker.lorem.sentence(), | ||
| body: faker.lorem.paragraph() | ||
| }; | ||
|
|
||
| signInPage.signIn(user.email, user.password); | ||
| articlePage.createArticle(article); | ||
|
|
||
| articlePage.deleteArticle(); | ||
| articlePage.assertArticleNotPresent(article.title); |
There was a problem hiding this comment.
Your assertions only check the article title presence/absence. For stronger verification (and to satisfy the persistence-related checklist items), consider also asserting description or body after creation and after edit, and after password/email changes in settings tests (not in this file, but as a general rule). For delete, checking absence of the title is okay but can be complemented with a redirect/assertion that you are back on the global articles list.
| import { faker } from '@faker-js/faker'; | ||
|
|
||
| describe('Article', () => { | ||
| before(() => { | ||
| const signInPage = new SignInPageObject(); | ||
| const articlePage = new ArticlePageObject(); | ||
|
|
||
| }); | ||
| describe('Articles', () => { | ||
| let user; | ||
|
|
||
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| cy.register(user.email, user.username, user.password); | ||
| }); | ||
| }); | ||
|
|
||
| it('should be created using New Article form', () => { | ||
| it('should create a new article', () => { | ||
| const article = { | ||
| title: faker.lorem.words(3), | ||
| description: faker.lorem.sentence(), | ||
| body: faker.lorem.paragraph() | ||
| }; | ||
|
|
||
| signInPage.signIn(user.email, user.password); | ||
|
|
||
| articlePage.createArticle(article); |
There was a problem hiding this comment.
General reminder: the task requires that all UI elements interacted with by tests expose data-qa attributes for stable selectors. This file assumes the POM methods rely on those attributes; please confirm the front-end components were updated with data-qa attributes and that the POM uses them (not classes or text content). If the front-end is missing these attributes, add them and push those changes to the repo used by CI.
| cy.register(user.email, user.username, user.password); | ||
| signInPage.signIn(user.email, user.password); |
There was a problem hiding this comment.
This test calls signInPage.signIn(user.email, user.password) but the SignInPageObject class does not expose a signIn method — only visitPage, typeEmail, typePassword, and clickSignInBtn are implemented. Add a convenience helper such as:
signIn(email, password) {
this.visitPage();
this.typeEmail(email);
this.typePassword(password);
this.clickSignInBtn();
}Without this, the call here will throw signIn is not a function at runtime. See the current SignIn page object for reference.
| settingsPage.updateEmail(newEmail); | ||
|
|
||
| settingsPage.logout(); | ||
| signInPage.signIn(newEmail, user.password); |
There was a problem hiding this comment.
Same issue as above: the test signs in with the new email using signInPage.signIn(newEmail, user.password) but signIn helper is missing from the SignIn POM. Add the signIn method or replace this call with the explicit sequence (visitPage, typeEmail, typePassword, clickSignInBtn).
| settingsPage.updatePassword(newPassword); | ||
|
|
||
| settingsPage.logout(); | ||
| signInPage.signIn(user.email, newPassword); |
There was a problem hiding this comment.
Again the test signs in with the new password using signInPage.signIn(user.email, newPassword) — ensure the signIn helper exists in the SignIn page object or replace with the explicit steps. Otherwise this will fail at runtime.
| settingsPage.updateBio(newBio); | ||
| settingsPage.assertBio(newBio); |
There was a problem hiding this comment.
After updating the bio you call settingsPage.assertBio(newBio) which only asserts the settings form value. The task requires verifying that changes persist and are visible on the profile page. Add a step to visit the profile (e.g. /profile/${user.username}) and assert the bio is displayed there (use a stable data-qa selector). The task description requires verifying persistence for settings updates.
| const settingsPage = new SettingsPageObject(); | ||
|
|
||
| }); | ||
| describe('Settings', () => { |
There was a problem hiding this comment.
The task list explicitly requires a test that updates the username. This spec covers bio, email and password but does not include a test for updating the username — add an it('should update username', ...) test that updates the username and asserts persistence (profile/header shows the new username).
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
Minor determinism improvement: inside beforeEach you call cy.register(...) then immediately signInPage.signIn(...) inside the same then callback. While this is likely fine, consider returning or chaining cy.register(...) (or chaining a .then() after it) so Cypress command ordering is explicit and deterministic. This reduces flakiness if cy.register is async. Example:
cy.task('generateUser').then((generatedUser) => {
user = generatedUser;
return cy.register(user.email, user.username, user.password);
}).then(() => {
signInPage.signIn(user.email, user.password);
});
This ensures signIn runs only after registration completes.
|
|
||
| settingsPage.logout(); | ||
| signInPage.signIn(newEmail, user.password); | ||
| cy.contains(newEmail).should('exist'); |
There was a problem hiding this comment.
Assertion robustness: in the email update test you assert cy.contains(newEmail).should('exist'). This may accidentally match text elsewhere. Prefer asserting a stable element (for example the profile link or header element with a data-qa attribute) to confirm login with new email succeeded. The Settings POM already includes logout() which is used correctly — good.
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generateUser) => { | ||
| user = generateUser; | ||
| cy.task('generateUser').then((generatedUser) => { |
There was a problem hiding this comment.
You call cy.task('generateUser').then(...) and then cy.register(...) inside the then. Mixing the task promise and Cypress commands can lead to timing/ordering issues. Ensure the Cypress command chain is returned so Cypress waits for the registration to complete — e.g. return the cy.task(...) chain or restructure so registration is part of the Cypress command queue. This improves determinism of test setup.
| user = generateUser; | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
After the generated user is assigned you call cy.register(...) to create the user in the app. Make sure cy.register is returning a Cypress chain (and/or return the whole cy.task(...).then(...) call) so Cypress waits for the registration to finish before running tests. This reduces flakiness in setup.
| signInPage.visit(); | ||
| cy.register(user.email, user.username, user.password); | ||
|
|
||
| it('should log in with correct credentials', () => { |
There was a problem hiding this comment.
This spec uses HomePageObject.assertHeaderContainUsername(user.username) after sign-in, but there is no cypress/support/pages/home.pageObject.js in the repo — the import will fail at runtime. Add a HomePageObject with assertHeaderContainUsername (using a stable data-qa selector) or change the assertion to use an existing POM/selector so the test can run. See other POM files (e.g., signIn.pageObject.js) for patterns.
| cy.register(user.email, user.username, user.password); | ||
|
|
||
| it('should log in with correct credentials', () => { | ||
| signInPage.visitPage(); |
There was a problem hiding this comment.
You call the sign-in steps individually (typeEmail, typePassword, clickSignInBtn) — consider adding a signIn(email, password) convenience method in SignInPageObject and using it here (other specs already call signInPage.signIn(...)), it reduces duplication and keeps tests consistent. For reference, other specs rely on the convenience method; aligning helps maintainability.
| signInPage.typePassword('wrongPassword'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.get('[data-qa="error-messages"]').should('contain.text', 'email or password is invalid'); |
There was a problem hiding this comment.
The negative assertion checks the exact text 'email or password is invalid'. That exact phrasing may change and make the test brittle. Prefer a partial/less brittle check, for example .should('contain.text', 'email or password') or check that you're still on the login page and that an error message element is visible.
| signUpPage.typeEmail(user.email); | ||
| signUpPage.typePassword(user.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
There was a problem hiding this comment.
After clicking the Sign Up button you immediately call methods on signInPage (typeEmail/typePassword/clickSignInBtn) without ensuring the app is on the sign-in page. This will fail because the sign-in inputs probably aren't present. Fix one of the following:
- Preferred: After
signUpPage.clickSignUpBtn()assert the app shows the logged-in state (e.g., check the username in the header). Remove the subsequent sign-in steps. - Or: Navigate to the login page first (e.g.,
signInPage.visitPage()orcy.visit('/login')) before usingsignInPagemethods.
As written this test is order/flow dependent and will be flaky or fail at runtime.
| signUpPage.typeUsername(user.username); | ||
| signUpPage.typeEmail(user.email); | ||
| signUpPage.typePassword(user.password); |
There was a problem hiding this comment.
Add .clear() before typing into the sign-up form inputs to avoid leftover text interfering with tests. For example: signUpPage.getEmailInput().clear().type(user.email) (or add .clear() inside the POM helper). This improves determinism and reduces flakiness.
| signUpPage.typeUsername(user.username); | ||
| signUpPage.typeEmail(user.email); | ||
| signUpPage.typePassword(user.password); |
There was a problem hiding this comment.
Also add .clear() for the second sign-up attempt in the negative test (username/email/password fields) to keep behavior consistent and robust.
| }); | ||
|
|
||
| it('should show error for existing email', () => { | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
This test uses cy.register(user.email, user.username, user.password) to prepare the existing-user precondition. Ensure that this custom command/task is implemented and reliably creates the user in the test database. Prefer creating users via a backend API task (cy.task) or a proper custom command that returns success so tests are deterministic.
| signUpPage.typePassword(user.password); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| cy.contains('email has already been taken').should('be.visible'); |
There was a problem hiding this comment.
The negative assertion checks the exact text 'email has already been taken', which is brittle. Prefer matching a substring or using .should('contain.text', 'email') or a case-insensitive regex, so small wording changes don't break the test unnecessarily.
| cy.register(target.email, target.username, target.password); | ||
| }); | ||
|
|
||
| signInPage.signIn(actor.email, actor.password); |
There was a problem hiding this comment.
You call signInPage.signIn(actor.email, actor.password) here but the SignInPageObject does not implement a signIn helper — it only has visitPage, typeEmail, typePassword, and clickSignInBtn. Add a convenience method to the SignIn POM, e.g.:
signIn(email, password) {
this.visitPage();
this.typeEmail(email);
this.typePassword(password);
this.clickSignInBtn();
}Without this the signIn call will throw at runtime. See the SignIn page object for the existing methods.
| cy.task('generateUser').then((generatedActor) => { | ||
| actor = generatedActor; | ||
| cy.register(actor.email, actor.username, actor.password); | ||
| }); | ||
|
|
||
| cy.task('generateUser').then((generatedTarget) => { | ||
| target = generatedTarget; | ||
| cy.register(target.email, target.username, target.password); |
There was a problem hiding this comment.
The two cy.task('generateUser').then(...) blocks register the actor and target separately, but signInPage.signIn(...) is called after those blocks in the beforeEach. To guarantee that both users are registered before signing in the actor, chain the operations so sign-in is executed only after both registrations complete. For example, nest the second cy.task(...).then(...) (or return promises) and then call signIn in the final .then(...), or chain them with returned commands. This prevents race conditions and makes the setup deterministic.
| cy.task('generateUser').then((generatedActor) => { | ||
| actor = generatedActor; | ||
| cy.register(actor.email, actor.username, actor.password); | ||
| }); | ||
|
|
||
| cy.task('generateUser').then((generatedTarget) => { | ||
| target = generatedTarget; | ||
| cy.register(target.email, target.username, target.password); |
There was a problem hiding this comment.
Related to the previous point: inside each .then() you call cy.register(...). Make sure you return or chain this registration command so Cypress runs it in sequence. Example pattern:
cy.task('generateUser').then((u) => {
user = u;
return cy.register(user.email, user.username, user.password);
}).then(() => {
// next steps here
});
This ensures cy.register is completed before subsequent commands are executed.
| it('should unfollow a user', () => { | ||
| userPage.visitProfile(target.username); | ||
| userPage.clickFollow(); | ||
| userPage.assertFollowButtonText('Unfollow'); | ||
|
|
||
| userPage.clickFollow(); | ||
| userPage.assertFollowButtonText('Follow'); | ||
| userPage.assertFollowerCount(0); |
There was a problem hiding this comment.
The unfollow test follows then unfollows using UI clicks in the same test. That can be fine, but to reduce flakiness consider making the initial "follow" state explicit in a beforeEach (e.g., create follow via API or ensure the actor follows the target before running the unfollow assertions). If you keep the current approach, at least assert the initial follower count or the 'Unfollow' text after the first click before performing the second click.
| let actor, target; | ||
|
|
||
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
|
|
||
| cy.task('generateUser').then((generatedActor) => { | ||
| actor = generatedActor; | ||
| cy.register(actor.email, actor.username, actor.password); | ||
| }); | ||
|
|
||
| cy.task('generateUser').then((generatedTarget) => { | ||
| target = generatedTarget; |
There was a problem hiding this comment.
Small robustness suggestion: after DB clear and user generation it can help to assert users are registered or use returned cy commands to guarantee the state before proceeding to sign in. Also consider verifying that actor.username !== target.username (they should be unique when using generateUser but an explicit check can help catch a bad generator). This is defensive and will reduce intermittent failures.
| visit(url) { | ||
| cy.visit(url || this.url); |
There was a problem hiding this comment.
visit(url) implementation is correct and appropriate as a shared helper. Tests and page objects call this via super.visit(...) already — good. Keep this as-is.
| assertAlert(alertMessage) { // poprawiona literówka | ||
| cy.on('window:alert', (alert) => { | ||
| expect(alert).to.eq(alertMessage); | ||
| }); |
There was a problem hiding this comment.
assertAlert(alertMessage) will work, but note two points:
- The inline comment
// poprawiona literówkais in Polish — consider removing or translating it to avoid confusion. cy.on('window:alert', ...)must be registered before the action that triggers the alert; ensure tests call this (or the page object exposes a wrapper) prior to triggering the alert so the handler captures it reliably.
| } | ||
| } | ||
|
|
||
| export default PageObject; |
There was a problem hiding this comment.
Exporting the class with export default PageObject; is correct. After adding shared helpers (profile/header/logout) the tests and other POMs will be cleaner — e.g., this.profileLink() or this.logout() can be reused across page objects. Consider adding these helpers in this file rather than duplicating selectors in each POM.
| editArticle({ title, description, body }) { | ||
| this.titleField().clear().type(title); | ||
| this.descriptionField().clear().type(description); | ||
| this.bodyField().clear().type(body); | ||
| this.submitBtn().click(); | ||
| } |
There was a problem hiding this comment.
Functional gap: editArticle types into the editor fields but does not navigate to the editor page. In typical app flow the user is on the article view after creating an article, so calling editArticle immediately after createArticle will likely fail because the editor fields are not present. Add navigation to the editor (for example, a method that clicks an Edit button on the article view) or make editArticle itself open the editor before typing.
| visitPage() { | ||
| super.visit('/editor'); | ||
| } | ||
|
|
||
| createArticle({ title, description, body }) { | ||
| this.visitPage(); |
There was a problem hiding this comment.
Missing navigation helpers that tests commonly need: add methods like openArticleByTitle(title) (which finds the article link by title and clicks it) and clickEdit() (which clicks an Edit button on the article view). These make test flows explicit (create -> open article -> edit). Right now tests must rely on implicit navigation.
| assertArticleNotPresent(title) { | ||
| cy.get('[data-qa="article-title-display"]').contains(title).should('not.exist'); |
There was a problem hiding this comment.
Assertion robustness: assertArticleNotPresent uses cy.get('[data-qa="article-title-display"]').contains(title).should('not.exist'). If .contains finds no matching element Cypress will throw before the should runs. Prefer a pattern that doesn't throw, e.g.:
cy.get('[data-qa="article-title-display"]').should('not.contain', title);or query for elements and assert the length is 0 for matches. Update this method to avoid false negatives and flakiness.
| visitPage() { | ||
| super.visit('/editor'); |
There was a problem hiding this comment.
Minor: visitPage() uses super.visit('/editor'), which is fine for opening a new article editor. Consider adding a method to visit an editor for a specific article slug if tests need to open the editor for editing an existing article, e.g. visitEditorForSlug(slug) or visitEditorForTitle(title) that first opens the article and then clicks Edit.
| createArticle({ title, description, body }) { | ||
| this.visitPage(); | ||
| this.titleField().clear().type(title); | ||
| this.descriptionField().clear().type(description); | ||
| this.bodyField().clear().type(body); | ||
| this.submitBtn().click(); |
There was a problem hiding this comment.
Style/clarity suggestion: Consider returning Cypress chainables from action methods when useful (e.g., createArticle could return this.submitBtn().click()), this can help tests chain further assertions reliably. This is optional but can improve test readability and determinism.
| assertBio(bio) { | ||
| cy.get('[data-qa="bio-field"]').should('have.value', bio); |
There was a problem hiding this comment.
Bug: assertBio uses a different selector than the bioField getter. bioField() targets [data-qa="settings-bio"] (line 8) but assertBio checks [data-qa="bio-field"] (line 43). This mismatch will cause assertBio to fail. Use the same selector (prefer reusing the getter) — for example:
assertBio(bio) {
this.bioField().should('have.value', bio);
}Fixing this is critical because the settings spec calls settingsPage.assertBio(newBio) expecting to verify the saved bio.
| updateUsername(username) { | ||
| this.usernameField().clear().type(username); | ||
| this.updateBtn().click(); | ||
| } |
There was a problem hiding this comment.
Behavior / feature suggestion: the task requires verifying persistence of settings changes. Right now assertBio checks the settings form value only. Add a method that asserts the bio on the public profile (e.g., assertProfileBio(bio, username) that visits /profile/${username} and checks a profile-bio data-qa). Either add this helper here or ensure the test navigates to the profile and asserts the visible bio. Example approach:
assertProfileBio(username, bio) {
super.visit(`/profile/${username}`);
cy.get('[data-qa="profile-bio"]').should('contain.text', bio);
}This will satisfy the persistence verification requirement in the task description.
| updateUsername(username) { | ||
| this.usernameField().clear().type(username); | ||
| this.updateBtn().click(); | ||
| } | ||
|
|
||
| updateBio(bio) { | ||
| this.bioField().clear().type(bio); | ||
| this.updateBtn().click(); | ||
| } | ||
|
|
||
| updateEmail(email) { | ||
| this.emailField().clear().type(email); | ||
| this.updateBtn().click(); | ||
| } | ||
|
|
||
| updatePassword(password) { | ||
| this.passwordField().clear().type(password); | ||
| this.updateBtn().click(); |
There was a problem hiding this comment.
Stability suggestion: after calling this.updateBtn().click() in update methods (lines 20, 25, 30, 35), consider adding a post-submit verification (for example, wait for a success notification or ensure the button becomes enabled/disabled) before returning. This reduces flakiness when tests immediately navigate away or try to log out. It is optional but recommended for reliability.
| clickSignInBtn() { | ||
| this.signInBtn | ||
| .click(); | ||
| this.signInBtn().should('be.visible').click(); |
There was a problem hiding this comment.
Tests in the suite call signInPage.signIn(email, password) but this class does not implement a signIn helper. Add a convenience method that composes the existing actions, for example:
signIn(email, password) {
this.visitPage();
this.typeEmail(email);
this.typePassword(password);
this.clickSignInBtn();
}Add it before the export default so test calls succeed. Without this the test runner will throw signIn is not a function at runtime. See the existing low-level methods in this file for implementation details.
| get signInBtn() { | ||
| return cy.getByDataCy('sign-in-btn'); | ||
| visitPage() { | ||
| super.visit(this.url); |
There was a problem hiding this comment.
Consider adding a small alias visit() that calls visitPage() to support tests that may call signInPage.visit() (some test examples expect a visit method). That keeps the API flexible and avoids calling differences between specs. Add it near visitPage() (e.g., right after it).
Example:
visit() {
return this.visitPage();
}| clickSignUpBtn() { | ||
| this.signUpBtn().should('be.visible').click(); | ||
| } |
There was a problem hiding this comment.
Add a convenience helper that encapsulates the full sign-up flow to reduce duplication in specs. For example:
signUp(username, email, password) {
this.visitPage();
this.typeUsername(username);
this.typeEmail(email);
this.typePassword(password);
this.clickSignUpBtn();
}This keeps tests concise and centralizes changes to the sign-up flow in one place. Consider placing this near the other action methods (around lines 17–31).
| clickSignUpBtn() { | ||
| this.signUpBtn().should('be.visible').click(); |
There was a problem hiding this comment.
clickSignUpBtn() simply clicks the button. To improve determinism, consider asserting or waiting for the expected navigation/result after the click (for example, check that the profile link appears or that the URL changed). Alternatively, return the command chain from clickSignUpBtn() so callers can chain further assertions. This will reduce flakiness when tests immediately assert post-signup state. Example: this.signUpBtn().should('be.visible').click(); followed by a cy.url().should('include', '/'); in the test or inside a signUp helper.
| assertSignedUp(username) { | ||
| cy.get('[data-qa="profile-link"]').should('contain.text', username); |
There was a problem hiding this comment.
assertSignedUp(username) uses the profile link data-qa which is good. Consider making the assertion slightly stricter (e.g., should('be.visible').and('contain.text', username)) to ensure the element is present and visible rather than only containing text somewhere in the DOM. This helps avoid accidental matches elsewhere on the page.
| visitProfile(username) { | ||
| super.visit(`/profile/${username}`); | ||
| cy.url().should('include', `/profile/${username}`); |
There was a problem hiding this comment.
visitProfile calls super.visit(/profile/${username}) and then asserts cy.url().should('include', "/profile/${username}"). Consider also asserting that a key profile element (for example an element with data-qa="profile-username" or the follow button) is visible to ensure the page has finished loading before interacting. This reduces occasional race conditions where the URL changed but the DOM isn't ready yet.
| assertFollowButtonText(expectedText) { | ||
| this.followBtn().should('have.text', expectedText); |
There was a problem hiding this comment.
assertFollowButtonText uses this.followBtn().should('have.text', expectedText). This is brittle to surrounding whitespace or child elements. Prefer a more resilient assertion such as:
this.followBtn().should('contain.text', expectedText);or normalize the button text before comparing. This reduces false negatives when small markup/spacing differs. Example change: replace have.text with contain.text or use .invoke('text').should('eq', expectedText.trim()) if an exact match is required.
| assertFollowerCount(expected) { | ||
| this.followerCount().should('contain.text', expected); |
There was a problem hiding this comment.
assertFollowerCount currently does this.followerCount().should('contain.text', expected), but tests pass a numeric value (e.g., 1). Consider exposing a helper that returns the follower count as a number so tests can assert numeric equality. Example:
getFollowerCount() {
return this.followerCount().invoke('text').then(t => Number(t.trim()));
}Then in tests you can do userPage.getFollowerCount().should('equal', 1). This is more robust than relying on contain.text and avoids matching unrelated text fragments.
No description provided.