Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the tests for the ifElse function! You've correctly verified that the condition is called and that the right callback is executed based on its return value.
However, this requires a couple of changes to make the tests more complete. The requirements state that the first and second callbacks must be run with no arguments. Your tests currently check if they are called, but not that they are called with the correct (empty) argument list.
You can add this check by using expect(first).toHaveBeenCalledWith(); in your test cases. For even more robust tests, you could also assert that the other callback was not called. Please add these assertions to your tests in src/ifElse.test.js.
✨ 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
|
|
||
| ifElse(condition, first, jest.fn()); | ||
|
|
||
| expect(first).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
This correctly checks that the first callback is called. However, the requirements also state that it should be called with no arguments. How could you add an assertion to verify that? For a more robust test, you could also check that the second callback was not executed.
|
|
||
| ifElse(condition, jest.fn(), second); | ||
|
|
||
| expect(second).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Good, this checks that the second callback is called when the condition is false. Similar to the test above, can you also add an assertion to confirm it was called with no arguments as per the requirements? It's also good practice to ensure the first callback was not called in this scenario.
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 previous feedback and met all the core requirements. The tests are much stronger now.
As a final suggestion to make your test suite even more robust, consider adding a test for a truthy value that isn't strictly true (like 1 or a non-empty string). This would fully confirm that the second callback is called for any value not strictly equal to true.
You can safely ignore the failing build (12.x) check; it's due to a project configuration issue and not a problem with your code. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| expect(second).not.toHaveBeenCalled() | ||
| }); | ||
|
|
||
| it('should call second if condition returns false', () => { |
There was a problem hiding this comment.
This test correctly handles the false case. To make your tests more comprehensive for the 'otherwise' scenario, consider adding a test case where the condition returns a truthy value that isn't strictly true (like the number 1 or a non-empty string). This will confirm that the second callback is called for any value that is not strictly equal to true, as the requirements imply.
2 participants
No description provided.