Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because core requirements are missing and one test misinterprets a rule, which blocks full verification of the task.
Critical issues to fix:
- Missing required examples (must be explicit):
validateEmail('test@mail.com')should betrue.validateEmail('t@q.c')should betrue.validateEmail('false@email')should befalse.
Add these as separate tests in src/validateEmail.test.js (place them near your “valid”/“invalid” sections).
- Incorrect domain rule test and description:
- Requirement: the domain CANNOT start with a dot.
- Your current test checks a domain without any dot (e.g.,
gmailcom). Update the description and add a test for a leading-dot domain, e.g.,validateEmail('test838@.gmail.com')should befalse. - Ensure the test title clearly states: “should return false when the domain starts with a dot”.
Minor improvements:
- The test labeled as valid uses
test838@gmail.com.which ends with a dot; consider aligning with the provided valid examples for clarity.
You’ve covered many important cases—nice work. Add the required tests and correct the domain-leading-dot case, and this should be ready to approve.
✨ 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(`'false' if 'domain' didn't start with dot`, () => { | ||
| expect(validateEmail('test838@gmailcom')).toBe(false); |
There was a problem hiding this comment.
The test description and implementation are incorrect. The requirement is that the domain CANNOT start with a dot, but this test checks for a domain that does not contain a dot at all ('gmailcom'). Please add a test for a domain that starts with a dot (e.g., 'test838@.gmail.com') and ensure the description matches the requirement.
| it(`'false' if email has not allowed chars`, () => { | ||
| expect(validateEmail(`test838!$%&'*+/=?^{|}~@gmailcom`)).toBe(false); | ||
| }) | ||
| }); |
There was a problem hiding this comment.
You are missing required tests for these cases:
validateEmail('test@mail.com') === truevalidateEmail('t@q.c') === truevalidateEmail('false@email') === false
Please add these tests to fully satisfy the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: APPROVE
Your Jest test suite for validateEmail fully meets the specified requirements. It includes the three mandatory examples (test@mail.com → true, t@q.c → true, false@email → false) and thoroughly covers the rules for personal_info (allowed characters, no leading/trailing dot, no consecutive dots, and forbidden characters) and domain (allowed characters, no leading dot, must include a dot). The tests are clear, focused on realistic scenarios, and avoid unnecessary edge cases, aligning well with the priorities.
No critical blockers or missing requirements were found. Nice job mapping each requirement to explicit, readable test cases and addressing previous feedback. Keep up the solid work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.