Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've done a great job setting up the tests for the ifElse function and correctly using mocks to check the main logic. However, a couple of requirements from the task description are not fully tested, so some changes are needed.
Please add assertions for the following points:
- No arguments for
condition: The tests should verify that theconditioncallback is called with no arguments. You can achieve this by using.toHaveBeenCalledWith(). undefinedreturn value: The tests need to check that theifElsefunction itself returnsundefined. You can do this by capturing its return value and asserting that it.toBeUndefined().
Addressing these will ensure your solution fully meets all 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
| const condition = jest.fn(() => true); | ||
| const first = jest.fn(); | ||
| const second = jest.fn(); | ||
| ifElse(condition, first, second); |
There was a problem hiding this comment.
The task description specifies that the ifElse function should not return any result. You could capture the return value from this function call and add an assertion to verify that it is undefined.
| const second = jest.fn(); | ||
| ifElse(condition, first, second); | ||
|
|
||
| expect(condition).toHaveBeenCalled(); |
There was a problem hiding this comment.
This is a good check. To fully meet the requirement that condition runs 'with no arguments', you can make this assertion more specific by using .toHaveBeenCalledWith() to ensure no arguments were passed.
|
|
||
| ifElse(condition, first, second); | ||
|
|
||
| expect(condition).toHaveBeenCalled(); |
There was a problem hiding this comment.
Similar to the other test case, you can strengthen this assertion by checking that condition was called with no arguments using .toHaveBeenCalledWith().
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this revision! Your tests successfully cover every requirement of the task, and you've addressed all the previous feedback. I am approving your solution.
Please disregard the failed build check. As the analysis shows, this is due to a project configuration issue and is not a problem with your code.
As a minor tip for future tests, you can often make them more concise by removing redundant assertions. For instance, toHaveBeenCalledWith() already confirms that a mock was called and with the correct arguments, making separate checks like toHaveBeenCalled() unnecessary. This is just a best practice for readability, and your current tests are functionally perfect. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| expect(first.mock.calls).toEqual([[]]); | ||
| expect(first).toHaveBeenCalledWith(); |
There was a problem hiding this comment.
It's great that you're exploring different ways to assert mock calls. For conciseness, you can often rely on a single, powerful matcher. In this case, toHaveBeenCalledWith() on line 18 already confirms that first was called and with the correct arguments, making the checks on line 14 (toHaveBeenCalled) and line 17 (mock.calls) redundant. Simplifying to just one assertion for this check is a common best practice for readable tests.
| expect(first.mock.calls).toEqual([]); | ||
| expect(second).toHaveBeenCalledWith(); | ||
|
|
||
| expect(second.mock.calls).toEqual([[]]); |
There was a problem hiding this comment.
The same principle of conciseness applies here. The assertion toHaveBeenCalledWith() on line 36 covers the checks on lines 33 and 38. Similarly, not.toHaveBeenCalled() on line 32 is a more direct and readable way to check that first was not called than inspecting .mock.calls on line 35. Sticking to the most direct assertion for each check makes tests easier to understand and maintain.
No description provided.