<commit message>#65
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall structure and the Page Object Models! However, this requires a couple of changes. The primary issue is in eslint.config.mjs, where configurations are being merged with a spread operator. This is incorrect for ESLint's flat config and can lead to rules being overwritten; they should be listed as separate objects in the exported array. [CHECKLIST ITEM #1] You'll also need to fix the linting errors this new configuration will find, such as lines exceeding the 80-character limit in tests/signUp/signUpNegative.spec.js and an unused argument in tests/signIn/signInNegative.spec.js. Addressing these points will make your setup fully functional.
✨ 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
| { | ||
| ...pluginJs.configs.recommended, | ||
| ...playwright.configs['flat/recommended'], | ||
| rules: { | ||
| ...pluginJs.configs.recommended.rules, | ||
| 'no-unused-vars': 'error', | ||
| 'max-len': [ | ||
| 'error', | ||
| { | ||
| code: 80, | ||
| comments: 80, | ||
| }, | ||
| ], | ||
| ...playwright.configs['flat/recommended'].rules, | ||
| 'playwright/expect-expect': 'off', | ||
| }, | ||
| ignores: [ | ||
| '**/node_modules/*', | ||
| 'playwright.config.js', | ||
| '**/playwright-report/**', | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Merging different configurations like pluginJs.configs.recommended and playwright.configs['flat/recommended'] into a single object using the spread operator is not the correct approach for ESLint's flat config. If both configurations have properties with the same name (like rules), the property from the last configuration spread will overwrite the previous ones.
The recommended practice is to list each configuration as a separate object in the exported array. This ensures that all rules and settings are combined correctly.
|
|
||
| test("Assert error message for empty email", async ({ page }) => { | ||
| await signInPage.fillPasswordField("newpass123!"); | ||
| test('Assert error message for empty email', async ({}) => { |
There was a problem hiding this comment.
This test function includes an empty destructured object {} as an argument, but it's not used within the test. The ESLint configuration includes the no-unused-vars rule, which will report this as an error. To resolve the ESLint error, you can remove the unused argument, changing async ({}) to async ().
| test("Assert error message for empty username in Sign up form", async () => { | ||
| await signUpPage.fillEmailField("test@gmail.com"); | ||
| await signUpPage.fillPasswordField("newpass123!"); | ||
| test('Assert error message for empty username in Sign up form', async () => { |
There was a problem hiding this comment.
This line exceeds the 80-character limit configured in eslint.config.mjs. Please refactor the test description to fit within the max-len rule.
| await signUpPage.assertErrorMessageContainsText( | ||
| `username:Username must start with a letter, have no spaces, and be 2 - 40 characters.` | ||
| `username:Username must start with a letter, have no spaces, and be 2-40`, | ||
| ); |
There was a problem hiding this comment.
This line is longer than the 80-character limit set in the ESLint configuration. To resolve this, you could store the long error message string in a variable or constant and pass that to the method.
| await signUpPage.assertErrorMessageContainsText( | ||
| `email:This email does not seem valid.` | ||
| `email:This email does not seem valid.`, | ||
| ); |
There was a problem hiding this comment.
This line exceeds the 80-character limit. Consider moving the long string to a constant to improve readability and adhere to the linting rules.
| test("Assert error message for empty password in Sign up form", async () => { | ||
| await signUpPage.fillUsernameField("newuser"); | ||
| await signUpPage.fillEmailField("test@gmail.com"); | ||
| test('Assert error message for empty password in Sign up form', async () => { |
There was a problem hiding this comment.
This line also violates the 80-character max-len rule established in the ESLint configuration. Please shorten the test description.
2 participants
No description provided.