Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on setting up the tests for the restoreNames function. However, a couple of critical changes are needed before this can be approved.
First, your tests are not isolated because they share and modify the same users array. This makes them unreliable, as one test's outcome affects the next. To fix this, please ensure each test gets a fresh copy of the data, for example, by using a beforeEach block.
Second, a core requirement is to verify that the function doesn't return anything, but a test for this is missing. Please add a test to confirm that the function's return value is undefined.
✨ 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
| const users = [ | ||
| { | ||
| firstName: undefined, | ||
| lastName: "Holy", | ||
| fullName: "Jack Holy", | ||
| }, | ||
| { | ||
| lastName: "Adams", | ||
| fullName: "Mike Adams", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
This users array is defined in a shared scope and gets mutated by your first test. The second test then operates on this already-modified data, which means it isn't testing the original scenario. Tests must be independent. To fix this, you can either create a fresh copy of the users array inside each it block or use Jest's beforeEach hook to reset the data before each test.
src/restoreNames.test.js
Outdated
| }); | ||
|
|
||
| // write tests here | ||
| it("should have firstName", () => { |
There was a problem hiding this comment.
This test and the one on line 27 have the same name. It's a best practice to give each test a unique and descriptive name that explains the specific scenario it covers.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your submission. While your tests correctly cover the function's main logic, there are two critical issues from the previous review that still need to be addressed. For this reason, I am requesting changes.
-
Tests are not isolated: Your tests currently share and modify the same
usersarray. This means the outcome of one test can affect the next, making them unreliable. To fix this, please ensure each test gets a fresh copy of the data. AbeforeEachblock is the standard way to reset data before each test case runs. -
Missing test for the return value: A key requirement is that the
restoreNamesfunction should not return a value. You still need to add a test to verify this. Please add a new test case that calls the function and asserts that its return value isundefined.
Once these two points are addressed, your solution will be ready. 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/restoreNames.test.js
Outdated
| const users = [ | ||
| { | ||
| firstName: undefined, | ||
| lastName: "Holy", | ||
| fullName: "Jack Holy", | ||
| }, | ||
| { | ||
| lastName: "Adams", | ||
| fullName: "Mike Adams", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
This users array is defined once and then mutated by your tests. This means the tests are not isolated from each other, and the outcome of one can affect the next. To ensure each test is reliable and independent, you should provide a fresh copy of the test data for each test case. A beforeEach block is a great way to handle this.
| restoreNames(users); | ||
| expect(users[1].firstName).toBe("Mike"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
One of the key requirements is that the function should not return a value. As mentioned in the previous review, you need to add a test case to verify that calling restoreNames results in undefined.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you move forward, but there are a couple of important points to review for future tasks. You did a great job adding the test to verify the function's return value!
The main issue remaining is test isolation. Your call to restoreNames(users) happens outside of any it block, which modifies the data before the tests run. To make your tests reliable, each one should be self-contained. Please move the function call and its corresponding assertions inside a single it block.
Additionally, one of your test descriptions is a bit misleading. The test checking for an undefined return is named 'should be a function'. A clearer name would be 'should not return a value'. Keep up the great work—you're very close to a perfect solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
src/restoreNames.test.js
Outdated
| }, | ||
| ]; | ||
|
|
||
| it('should be a function', () => { |
There was a problem hiding this comment.
This test's description is a bit misleading. It correctly checks that the function returns undefined, but the description says it 'should be a function'. Consider renaming it to something like 'should not return a value' to better reflect its purpose.
src/restoreNames.test.js
Outdated
| restoreNames(users); | ||
|
|
||
| users.forEach((item, index) => { | ||
| it('firstName should be equl first part of fullName', () => { | ||
| expect(item.firstName).toBe(item.fullName.split(' ')[0]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This block of code doesn't follow the principle of test isolation. The call to restoreNames(users) on line 28 happens outside of any it block, which means it modifies the shared users array before the tests generated by the forEach loop even run. The tests then assert against data that has already been changed, rather than testing the function's transformation itself.
A test should be self-contained. Please move the function call and the assertions into a single it block to properly test the function's behavior in an isolated manner.
For example:
it('should set the correct firstName for users', () => {
// Use a fresh copy of data for the test
const testUsers = [
{ fullName: 'Jack Holy' },
{ fullName: 'Mike Adams' }
];
restoreNames(testUsers);
expect(testUsers[0].firstName).toBe('Jack');
expect(testUsers[1].firstName).toBe('Mike');
});
No description provided.