solution#330
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! 😊 There are a few critical issues that need to be fixed before your PR can be approved: (1) The filename acticlePage.pageObject.js is misspelled and should be articlePage.pageObject.js—this typo causes import errors and will prevent tests from running; (2) In your sign-up and user follow tests, asynchronous user data generation and registration are not handled correctly, which may lead to unreliable or broken tests; (3) Please remove the unnecessary src/package-lock.json unless your project specifically requires it. Once you address these issues, your PR should be ready for approval! Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| /// <reference types='cypress' /> | ||
| /// <reference types='../support' /> | ||
|
|
||
| import ArticlePageObject from '../support/pages/acticlePage.pageObject'; |
There was a problem hiding this comment.
There is a typo in the import path: 'acticlePage.pageObject' should be 'articlePage.pageObject'. This will cause a module not found error and must be fixed to run the tests.
| it('should sign up succefully', () => { | ||
| signInPage.visit(); | ||
| cy.register(email, username, password); | ||
| }); |
There was a problem hiding this comment.
Issue: The test uses 'email', 'username', and 'password' variables that are set asynchronously in beforeEach. There is no guarantee that these variables are set before the test runs, which may lead to them being undefined. Consider returning the 'cy.task' promise or chaining the test logic to ensure the variables are set before use.
| userTarget = generateUser; | ||
| cy.register(userTarget.email, userTarget.username, userTarget.password); | ||
| userFollower = generateUser; | ||
| userFollower.email += 'world'; | ||
| userFollower.username += 'follower'; | ||
| cy.register( | ||
| userFollower.email, | ||
| userFollower.username, | ||
| userFollower.password | ||
| ); |
There was a problem hiding this comment.
Issue: Both userTarget and userFollower are derived from the same generated user object, with userFollower being a mutated version. This could cause unexpected behavior since both users will share the same password and initial data. Consider generating two separate users using 'cy.task('generateUser')' for each.
| userTarget = generateUser; | ||
| cy.register(userTarget.email, userTarget.username, userTarget.password); | ||
| userFollower = generateUser; | ||
| userFollower.email += 'world'; | ||
| userFollower.username += 'follower'; | ||
| cy.register( | ||
| userFollower.email, | ||
| userFollower.username, | ||
| userFollower.password | ||
| ); |
There was a problem hiding this comment.
Issue: The registration of both users is not properly chained. Since 'cy.register' is asynchronous, you should ensure that the registration of userFollower happens after userTarget is fully registered. Consider chaining the promises or using separate 'then' blocks.
| @@ -0,0 +1,47 @@ | |||
| import PageObject from '../PageObject'; | |||
There was a problem hiding this comment.
Critical issue: The filename is misspelled as 'acticlePage.pageObject.js'. It should be 'articlePage.pageObject.js' to match the import statements in your test files and avoid module not found errors.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! There are still a few critical issues that need to be addressed before your PR can be approved. Specifically:
- In
article.cy.js, ensure the import path forArticlePageObjectmatches the actual filename exactly (articlePage.pageObject.js), as mismatches will break your tests. - In
signUp.cy.js, user data is generated asynchronously in thebeforeEachhook, but your tests do not wait for this data to be set before using it. Please ensure your tests only run after the user data is generated. - In
user.cy.js, bothuserTargetanduserFollowerare assigned from the same generated user object and then mutated. Please generate two separate users to avoid data overlap.
Once you fix these issues, your tests should run reliably and your PR will be ready for approval! Keep going—you're almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| /// <reference types='cypress' /> | ||
| /// <reference types='../support' /> | ||
|
|
||
| import ArticlePageObject from '../support/pages/articlePage.pageObject'; |
There was a problem hiding this comment.
There may be a mismatch between the import path '../support/pages/articlePage.pageObject' and the actual filename. Please ensure the file is named 'articlePage.pageObject.js' and the import path matches exactly, as a typo or case sensitivity issue will cause import errors. This is a critical issue that must be fixed for your tests to run.
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((user) => { | ||
| username = user.username; | ||
| email = user.email; | ||
| password = user.password; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The user data is generated asynchronously in the beforeEach hook, but the test cases do not wait for this data to be set before using it. This can cause tests to run before username, email, and password are assigned, leading to failures. To fix this, ensure your tests only run after the user data is generated, for example by returning the promise from the beforeEach hook.
| cy.task('generateUser').then((generateUser) => { | ||
| userTarget = generateUser; | ||
| cy.register(userTarget.email, userTarget.username, userTarget.password); | ||
| userFollower = generateUser; | ||
| userFollower.email += 'world'; | ||
| userFollower.username += 'follower'; | ||
| cy.register( | ||
| userFollower.email, | ||
| userFollower.username, | ||
| userFollower.password | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Both userTarget and userFollower are assigned from the same generated user object and then mutated. This can cause both users to have overlapping or invalid data. To ensure unique users, generate two separate users by calling cy.task('generateUser') twice.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous issues! 🎉 Your updated tests now properly handle asynchronous user generation, ensuring that user data is always available before being used. Specifically:
- In
signIn.cy.js, please ensure that the test always waits for theuserobject to be generated before accessing it. This will prevent any race conditions or undefined errors. - In
user.cy.js, you've improved the setup, but make sure bothuserTargetanduserFollowerare guaranteed to be available before the tests run. This is crucial for reliable and non-flaky tests.
All other core requirements are met: you use the Page Object Model, manage database state correctly, and generate fake user data as required. Your test structure is clear and follows best practices.
Next steps:
Please review the inline comments and make the suggested improvements to your asynchronous test setups. This will help you avoid potential issues in future projects and ensure your tests remain robust.
Keep up the good work and continue learning! If you need more information on handling asynchronous code in Cypress, check out the Cypress documentation on Promises and async code. 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| signInPage.visit(); | ||
|
|
||
| signInPage.typeEmail(user.email); | ||
| signInPage.typePassword(user.password + 'wrong111'); | ||
| signInPage.clickSignInBtn(); | ||
|
|
||
| cy.contains('div[class="swal-title"]', 'Login failed!').should( | ||
| 'be.visible' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Issue: This test also uses the user object without ensuring it is available. Make sure the test waits for user generation before accessing user, as per the checklist requirements.
| it('should be able to follow the another user', () => { | ||
| signInPage.visit(); | ||
|
|
||
| signInPage.typeEmail(userFollower.email); | ||
| signInPage.typePassword(userFollower.password); | ||
|
|
||
| signInPage.clickSignInBtn(); | ||
|
|
||
| // eslint-disable-next-line cypress/no-unnecessary-waiting | ||
| cy.wait(1000); | ||
|
|
||
| cy.visit(`/#/@${userTarget.username.replace('follower', '')}`); | ||
|
|
||
| cy.contains('button', `Follow ${userTarget.username.replace('follower', '')}`).click(); | ||
| }); |
There was a problem hiding this comment.
Issue: The test uses userTarget and userFollower without ensuring both are available before the test runs. Since user generation is asynchronous, you should guarantee both users are set before proceeding with the tests, as required by the checklist.
2 participants
No description provided.