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 the current test suite does not fully match the specified email validation rules, which can lead to incorrect behavior being accepted as correct. Specifically, there are several "valid" tests in src/validateEmail.test.js that use emails with a trailing dot after the domain (e.g. 1234@gmail.com. at its line, similarly 123-4@gmail.com. and 123_4@gmail.com.), but a trailing . after the domain should make the email invalid according to the spec. These inputs should be updated to 1234@gmail.com, 123-4@gmail.com, and 123_4@gmail.com so that the tests verify proper handling of digits, hyphens, and underscores in the personal_info part without accepting malformed domains.
In addition, while you do test one invalid character (test]@mail.com), the requirements explicitly list a set of forbidden characters like ! $ % & ' * + / = ? ^ { | } ~, and there is currently no test that directly uses one of these. Please add at least one negative test case such as test!@mail.com to assert that an email containing one of these forbidden characters in personal_info is rejected. Once these tests are corrected and the new negative test is added, your suite will better enforce the intended validation rules. Overall, your coverage of @ presence, dot placement, and double-dot rules is solid, and with these adjustments your tests will align well with the specification.
✨ 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/validateEmail.test.js
Outdated
|
|
||
| it(`should return 'true' for the valid email | ||
| with just digits`, () => { | ||
| expect(validateEmail('1234@gmail.com.')).toBeTruthy(); |
There was a problem hiding this comment.
This test expects a valid result for 1234@gmail.com. which has a trailing dot after the domain. The domain shouldn't include a trailing . per the realistic validation rules — change the input to 1234@gmail.com so the test matches the spec.
src/validateEmail.test.js
Outdated
|
|
||
| it(`should return 'true' for the valid email | ||
| with character '-'`, () => { | ||
| expect(validateEmail('123-4@gmail.com.')).toBeTruthy(); |
There was a problem hiding this comment.
This test expects 123-4@gmail.com. to be valid but it ends with an extra .. Remove the trailing dot (123-4@gmail.com) to correctly test a - in the personal_info part.
src/validateEmail.test.js
Outdated
|
|
||
| it(`should return 'true' for the valid email | ||
| with character '_'`, () => { | ||
| expect(validateEmail('123_4@gmail.com.')).toBeTruthy(); |
There was a problem hiding this comment.
This test expects 123_4@gmail.com. to be valid but contains a trailing .. Use 123_4@gmail.com to accurately test underscore support in personal_info.
src/validateEmail.test.js
Outdated
| }); | ||
|
|
||
| it(`should return 'false' for email with not allowed character`, () => { | ||
| const isEmailValid = validateEmail('test]@mail.com'); |
There was a problem hiding this comment.
The negative test uses test]@mail.com which is fine as invalid — but the task lists specific forbidden characters (for example ! $ % & ' * + / = ? ^ { | } ~). Add at least one test using one of those explicitly forbidden characters (e.g. test!@mail.com) so the suite verifies the listed rule directly.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because the test suite clearly exercises the core email validation requirements and there are no critical blockers. You cover the presence of @, valid and invalid personal_info characters (letters, digits, -, _, .), dot-placement rules (not at the start or end and no consecutive dots), and domain structure including the need for a top-level dot. You also correctly include negative tests for forbidden characters (like }) and reject non-ASCII input such as Cyrillic, which matches the specification. One small gap is that domains containing digits and hyphens (for example user@mail-server1.com) are not explicitly tested, so adding a positive test like expect(validateEmail('user@mail-server1.com')).toBe(true); would further strengthen coverage. As a minor enhancement, you could add one more forbidden-character negative test (e.g. test!@mail.com) to make the intent of that rule even clearer. Overall, your tests are focused, well-structured, and meet the task’s functional requirements—nice work.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.