Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your test suite is close, but there are several critical gaps that violate the task requirements and one test that contradicts the spec, so it can’t be approved yet.
Critical issues to fix:
- Update the misleading test title in
src/validateEmail.test.jswhere the case labeled “should return true for valid email with ending dot” actually testsvalidateEmail('test838@gmail.com'). Rename to something like “should return true for a valid email” or change the input/expectation to truly cover an ending dot case. - In the boolean type test, replace
validateEmail('test@gmail.com.')with a clearly valid canonical input liketest@mail.comto avoid implying trailing dots might be valid. - Remove the entire test “should return false for domain longer than 66 characters” (length checks are out of scope per the spec).
- Add the required missing tests to satisfy the rules:
- Examples:
validateEmail('test@mail.com') === true,validateEmail('t@q.c') === true,validateEmail('false@email') === false. - personal_info leading dot:
'.test@mail.com'=> false. - personal_info trailing dot:
'test.@mail.com'=> false. - domain leading dot:
'test@.mail.com'=> false. - multiple
@:'a@b@c.com'=> false. - disallowed ASCII punctuation in personal_info (e.g.,
'test+name@mail.com','test!name@mail.com') => false. - disallowed characters in domain (e.g.,
'test@mail!.com') => false.
- Examples:
Minor improvements: Keep test names precise and aligned with assertions.
You’ve covered useful cases (double dots, spaces, Cyrillic, hyphen/underscore). Address the above and this will fully meet the spec—nice work so far!
✨ 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 true for valid email with ending dot`, () => { | ||
| expect(validateEmail('test838@gmail.com')).toBe(true); |
There was a problem hiding this comment.
The test title is misleading: the test labeled should return true for valid email with ending dot asserts validateEmail('test838@gmail.com') which is a normal valid email (not an "ending dot" case). Update the test description to match the assertion (for example, "should return true for a valid email") or change the asserted email and expectation to reflect the intended case.
| }); | ||
|
|
||
| it(`should return boolean`, () => { | ||
| expect(typeof validateEmail('test@gmail.com.')).toBe('boolean'); |
There was a problem hiding this comment.
The should return boolean test uses validateEmail('test@gmail.com.') (trailing dot in the domain) as input for the type check. Using a clearly valid and simple example from the description (for example test@mail.com or t@q.c) will make this test clearer and avoid confusion about acceptance of trailing dots. Replace that input with a canonical valid example.
| it(`should return false for domain longer than 66 characters`, () => { | ||
| const longDomain = `test@${'a'.repeat(70)}.com`; | ||
|
|
||
| expect(validateEmail(longDomain)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
This test block checks domain length which is out of scope: the task explicitly states the function does not validate email length. Remove this entire test (should return false for domain longer than 66 characters) because length checks must not be tested here.
| it(`should return false for email with Cyrillic letters`, () => { | ||
| expect(validateEmail('тест@gmail.com')).toBe(false); | ||
| }); | ||
|
|
There was a problem hiding this comment.
You are missing several required tests described in the task. Please add explicit tests for the following (each should assert true/false according to the rules):
- Examples from the description:
validateEmail('test@mail.com') === true,validateEmail('t@q.c') === true,validateEmail('false@email') === false. - personal_info cannot start with dot:
'.test@mail.com'=> false. - personal_info cannot end with dot:
'test.@mail.com'=> false. - domain cannot start with dot:
'test@.mail.com'=> false. - Multiple @ signs (e.g.
'a@b@c.com') => false. - personal_info with disallowed ASCII punctuation from the list (! $ % & ' * + / = ? ^ { | } ~), e.g.
'test+name@mail.com'or'test!name@mail.com'=> false. - domain containing disallowed characters, e.g.
'test@mail!.com'=> false.
Add these tests after the existing cases so the suite covers the full set of rules required in the description.
mateacademy-ai-mentor
left a comment
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 of the required behaviors (valid/invalid examples, boolean return, single-@ checks, and leading/trailing dot rules). However, two critical specification checks are missing, which blocks full compliance:
- Missing positive tests for allowed characters in the local (personal_info) part: hyphen
-and underscore_. The spec explicitly permits these, so add tests like:- it('should return true for email with hyphen in local part', ...) using
first-last@mail.com - it('should return true for email with underscore in local part', ...) using
first_last@mail.com
- it('should return true for email with hyphen in local part', ...) using
- Missing negative test to reject consecutive dots in the local part. The spec states double dots are not allowed; add a test like
expect(validateEmail('test..name@mail.com')).toBe(false).
These are core requirements; without them, the suite doesn’t fully validate the implementation. As a minor improvement (optional), you could add a positive domain hyphen case (e.g., user@mail-server.com) and one more disallowed symbol in local part for broader coverage. Nice work overall—once these tests are added, the suite should meet the spec completely.
✨ 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 true for short valid email 't@q.c'`, () => { | ||
| expect(validateEmail('t@q.c')).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Add positive tests to confirm that hyphen (-) and underscore (_) are allowed in the personal_info (local) part. The spec explicitly permits these characters. Suggested tests (insert after the short valid email test):
it('should return true for email with hyphen in local part', () => {
expect(validateEmail('first-last@mail.com')).toBe(true);
});
it('should return true for email with underscore in local part', () => {
expect(validateEmail('first_last@mail.com')).toBe(true);
});These ensure the implementation accepts - and _ in the local part as required.
|
|
||
| it(`should return false for email with trailing dot before @`, () => { | ||
| expect(validateEmail('test.@mail.com')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add a negative test to ensure consecutive dots in the personal_info part are rejected (the spec states "double dots are not allowed in personal_info part"). Suggested test (insert near other local-part negative tests, for example after the trailing-dot test):
it('should return false for personal_info with consecutive dots', () => {
expect(validateEmail('test..name@mail.com')).toBe(false);
});This checks the implementation enforces the "no consecutive dots" rule.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational Approval: Approving to support your progress while learning. You’ve done a strong job covering core cases: existence and boolean return, valid examples (test@mail.com, t@q.c), local-part dot rules (no leading, trailing, or consecutive dots), allowed characters (hyphen/underscore), and domain constraints (no leading dot, hyphen handling).
Critical fix needed: On src/validateEmail.test.js, right after the existing type check around line 13 where you call validateEmail('invalid-email'), add an explicit negative assertion to satisfy the “exactly one @” requirement: expect(validateEmail('invalid-email')).toBe(false). This ensures the missing-@ path is clearly verified.
Minor improvements (optional):
- Add a positive test with digits:
user123@mail.comanduser@mail123.comto confirm digits are allowed in both parts. - Include one more disallowed character check (e.g.,
%or') to broaden coverage.
Great progress—your tests are focused and readable. With the missing-@ assertion added, your suite will fully match the requirement for exactly one @. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| it(`should return boolean`, () => { | ||
| expect(typeof validateEmail('test@mail.com')).toBe('boolean'); | ||
| expect(typeof validateEmail('invalid-email')).toBe('boolean'); |
There was a problem hiding this comment.
The test suite currently uses invalid-email only to check the return type (line 13) but does not assert that it is invalid. The description requires verifying that emails must contain exactly one @. Add an explicit negative assertion for a missing-@ case, for example:
expect(validateEmail('invalid-email')).toBe(false);Place this assertion right after the existing type check or add a dedicated it block for the missing-@ case so the requirement is clearly covered.
2 participants
No description provided.