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 the tests for the validateEmail function! The structure is clear and you've covered most of the requirements. However, I'm requesting a few changes to ensure the tests are fully correct and comprehensive.
There is one test case that is incorrect. The test for 'test838@gmail.com.' expects a true result, but an email's domain should not end with a dot. This test should be updated to expect false.
Additionally, a couple of required test scenarios are missing. Please add tests to verify that an email is invalid if:
- The
personal_infopart ends with a dot (e.g.,'test.@gmail.com') - The
domainpart starts with a dot (e.g.,'test@.gmail.com')
Addressing these points will make your test suite robust and fully aligned with the 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
src/validateEmail.test.js
Outdated
| expect(validateEmail('t@q.c')).toBe(true); | ||
| expect(validateEmail('user_123@gmail.com')).toBe(true); | ||
| expect(validateEmail('my-test.email@yahoo.com')).toBe(true); | ||
| expect(validateEmail('test838@gmail.com.')).toBe(true); |
There was a problem hiding this comment.
This test case appears to be incorrect. Standard email format rules typically don't allow a domain name to end with a dot. Based on the requirements, this should likely be a test for a false outcome.
| it(`should return false if personal part starts with dot`, () => { | ||
| expect(validateEmail('.test@gmail.com')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
This test correctly covers the rule for the personal part not starting with a dot. However, the requirements mention two other similar rules that are not yet tested:
- The
personal_infopart cannot end with a dot (e.g.,'test.@gmail.com'). - The
domainpart cannot start with a dot (e.g.,'test@.gmail.com').
Please add test cases for these scenarios 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
Excellent work on this iteration! You've done a great job addressing the previous feedback, and your test suite is now very robust.
I am requesting one small but important change to fully meet the task requirements. In src/validateEmail.js, your regular expression allows underscores (_) in the domain part of the email, likely because \w is being used. The task description specifies that the domain should only contain 'letters, digits, hyphens, and dots'.
To resolve this, please update the regex to ensure it doesn't permit underscores in the domain. You're very close to a perfect solution!
✨ 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
| const validEmailMask = new RegExp( | ||
| /^([\w-]+(?:\.[\w-]+)*)@((?:[\w-]+\.)*\w[\w-]{0,66})\./i, | ||
| ); | ||
| const validEmailMask = /^[\w-]+(?:\.[\w-]+)*@[\w-]+(?:\.[\w-]+)+$/i; |
There was a problem hiding this comment.
This regular expression allows underscores (_) in the domain part of the email because \w matches [A-Za-z0-9_]. However, the task description specifies that the domain part should only contain 'letters, digits, hyphens, and dots'. Please adjust the domain part of the regex to exclude underscores.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed all the previous feedback. The regular expression has been perfected to correctly validate the domain part of the email, and your updated test suite confirms the fix. The solution is now robust, clean, and fully compliant with all task requirements. I am approving your submission, great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.