Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the test suite does not yet cover all required examples and contains a mislabeled and duplicated test, which are blockers for validating the task requirements. Specifically, in src/arrayReverse.test.js there is a test whose title says it checks the empty-string case, but its body asserts ['Mate', 'Academy']; either update the description to match the assertion or, better, change the body to actually test arrayReverse(['']) (for example at the current line where that mismatched test is defined). Another critical gap is that the task description explicitly requires the example arrayReverse(['Hell0']) === ['0lleH'], but your tests only cover ['Hello']; add a dedicated test like expect(arrayReverse(['Hell0'])).toEqual(['0lleH']) near your other single-word tests. There is also a duplicate ['Mate','Academy'] case: it’s currently asserted in two separate tests; keep one clearly named test for this example and remove the redundant one so the suite stays clear and non-repetitive. As a minor improvement (not blocking), consider adding a test that includes an empty string among other elements (e.g. ['', 'abc']) and fixing small grammar issues in test names to improve readability. Once you address these issues, your test suite will more completely and accurately validate the required behavior of arrayReverse, and the solution should be ready to pass.
✨ 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
| }); | ||
|
|
||
| // write more tests here | ||
| it('should reverses a single word', () => { |
There was a problem hiding this comment.
The test title says it should check the empty-string case, but the assertion checks ['Mate', 'Academy']. Either change the test body to expect(arrayReverse([''])).toEqual(['']) or update the test title and description to match the current assertion.
src/arrayReverse.test.js
Outdated
|
|
||
| // write more tests here | ||
| it('should reverses a single word', () => { | ||
| expect(arrayReverse(['Hello'])).toEqual(['olleH']); |
There was a problem hiding this comment.
The task description requires the example arrayReverse(['Hell0']) === ['0lleH']. Add a test for this exact case (you currently have a single-word test for ['Hello'], but the required example is missing).
| }); | ||
|
|
||
| it('should reverses two words and preserves their lengths', () => { | ||
| expect(arrayReverse(['Mate', 'Academy'])).toEqual(['ymed', 'acAetaM']); |
There was a problem hiding this comment.
The ['Mate', 'Academy'] example is asserted earlier (mismatched test) and again here, causing duplication. Keep a single clearly-named test for this example and remove the redundant one to avoid overlap.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one explicit requirement from the task description is still missing from your tests, which means the task is not fully compliant yet. The instructions require the exact example arrayReverse(['Hell0']) === ['0lleH'] to be covered; currently, src/arrayReverse.test.js does not contain this specific test case. To fix this, add a dedicated test near the other single-word tests, for example:
expect(arrayReverse(['Hell0'])).toEqual(['0lleH']);This ensures the behavior mandated by the task description is explicitly and automatically verified. As an optional improvement (not blocking), you could also add a mixed-case test like expect(arrayReverse(['', 'abc'])).toEqual(['cba', '']); after your empty-string test to broaden edge-case coverage. Overall, your existing tests are well-structured and cover most behaviors; once you add the required Hell0 test, the suite should fully meet the assignment 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/arrayReverse.test.js
Outdated
| it('should reverses a single word', () => { | ||
| expect(arrayReverse(['Hello'])).toEqual(['olleH']); |
There was a problem hiding this comment.
The task description requires the exact example arrayReverse(['Hell0']) === ['0lleH']. Add a dedicated test such as:
expect(arrayReverse(['Hell0'])).toEqual(['0lleH']);Place it near the other single-word tests so the required example is explicitly verified (see description) .
|
|
||
| it(`should return an empty string | ||
| if original array consists of an empty string`, () => { | ||
| expect(arrayReverse([''])).toEqual(['']); |
There was a problem hiding this comment.
Optional improvement: add a test that includes an empty string among other elements to cover mixed cases, for example:
expect(arrayReverse(['', 'abc'])).toEqual(['cba', '']);Consider adding this after the existing empty-string test to improve edge-case coverage.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because your Jest tests correctly cover all the required behaviors for arrayReverse, including the specified examples like arrayReverse(['Hell0']) === ['0lleH'] and the multi-word cases with special characters. The tests are well-structured, use appropriate matchers such as toBeInstanceOf and toEqual, and clearly verify that the function is declared and returns an array.
This approval is an educational override to help you progress while learning, because there are still a couple of small improvements you could make. For example, consider adding a test that explicitly checks what should happen with an empty input array, e.g. expect(arrayReverse([])).toEqual([]), to define the behavior for that edge case. It would also be helpful to add a test that ensures the original input array is not mutated after calling arrayReverse, by comparing the array before and after the call. As a minor detail, you could reword the test description about the empty string case to make it clearer that the function returns an array containing an empty string, not just an empty string by itself. Overall, your work demonstrates a strong understanding of the requirements and Jest testing basics—nice job, and keep building on this foundation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.