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 implementing the email validation function and writing a comprehensive test suite. However, some changes are required before this can be approved. The primary issue is that the regular expression in src/validateEmail.js does not allow for hyphens (-) or underscores (_) in the personal information part of the email, which is a core requirement. Please update the regex to support these characters. To ensure this works correctly, please also add a positive test case in src/validateEmail.test.js for an email containing these characters, such as test-user@domain.com. Lastly, please clean up the test file by removing duplicate tests and correcting a few misleading test descriptions. Once these issues are addressed, your solution will be in great shape!
✨ 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.js
Outdated
| function validateEmail(email) { | ||
| // eslint-disable-next-line | ||
| const validEmailMask = new RegExp(/^([\w-]+(?:\.[\w-]+)*)@((?:[\w-]+\.)*\w[\w-]{0,66})\./i); | ||
| const validEmailMask = /^[A-Za-z0-9]+(?:\.[A-Za-z0-9]+)*@[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)+$/; |
There was a problem hiding this comment.
This regular expression doesn't fully match the requirements. The personal_info part of the email should allow for - and _ characters, but they are not included in the character set for the part before the @ symbol.
| it(`should be 'false' for 'personal_info' without number`, () => { | ||
| expect(validateEmail('test@gmail.com.')).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
The description of this test is misleading. According to the requirements, digits are allowed in the personal_info part, but they are not mandatory. An email like 'test@gmail.com' should be valid. Your test passes only because the provided email 'test@gmail.com.' is invalid due to the trailing dot.
src/validateEmail.test.js
Outdated
| it(`should be 'false' if 'personal_info' without '-' and '_' chars`, () => { | ||
| expect(validateEmail('test@gmail.com.')).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Similar to the previous test, the characters - and _ are allowed, not mandatory. This test passes for the wrong reason (the trailing dot). You should also add a positive test case to ensure that emails with - or _ in the personal_info part are considered valid, as required. For example, test-name@domain.com.
src/validateEmail.test.js
Outdated
| it(`should be 'false' if in 'personal_info' the first is '.'`, () => { | ||
| expect(validateEmail('.test838@gmail.com')).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
This test case is a duplicate of the one on lines 46-48. Please remove one of them to avoid redundancy.
src/validateEmail.test.js
Outdated
| it(`should return false if 'personal_info' not include '@'`, () => { | ||
| expect(validateEmail('test838gmail.com')).toBeFalsy(); | ||
| }); | ||
|
|
||
| it(`should return false if 'personal_info' not include '@'`, () => { | ||
| expect(validateEmail('test838gmail.com')).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
This test case is also duplicated. One of them should be removed.
src/validateEmail.test.js
Outdated
| it(`should return true if 'domain' with digits`, () => { | ||
| expect(validateEmail('test838@gmail.c-om')).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
The test description mentions 'digits', but the email being tested, 'test838@gmail.c-om', contains a hyphen in the domain part. Please make the description match the test case.
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 core logic in src/validateEmail.js! The regular expression is well-crafted and meets the requirements. However, I'm requesting a few changes in your test file, src/validateEmail.test.js, to improve its accuracy and clarity.
The test with the description 'should be 'false' for 'personal_info' without number' is misleading. The provided email test@gmail.com. fails because of the trailing dot, not because it lacks a number, as numbers are not required in the personal info part. Please update the test to check for a valid condition, such as the trailing dot.
Additionally, another test case, 'тест838@gmail.com.', is invalid for two reasons (non-Latin characters and a trailing dot). For tests to be precise, they should ideally check for a single condition. Please remove the trailing dot to focus the test on handling non-Latin characters.
Once these test cases are corrected, this will be ready to go!
✨ 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 be'false' for 'personal_info' with non-Latin characters`, () => { | ||
| expect(validateEmail('тест838@gmail.com.')).toBeFalsy(); |
There was a problem hiding this comment.
This test case is invalid for two reasons: it contains non-Latin characters and it has a trailing dot. For clarity, it's best for a unit test to check for a single condition. Please remove the trailing dot to test specifically for the use of non-Latin characters.
| it(`should be 'false' for 'personal_info' without number`, () => { | ||
| expect(validateEmail('test@gmail.com.')).toBeFalsy(); |
There was a problem hiding this comment.
This test's description is misleading. The task requirements do not mandate that an email's personal_info part must include a number. The test passes only because the email provided, test@gmail.com., is invalid due to the trailing dot. The email test@gmail.com is valid and should pass. Please either remove this test or correct its description to accurately reflect what is being tested (e.g., an email with a trailing dot).
| expect(validateEmail('test838gmail.com')).toBeFalsy(); | ||
| }); | ||
|
|
||
| it(`should return false if 'personal_info' not include ':'`, () => { |
There was a problem hiding this comment.
The description "'personal_info' not include ':'" could be clearer. The test is checking for an email that does include a forbidden character. A better description would be something like "should return false if 'personal_info' includes a forbidden character like ':'".
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 core validation logic in src/validateEmail.js—it's implemented perfectly! I am approving this submission as an educational override so you can keep making progress, but please take a moment to review the feedback on your tests.
In src/validateEmail.test.js, a couple of test cases could be more precise:
- The test for non-Latin characters (
'тест838@gmail.com.') also includes a trailing dot. It's best practice for a test to check only one failure condition at a time, so please remove the dot. - The description
'should be 'false' for 'personal_info' without number'is misleading for the email'test@gmail.com.'. This email fails because of the trailing dot, not a missing number. Please update the description to accurately reflect this, for example:should be 'false' if domain ends with a dot.
Refining your tests this way is a crucial skill. You're doing a fantastic job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| }); | ||
|
|
||
| it(`should be'false' for 'personal_info' with non-Latin characters`, () => { | ||
| expect(validateEmail('тест838@gmail.com.')).toBeFalsy(); |
There was a problem hiding this comment.
This test case is checking for two failure conditions at once: non-Latin characters and a trailing dot. To make the test more specific, please remove the trailing dot so it only checks for the use of invalid characters.
| expect(validateEmail('тест838@gmail.com.')).toBeFalsy(); | ||
| }); | ||
|
|
||
| it(`should be 'false' for 'personal_info' without number`, () => { |
There was a problem hiding this comment.
The description for this test is misleading. The email test@gmail.com. is invalid because it ends with a dot, not because it's missing a number (which isn't a requirement). Please update the description to accurately reflect what's being tested, for example: should be 'false' if domain ends with a dot.
2 participants
No description provided.