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 setting up a comprehensive test suite for the validateRegisterForm function. The structure is good, and you've covered many validation scenarios correctly. However, a few changes are needed to fully meet the requirements.
There is one test that contradicts the requirements: the test for emails with special characters like ! # $ % & should be a success case, not an error case. Additionally, two required test scenarios are missing:
- A test for a valid password containing Cyrillic letters (e.g.,
'passwordValidПароль1!'). - A test to ensure an email is invalid if it ends with a dot (e.g.,
'test@mail.com.').
Please address these points to ensure your tests fully cover all the specified requirements. Keep up the great work!
✨ 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
|
|
||
| expect(invalidPassword.code).toBe(422); | ||
| expect(invalidPassword.message).toBe('Password is invalid.'); | ||
| }); |
There was a problem hiding this comment.
The requirements state that valid passwords can contain Cyrillic letters (Aa-Яя). It's important to add a test case to ensure this functionality is covered, for example, a test with a valid Cyrillic password.
src/validateRegisterForm.test.js
Outdated
| it(`should return error when we have characters # $ % &`, () => { | ||
| const invalidPassword = validateRegisterForm('t#es$tmyk%&@mail.com', | ||
| 'P@ssword1!'); | ||
|
|
||
| expect(invalidPassword.code).toBe(422); | ||
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); |
There was a problem hiding this comment.
This test case contradicts the requirements. The description specifies that characters like ! # $ % & are valid in an email. This test should be changed to a success case, expecting a 200 status code for an email like 't#es$tmyk%&@mail.com'.
|
|
||
| expect(invalidPassword.code).toBe(422); | ||
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); |
There was a problem hiding this comment.
The requirements mention that an email cannot end with a dot. A test case for this scenario is missing. Please add one to ensure full coverage of the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the new tests and addressing the previous feedback! I'm requesting a couple of changes to fix two issues in the new tests.
- The test for Cyrillic passwords is using an invalid email (
'test@mail.com.') but expects a successful validation (code: 200). Because the email is invalid, the test should fail. Please use a valid email address in this test to correctly verify the password logic. - The test for special characters in an email is not comprehensive enough. The requirements state that emails can contain characters like
! # $ % & ' * +, but the current test only covers_and-. Please add a new test case that includes several of the other allowed special characters.
Once these two test cases are corrected, this will be ready to go. Keep up the great work!
✨ 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 pass if we have some characters`, () => { | ||
| const isValid = validateRegisterForm('test_nik-ch@mail.com', 'P@ssword1!'); | ||
|
|
||
| expect(isValid.code).toBe(200); | ||
| expect(isValid.message).toBe('Email and password are valid.'); | ||
| }); |
There was a problem hiding this comment.
It's good that you've added a test for special characters in the email. However, this only covers _ and -. The requirements specify that emails can also contain other characters like ! # $ % & ' * + / = ? ^ { | } ~`. Could you add another test case that includes some of these other characters to make sure they are also handled correctly?
| it(`should pass if password containing Cyrillic letters`, () => { | ||
| const invalidPassword = validateRegisterForm('test@mail.com.', | ||
| 'Passwпароль1@'); | ||
|
|
||
| expect(invalidPassword.code).toBe(200); | ||
| expect(invalidPassword.message).toBe('Email and password are valid.'); | ||
| }); |
There was a problem hiding this comment.
This test is a great addition for checking Cyrillic characters in passwords! However, the email you're using here ('test@mail.com.') is invalid because it ends with a dot. Since the email is invalid, the function should return an email validation error (code: 422), not a success code (200). To test the password correctly, you should use a valid email.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job fixing the test case for Cyrillic passwords! Your solution is very close to perfect. However, the tests are still missing coverage for the full range of special characters allowed in email addresses, which was a key part of the previous review. The requirements specify that characters like ! # $ % & ' * + should be valid. To help you keep moving forward, I am approving this submission as an educational override. For future reference, please remember to add a new test case to verify that emails containing these special characters are handled correctly. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| expect(isValid.message).toBe('Email and password are valid.'); | ||
| }); | ||
|
|
||
| it(`should pass if we have some characters`, () => { |
There was a problem hiding this comment.
This test is a good start, but it only validates emails containing _ and -. The requirements specify a much wider range of allowed special characters (e.g., ! # $ % & ' * +). As requested in the previous review, please add a new, more comprehensive test case to ensure these other characters are also handled correctly.
2 participants
No description provided.