Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great start on the test file! The structure is well-organized. I'm requesting changes because several key validation rules from the task description are not yet covered, and a couple of existing tests need correction.
Specifically, your tests are missing checks for the rules about using dots (.) in the email's personal info part (cannot start/end with a dot, or have consecutive dots). Additionally, the test for a valid email ('test838@gmail.com.') should be updated, as an email ending in a dot is typically invalid. Finally, the test for disallowed characters uses ], which is not on the required list; please use a character from the list provided in the task, such as ~ or ?.
Addressing these points will ensure your test suite fully covers the requirements.
✨ 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 not have any special character | ||
| on the end of personal info`, () => { | ||
| const result = validateEmail('test838]@gmail.com'); |
There was a problem hiding this comment.
While this test correctly checks for an invalid character, the character ] is not on the list of disallowed characters provided in the task description: ! $ % & ' * + / = ? ^ { | } ~.
To make this test align with the requirements, please use one of the characters from that specific list.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the tests for leading and trailing dots in the personal info section! However, I'm requesting changes because there are a few critical test cases missing and one core test is incorrect.
Specifically, please address the following:
- Incorrect Valid Email Test: In
src/validateEmail.test.js, the testit('should return 'true' for the valid email')uses'test838@gmail.com.'. This email is invalid due to the trailing dot in the domain, so the test should be updated with a truly valid email like'test838@gmail.com'. - Missing Consecutive Dots Test: The requirements state that consecutive dots (
..) are not allowed in the part before the@. Please add a test case to ensure an email like'test..123@gmail.com'is correctly identified as invalid. - Missing Domain Validation: A domain must contain a dot (e.g.,
.com). A test is needed to verify that an email like'false@email'returnsfalse, as specified in the task description.
You're very close to a complete and robust test suite. Keep up the great work!
✨ 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
|
|
||
| // write more tests here | ||
| it('should have only english letters', () => { | ||
| const result = validateEmail('teść-żółty838@gmail.com.'); |
There was a problem hiding this comment.
While this test will likely pass correctly because of the non-English characters (ść, ół), the email string used here also ends with a trailing dot, which is another reason for it to be invalid.
It's best practice for a unit test to check for a single condition. To make the purpose of this test clearer, please remove the trailing dot so it only fails because of the invalid characters.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job fixing the test case for the trailing dot—that's an important correction! I'm approving this submission to keep you moving forward, but a few key test cases required by the task are still missing.
To make your test suite fully comprehensive, please add tests for the following scenarios:
- Consecutive dots in the username (e.g.,
'test..123@gmail.com') - A domain without a dot (e.g.,
'false@email') - An email with multiple
@symbols (e.g.,'test@example@com')
Adding these checks will ensure all requirements are covered. You're making excellent progress!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| expect(result).toBeFalsy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
You're making good progress, but a few key test cases required by the task description are still missing. Please add tests for the following scenarios:
-
Consecutive dots: The requirements state that dots cannot appear one after another in the personal info part. A test for an email like
'test..123@gmail.com'is needed. -
Domain must have a dot: The example
validateEmail('false@email') === falsefrom the description implies that the domain part must contain a dot. Please add a test for this specific example. -
Multiple
@symbols: An email should only have a single@symbol. A test case for an email like'test@example@com'would be a great addition to ensure this is handled correctly.
No description provided.