Task solution #93
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical runtime issues that will prevent the test suite from passing, which is a core requirement of this task. In tests/signIn/signInPositive.spec.js, beforeEach currently calls signInPage = new SignInPage(); and homePage = new HomePage(); without receiving the Playwright page fixture; since SignInPage (and likely HomePage) expects a page argument in the constructor, this will cause this.page to be undefined and lead to runtime errors when tests run. Similarly, in tests/signUp/signUpPositive.spec.js, signUpPage = new SignUpPage(); and homePage = new HomePage(); are also missing the required page argument, which will break those tests as soon as the page objects try to interact with the browser. These are critical blockers because they violate the requirement that all Playwright tests must run and pass after setting up ESLint/Prettier/Husky.
To fix this, update each affected test.beforeEach to accept the page fixture, for example test.beforeEach(async ({ page }) => { ... }), and then instantiate your page objects as signInPage = new SignInPage(page);, signUpPage = new SignUpPage(page); and homePage = new HomePage(page);, mirroring what you already did correctly in tests/signIn/signInNegative.spec.js. Once you make these changes, rerun the full Playwright test suite and ensure everything passes end-to-end. Aside from these wiring issues, your ESLint flat configuration and general page object structure look solid, so you’re very close—fixing the page injection in the positive tests will align your solution with the task requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| globals: { | ||
| ...globals.browser, |
There was a problem hiding this comment.
SignInPage expects a page argument in its constructor, but here it's instantiated without any arguments, which will cause runtime errors when tests run and prevent a clean test suite required by the task. Consider updating beforeEach to receive { page } and pass it into new SignInPage(page) (similar to the negative tests).
| @@ -1,16 +1,16 @@ | |||
| import { expect, test } from "@playwright/test"; | |||
| import { expect } from '@playwright/test'; | |||
|
|
|||
There was a problem hiding this comment.
The import import tseslint from "typescript-eslint"; is unlikely to work as expected; the theory instructions for ESLint with TypeScript usually rely on @typescript-eslint packages and their configs helpers. This impacts ESLint configuration correctness and may cause npx eslint to fail.
| this.passwordField = page.getByPlaceholder('Password'); | ||
| this.signInButton = page.getByRole('button', { name: 'Sign in' }); | ||
| this.errorMessage = page.getByRole('list').nth(1); | ||
| } |
There was a problem hiding this comment.
In beforeEach, you are not receiving the Playwright page fixture, so new SignInPage() is called without a page. Since SignInPage expects a page argument, this will break your tests and prevent you from achieving the "all tests pass" requirement.
| this.signInButton = page.getByRole('button', { name: 'Sign in' }); | ||
| this.errorMessage = page.getByRole('list').nth(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
new SignInPage(); is missing the required page argument that the SignInPage constructor uses. This will cause runtime errors when the tests run, so you should pass the Playwright page fixture here.
| this.errorMessage = page.getByRole('list').nth(1); | ||
| } | ||
|
|
||
| async open() { |
There was a problem hiding this comment.
new HomePage(); mirrors the same pattern as SignInPage; if HomePage also expects a page (as is typical in this project), you should construct it with page to keep tests working under the configured tooling.
| signInPage = new SignInPage(page); | ||
| homePage = new HomePage(page); | ||
| test.beforeEach(async () => { | ||
| signInPage = new SignInPage(); |
There was a problem hiding this comment.
SignInPage requires a page argument in its constructor (as used in the negative tests), so calling new SignInPage() without page will lead to this.page being undefined and runtime errors when methods use it. Update beforeEach to accept { page } and pass it into the constructor.
| homePage = new HomePage(page); | ||
| test.beforeEach(async () => { | ||
| signInPage = new SignInPage(); | ||
| homePage = new HomePage(); |
There was a problem hiding this comment.
HomePage is also likely a page object that needs the Playwright page instance; instantiating it without page can cause similar runtime issues. Consider passing page here as well, consistent with how other page objects are used in the suite.
| signUpPage = new SignUpPage(page); | ||
| homePage = new HomePage(page); | ||
|
|
||
| test.beforeEach(async () => { |
There was a problem hiding this comment.
test.beforeEach does not receive the Playwright page fixture, so SignUpPage and HomePage are constructed without a page instance. If these page objects are like SignInPage and expect page in the constructor, this will cause runtime failures and block a clean test run.
| homePage = new HomePage(page); | ||
|
|
||
| test.beforeEach(async () => { | ||
| signUpPage = new SignUpPage(); |
There was a problem hiding this comment.
signUpPage = new SignUpPage(); is likely missing the required page argument (as with SignInPage in other tests). You should pass the page fixture here so the page object can interact with the browser context.
|
|
||
| test.beforeEach(async () => { | ||
| signUpPage = new SignUpPage(); | ||
| homePage = new HomePage(); |
There was a problem hiding this comment.
homePage = new HomePage(); mirrors the same issue as signUpPage; if HomePage expects a page, instantiate it with page to avoid runtime errors during the automated test run.
2 participants
🤼