Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your test suite is strong and covers many core rules (function existence, boolean return, valid examples, dot rules, consecutive dots, domain starting with a dot, and missing domain extension). However, a few specification-critical cases are missing and must be added to fully enforce the requirements:
- Exactly one
@: Add a test insrc/validateEmail.test.js(near the existing@-related tests) to reject multiple@symbols:expect(validateEmail('a@b@c.com')).toBe(false). This enforces the “exactly one @” rule. - Allowed characters in
personal_info: In the valid-email block, include an underscore case to confirm it’s accepted:expect(validateEmail('user_name@mail.com')).toBe(true). - Allowed characters in
domain: In the valid-email block, add cases showing digits and hyphens are valid:expect(validateEmail('test@mail1-domain.com')).toBe(true).
Minor (nice-to-have) improvements: expand forbidden-character coverage (e.g., expect(validateEmail('test+plus@mail.com')).toBe(false)) and consider a test rejecting a domain ending with a dot (test@mail.com.). Once these critical tests are added, the suite will align well with the spec. Great job on the structure and clarity so far—these additions will make it complete.
✨ 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 return false when email does not contain @`, () => { | ||
| expect(validateEmail('testmail.com')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add a test that ensures emails with more than one @ are rejected. The spec requires exactly one @ separating personal_info and domain (personal_info@domain). Example assertion to add after the existing @-related test:
expect(validateEmail('a@b@c.com')).toBe(false);This enforces the "exactly one @" rule from the description.
| expect(typeof validateEmail('test@mail.com')).toBe('boolean'); | ||
| }); | ||
|
|
||
| it(`should return true for a valid email`, () => { |
There was a problem hiding this comment.
Extend the valid-email cases to explicitly cover other allowed characters and domain rules. The description requires personal_info to allow underscore and digits/hyphen/dots according to constraints, and the domain to allow digits and hyphens. Add assertions such as:
expect(validateEmail('user_name@mail.com')).toBe(true);
expect(validateEmail('test@mail1-domain.com')).toBe(true);Place these in the should return true for a valid email block so the suite verifies allowed characters for both parts.
| }); | ||
|
|
||
| // write more tests here | ||
| it(`should return false when forbidden characters are used`, () => { |
There was a problem hiding this comment.
Consider adding one more forbidden-character test to increase confidence that the implementation rejects the disallowed set described (e.g. + or *). Currently you test ! and &, which is fine per the hint to focus on priority cases, but adding another will make intent clearer. Example (optional):
expect(validateEmail('test+plus@mail.com')).toBe(false);This should be added to the should return false when forbidden characters are used block if you want slightly broader coverage.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your test suite is strong and covers most email validation rules well. However, there are two critical missing tests that block full compliance with the requirement that both parts of the email must exist (personal_info@domain).
-
Critical issues to fix:
- Add a test for a missing local part (no characters before
@). This should be placed after the "missing domain extension" test around lines 45–46. Example:it('should return false when local part is missing', () => { expect(validateEmail('@domain.com')).toBe(false); });
- Add a test for a missing domain part (no characters after
@). Place it near the test above. Example:it('should return false when domain part is missing', () => { expect(validateEmail('user@')).toBe(false); });
- Add a test for a missing local part (no characters before
-
Minor (optional) improvement: Consider adding a test rejecting invalid characters in the domain (e.g., underscore), to align with the allowed domain character set.
Once these two critical tests are added, the suite will fully meet the stated requirements. Great work on the comprehensive coverage elsewhere!
✨ 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 return false for missing domain extension`, () => { | ||
| expect(validateEmail('test@mail')).toBe(false); |
There was a problem hiding this comment.
Add a test that verifies a missing local part (no characters before @) is invalid. The description requires both parts to exist (personal_info@domain). Example to add:
it(`should return false when local part is missing`, () => {
expect(validateEmail('@domain.com')).toBe(false);
});A good insertion point is after the "missing domain extension" test (around lines 45–46).
| it(`should return false for missing domain extension`, () => { | ||
| expect(validateEmail('test@mail')).toBe(false); |
There was a problem hiding this comment.
Add a test that verifies a missing domain part (no characters after @) is invalid. The spec includes user@ as an invalid example. Example to add:
it(`should return false when domain part is missing`, () => {
expect(validateEmail('user@')).toBe(false);
});Place this near the other tests that check presence/absence of parts (for example after or next to the test suggested above).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution. You’ve built a strong, clear Jest suite that covers the core email validation rules well, including structure (exactly one @, missing parts), local-part rules (allowed underscore, dot placement), and domain basics (digits/hyphens allowed, cannot start with a dot). Educational override: there’s one small improvement opportunity to align exactly with the spec — add a negative test ensuring the domain disallows underscores.
- Specific fix: after the existing domain-related tests (around lines 65–66), add a test like:
This enforces the domain character set requirement (letters, digits, hyphens, dots only), and clarifies the edge case.
it('should return false when domain contains invalid character (underscore)', () => { expect(validateEmail('user@dom_ain.com')).toBe(false); });
Optional improvements: consider one more negative test for another forbidden domain character (e.g., +), though not strictly required. Great job overall—clean tests, meaningful cases, and strong alignment with the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| // write more tests here | ||
| it(`should allow digits and hyphens in domain`, () => { | ||
| expect(validateEmail('test@mail1-domain.com')).toBe(true); |
There was a problem hiding this comment.
Consider adding a test that rejects invalid characters in the domain (the domain must allow only letters, digits, hyphens and dots — underscores are not allowed). This is a small/optional missing case that makes the suite align exactly with the specification. Example test to add near other domain tests:
it('should return false when domain contains invalid character (underscore)', () => {
expect(validateEmail('user@dom_ain.com')).toBe(false);
});Placing it after the existing domain-related tests (for example after the test on line 65–66) keeps similar checks grouped together and improves clarity.
No description provided.