Skip to content

solution#333

Open
Mitlosh wants to merge 4 commits into
mate-academy:masterfrom
Mitlosh:develop
Open

solution#333
Mitlosh wants to merge 4 commits into
mate-academy:masterfrom
Mitlosh:develop

Conversation

@Mitlosh

@Mitlosh Mitlosh commented Feb 23, 2026

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one explicit requirement of the task is not yet satisfied: you do not currently assert that restoreNames returns no value (undefined). Your existing tests in src/restoreNames.test.js do a solid job of covering the main behaviors (restoring firstName when missing or undefined, preserving existing firstName, handling multiple users, and the empty array case), so the core logic is well-verified. However, the task description explicitly states that the function should not return anything, and there is no test that checks expect(restoreNames(users)).toBeUndefined(); yet.

Critical issue to fix [CHECKLIST ITEM #6]:

  • Add a dedicated test, for example:
    test('should return undefined', () => {
      const users = [];
      expect(restoreNames(users)).toBeUndefined();
    });
    or add a similar expectation to one of your existing tests after invoking restoreNames(users).

Once you add this assertion, your test suite will fully reflect the specified contract for restoreNames. The rest of your tests look strong and well-structured, so you’re very close—this is a small but important final adjustment to align with the stated 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/restoreNames.test.js
test('should work correctly with an empty array', () => {
const users = [];

restoreNames(users);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an explicit assertion that the function does not return anything (checklist item #6). For example, after calling restoreNames(users); add expect(restoreNames(users)).toBeUndefined(); or create a dedicated test like test('should return undefined', () => { const users = []; expect(restoreNames(users)).toBeUndefined(); }); so the behavior is verified.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because your test suite still does not fully verify a key requirement: that existing lastName and fullName values are preserved when restoreNames runs. The tests currently focus on asserting that firstName is restored or not overwritten, but they often omit checks that lastName and fullName remain unchanged for users that already have them. To fix this, extend each relevant test in src/restoreNames.test.js to include expectations like expect(users[0].lastName).toBe('Holy') and expect(users[0].fullName).toBe('Jack Holy') (or the appropriate values for that test case). For the test that restores firstName to 'Adams', also assert expect(users[0].lastName).toBe('Adams') and expect(users[0].fullName).toBe('Mike Adams'), and similarly for the 'Johnny Doe' case. In the combined toEqual assertion for multiple users, make sure the expected objects include lastName wherever it existed in the input, or add explicit lastName checks alongside your firstName and fullName assertions. Once these preservation checks are in place, your tests will fully exercise the specified behavior, and the overall structure and use of Jest expectations you already have will be in great shape.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/restoreNames.test.js

restoreNames(users);

expect(users[0].firstName).toBe('Jack');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test correctly asserts the new firstName, but it doesn't verify that existing lastName and fullName were preserved. Add expectations such as expect(users[0].lastName).toBe('Holy') and expect(users[0].fullName).toBe('Jack Holy') to fully satisfy the task requirements.

Comment thread src/restoreNames.test.js

restoreNames(users);

expect(users[0].firstName).toBe('Mike');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After restoring the missing firstName you assert the firstName value but not that lastName and fullName stayed the same. Add expect(users[0].lastName).toBe('Adams') and expect(users[0].fullName).toBe('Mike Adams') to verify preservation.

Comment thread src/restoreNames.test.js

restoreNames(users);

expect(users[0].firstName).toBe('John');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test ensures firstName is not changed, but it should also assert that lastName and fullName remain unchanged (e.g. expect(users[0].lastName).toBe('Doe') and expect(users[0].fullName).toBe('Johnny Doe')) so the preservation requirement is covered.

Comment thread src/restoreNames.test.js

restoreNames(users);

expect(users).toEqual([

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combined toEqual assertion for multiple users checks firstName and fullName for these inputs, but if any input includes lastName the expected result should include it as well. Ensure your expected array preserves lastName for users that had it in the input, or add explicit lastName checks where appropriate.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while learning, even though there are still a couple of details to address. Your restoreNames.test.js file is well-structured: you use Jest correctly, cover missing/undefined firstName, preserve lastName and fullName, handle multiple users, and include an empty-array case, which aligns nicely with most of the requirements.

Two main improvements are needed:

  • First, ensure that the implementation file src/restoreNames.js (or src/restoreNames/index.js, depending on your setup) actually exists and exports restoreNames, so that require('./restoreNames') in src/restoreNames.test.js works and the test suite can run. This directly affects whether your tests execute at all.
  • Second, to satisfy the requirement that the function “should not return anything” [CHECKLIST ITEM #2], add an explicit assertion in at least one test, for example at line where you first call the function:
    • const result = restoreNames(users);
    • expect(result).toBeUndefined();
      and then keep your existing expectations about the mutated users array.

Overall, you’ve done a strong job capturing the intended behavior with your tests; refining the return-value assertion and confirming the export will make your solution more robust and fully aligned with the specification.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/restoreNames.test.js

describe('restoreNames', () => {
// const { restoreNames } = require('./restoreNames');
const { restoreNames } = require('./restoreNames');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests require('./restoreNames') — make sure the implementation file exists at that path and exports restoreNames. If the module is missing, the test suite will fail to run. This relates to the implementation being present so tests can verify behavior (checklist items #1 and #6).

Comment thread src/restoreNames.test.js
},
];

restoreNames(users);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an explicit assertion that the function returns undefined to satisfy the requirement that it "should not return anything" (checklist item #2). For example: const result = restoreNames(users); expect(result).toBeUndefined(); then keep the existing checks on the mutated users array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants