Wrote test for main program#328
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is still not enforced by your tests. The task explicitly states that restoreNames must not return anything (it should return undefined), but your tests in src/restoreNames.test.js only verify array mutation and do not assert the function’s return value. This means checklist requirement about the return value is not actually checked by the test suite, so an incorrect implementation that returns a value would still pass.
To fix this, in each test where you call restoreNames(users), add an assertion like:
expect(restoreNames(users)).toBeUndefined();Place this immediately after the restoreNames(users); call so the test both checks the mutation of users and that the function returns undefined. Once you add these assertions to both relevant test cases (missing firstName and undefined firstName), your tests will fully enforce the specification and the solution will be in good shape. Overall, your coverage of the main behaviors is solid; this is just an important final detail to align the tests with 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
| }, | ||
| ]; | ||
|
|
||
| restoreNames(users); |
There was a problem hiding this comment.
Add an assertion that restoreNames returns undefined here to satisfy checklist item #5 (the function must not return anything). For example: expect(restoreNames(users)).toBeUndefined() — place it after the restoreNames(users); call so the test also verifies the return value alongside the mutated users array.
| }, | ||
| ]; | ||
|
|
||
| restoreNames(users); |
There was a problem hiding this comment.
Also add the same explicit return-value assertion in this test case as well: expect(restoreNames(users)).toBeUndefined(). This ensures both cases (absent field and undefined field) enforce the requirement that the function does not return a value (checklist item #5).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because your tests correctly and completely capture the required behavior of restoreNames. You verify that restoreNames is a function, that it mutates the users array in place, and that it restores firstName from fullName both when firstName is undefined and when it is missing entirely. You also explicitly assert that restoreNames(users) returns undefined in both tests, which enforces the requirement that the function should not return a value. The CI failure is due to a Node version mismatch in the workflow (Node 12 vs. dependencies requiring ≥14) and is not caused by your test code, so it is not a blocker for this task. As a tiny polish, you might want to fix the backtick typo in the second test description (it\\s→it's`) to keep your test names clean and readable. Overall, your tests are clear, focused, and aligned with the task requirements—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.