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 well-structured, but a few critical spec requirements are not yet covered, which means the suite doesn’t fully enforce the described behavior.
Critical issues to fix (blockers):
- Exactly one
@requirement is untested. Add a negative test for multiple@symbols (e.g., at src/validateEmail.test.js near the existing missing-@test):- it should assert
validateEmail('a@b@c.com')returnsfalse.
- it should assert
- Domain character-class rule is under-specified. Add tests ensuring only letters, digits, hyphens, and dots are allowed in the domain (place near other domain tests):
- Negative:
validateEmail('user@ma_il.com')→false - Negative:
validateEmail('user@ma!l.com')→false - Positive:
validateEmail('user@mail-domain.com')→true
- Negative:
Additional minor improvements (non-blockers):
- Include the exact example from the description:
validateEmail('false@email')→falsenear your “domain missing dot” test for parity with the spec. - Your
invalidCharsset for personal_info includes a backtick. If strict parity with the description is desired, remove it; otherwise keep it intentionally and note the stricter rule.
Once these tests are added, the suite will comprehensively enforce the spec. Nice job on clarity and coverage 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 for email missing @ symbol`, () => { | ||
| expect(validateEmail('testmail.com')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add a test that rejects emails containing multiple @ symbols. The description requires exactly one @ separating personal_info and domain; currently the suite only tests the missing-@ case. Example to add near this test block:
it('should return false for email with multiple @ symbols', () => {
expect(validateEmail('a@b@c.com')).toBe(false);
});Place this near the existing missing @ test so it's clear and grouped with @-related checks.
| it(`should return false for domain starting with dot`, () => { | ||
| expect(validateEmail('user@.mail.com')).toBe(false); |
There was a problem hiding this comment.
Add tests that ensure the domain rejects characters not allowed by the spec (domain may contain only letters, digits, hyphens and dots). Examples to add near the other domain tests:
it('should return false for domain containing underscore', () => {
expect(validateEmail('user@ma_il.com')).toBe(false);
});
it('should return false for domain containing invalid char', () => {
expect(validateEmail('user@ma!l.com')).toBe(false);
});
// Positive example for allowed hyphen
it('should allow hyphens in domain', () => {
expect(validateEmail('user@mail-domain.com')).toBe(true);
});This verifies the domain character-class requirement from the description.
| it(`should return false for domain missing dot`, () => { | ||
| expect(validateEmail('user@mail')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
The description uses the example validateEmail('false@email') === false. Add that explicit example to the invalid cases to match the specification precisely. For example:
it('should return false for email missing top-level domain (example from description)', () => {
expect(validateEmail('false@email')).toBe(false);
});Add this near the domain missing dot test.
src/validateEmail.test.js
Outdated
| }); | ||
|
|
||
| it(`should return false for personal info with invalid characters`, () => { | ||
| const invalidChars = "!$%&'*+/=?^`{|}~"; |
There was a problem hiding this comment.
Note about the invalidChars string used for personal_info invalid characters: it currently includes a backtick () at this line. The description's forbidden list doesn't explicitly show a backtick; if you intended to forbid it that's fine (stricter validation). If you want exact parity with the specification, remove the backtick from the string "!$%&'*+/=?^{|}~".
No change required if the stricter set was intentional — just be aware of the difference.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: APPROVE
Your Jest test suite for validateEmail meets the specification and checklist. It thoroughly covers the core rules: exactly one @, personal_info dot rules (no leading/trailing or consecutive dots), allowed/disallowed characters, domain structure constraints, and the exact example cases (test@mail.com, t@q.c, false@email). The tests are clear, well-organized, and use appropriate Jest expectations.
Minor, non-blocking suggestions to strengthen coverage:
- Add a positive case showing digits in the domain (e.g.,
user@123mail.com). - Optionally add a mixed-case example (e.g.,
User@Mail.COM) and an empty personal_info negative case (@mail.com). - Ensure the implementation file exists at
src/validateEmail.jsand is exported as required byrequire('./validateEmail')insrc/validateEmail.test.js:1.
Nice work tightening the suite and aligning it with the spec.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.