Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the comprehensive test suite for checkPassword. Your tests cover almost all the required scenarios. I am requesting one small change to ensure all boundary conditions are properly tested as per the requirements.
In src/checkPassword.test.js, the test for a valid password uses 'Password1!', which is 10 characters long. While this is a valid password, it doesn't specifically test the lower boundary condition. Please update this test to use a valid password that is exactly 8 characters long, such as 'Test1!ok', to confirm the minimum length requirement is handled correctly.
✨ 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
src/checkPassword.test.js
Outdated
| it(`should return 'true' for the valid password | ||
| with at least 8 characters`, () => { | ||
| expect(checkPassword('Password1!')).toBe(true); |
There was a problem hiding this comment.
This test is a good idea, but the password you're using ('Password1!') is 10 characters long, which doesn't specifically test the lower boundary. The task requirements state that you should test boundary conditions, including a valid password of exactly 8 characters. Please update this test with an 8-character password that meets all the other validity rules (e.g., 'Test1!ok').
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 feedback! Your test suite is very comprehensive and covers almost all the required scenarios. I am requesting one small change to fully align with the task description. The requirements mention testing a password that fails multiple criteria, using 'qwerty' as an example. While your tests cover each rule violation separately, a test for a multi-failure case is missing. Please add a test case for a password like 'qwerty' to ensure the function correctly handles simultaneous validation failures. After this change, your solution will be complete!
✨ 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
| it(`should return 'false' for the invalid password | ||
| without letters of the Latin alphabet `, () => { | ||
| expect(checkPassword('Пароль1!')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
This is a great set of tests! To make it complete, consider adding one more test case for a password that fails multiple validation rules at once, like 'qwerty', which is mentioned as an example in the task description.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on your tests! You've done a great job addressing the previous feedback by adding a test for a password that fails multiple criteria. Your test suite is now very thorough.
I am approving your solution as an educational override to help you keep moving forward. The only thing missing is a test case for the specific example checkPassword('Str@ng') === false, which was mentioned in the task description. Adding this final test will make your solution perfectly align with all requirements.
Fantastic work, you're very close to a complete solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| it(`should return 'false' for the invalid password | ||
| with incorrect multiple criteria`, () => { | ||
| expect(checkPassword('qwerty')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Great job adding the test for 'qwerty'. To fully satisfy the task requirements, please also add a test case for the other example provided in the description: checkPassword('Str@ng') === false. This will ensure all specified examples are covered.
2 participants
No description provided.