Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the test suite! It effectively covers most of the core logic for the restoreNames function. However, I'm requesting a couple of changes to ensure all requirements are fully tested. The main issue is that the tests don't verify that the function returns undefined, which is a specific requirement. Please add an assertion to one of your tests to check for this.
Additionally, two of your test descriptions in src/restoreNames.test.js are a bit misleading as they mention a 'return' value. Since the function mutates the array and doesn't return anything, please update these descriptions to reflect the action taken, such as 'should handle an empty array without errors'. Once these adjustments are made, your solution will be perfect!
✨ 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/restoreNames.test.js
Outdated
| ]); | ||
| }); | ||
|
|
||
| it('should return {} if users.length = 0', () => { |
There was a problem hiding this comment.
This test description is a bit misleading. The requirements state that the restoreNames function should not return anything. A description like 'should handle an empty array without errors' would be more accurate.
src/restoreNames.test.js
Outdated
| }); | ||
|
|
||
| // write tests here | ||
| it(`should return initial result if firstname isn't empty`, () => { |
There was a problem hiding this comment.
Similar to the test for an empty array, this description mentions 'return'. It would be clearer to describe what happens to the input data, for example: 'should not change users who already have a firstName'.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your solution as you've successfully addressed all the feedback from the previous review. The new test case to verify the undefined return value is perfect, and the updated test descriptions are now accurate. Your test suite now completely covers all the requirements for the restoreNames function. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.