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 setting up a comprehensive test suite! The structure is solid and you've covered most of the requirements. However, there are two critical issues in your tests that need to be fixed before this can be approved.
First, several tests for valid emails use a format where the domain ends with a dot, like 'test838@gmail.com.'. This is not a valid email format, so these tests are incorrectly asserting that an invalid email should be considered valid. Please remove the trailing dot from these test cases.
Second, the test for disallowed characters in the personal_info part is not working as intended. Using characters.split() creates an array with only one element (the full string), so your loop only tests for the ! character. To ensure every disallowed character is tested, please change this to characters.split('').
✨ 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`, () => { | ||
| expect(validateEmail('test838@gmail.com.')) | ||
| .toBeTruthy(); | ||
| expect(validateEmail('test838@gmail.com.')).toBeTruthy(); |
There was a problem hiding this comment.
This test case, along with several others in this file, uses an email address where the domain ends with a dot (.). According to domain name standards, this is typically not a valid format. To ensure you're testing for a genuinely valid email, please remove the trailing dot from this and other similar test cases.
src/validateEmail.test.js
Outdated
| it(`'!$%&'*+/=?^{|}~' are not allowed in personal_info part`, () => { | ||
| const characters = `!$%&*+/=?^{|}~`; | ||
|
|
||
| characters.split().forEach((char, index) => { |
There was a problem hiding this comment.
The split() method without an argument will not split the string into individual characters. It will create an array with the entire string as its only element, meaning your loop only runs once. To iterate over and test each character in the characters string, you should use split('').
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing most of the feedback from the previous review! Your test suite is very close to being complete. However, a couple of changes are needed before this can be approved.
There are two issues to resolve in src/validateEmail.test.js:
- One test case incorrectly asserts that an email with a domain ending in a dot is valid. The test
'domain can contain letters, digits, hyphens, and dots'uses'personal_part@g-m-a159il.com.', but domains cannot end with a dot. Please remove the trailing dot to make it a valid email for this test. - The test for disallowed characters is missing the single quote character (
'). Please add it to your test string to ensure all required characters are being checked.
Once these two items are fixed, your solution will be ready!
✨ 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(`personal_info can NOT contain character '.' at the end`, () => { | ||
| expect(validateEmail('--.--.--.--.@gmail.com.')).toBeFalsy(); |
There was a problem hiding this comment.
This test case is intended to check if the personal_info part ends with a dot. However, the email used here has two potential invalid parts: the personal_info ends with a dot, and the domain also ends with a dot. It's better to test for one condition at a time. Please remove the trailing dot from the domain (gmail.com) to ensure this test is specifically for the personal_info part.
src/validateEmail.test.js
Outdated
| }); | ||
|
|
||
| it(`'!$%&'*+/=?^{|}~' are not allowed in personal_info part`, () => { | ||
| const characters = `!$%&*+/=?^{|}~`; |
There was a problem hiding this comment.
The list of disallowed characters in the requirements is ! $ % & ' * + / = ? ^ { | } ~. Your string is missing the single quote character ('). Please ensure your test covers all the specified disallowed characters.
src/validateEmail.test.js
Outdated
| }); | ||
|
|
||
| it('domain can contain letters, digits, hyphens, and dots', () => { | ||
| expect(validateEmail('personal_part@g-m-a159il.com.')).toBeTruthy(); |
There was a problem hiding this comment.
This test case asserts that an email with a domain ending in a dot is valid, which is incorrect and contradicts the feedback from the previous review. A domain should not end with a dot. To fix this, please remove the trailing dot from the email address to make it a valid test case.
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 revision! I am approving this solution as an educational override so you can keep moving forward.
You've successfully addressed all the previous feedback, and your test suite is now very comprehensive. The only thing missing is a test case for an email with multiple @ symbols. The requirements state an email must contain exactly one @, so an input like 'test@test@gmail.com' should also be invalid. Adding that one final test would make your suite complete. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| it(`@ is required`, () => { | ||
| expect(validateEmail('test838gmail.com')).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
This test correctly checks for a missing @ symbol. To fully cover the requirement that an email has exactly one @, please also add a test case for an email with multiple @ symbols (e.g., 'test@test@gmail.com'). This should also be treated as an invalid email.
2 participants
No description provided.