added unit tests for isElse function#247
added unit tests for isElse function#247Anton-Kuchmasov wants to merge 4 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a great start, and your tests correctly cover the basic conditional logic. However, I'm requesting a few changes to ensure the tests fully align with the project requirements.
The main issue is that the tests don't verify that the callbacks are called with no arguments, which is a specific requirement. You can easily fix this by updating your assertions. For example, instead of using expect(first).toHaveBeenCalled(), you can use expect(first).toHaveBeenCalledWith() to be more precise.
Please apply this check for the condition, first, and second callbacks in your tests. This will make your test suite more robust and complete.
✨ 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
| it('should invoke first cb if truthy contidion', () => { | ||
| ifElse(truthyCondition, first, second); | ||
|
|
||
| expect(first).toHaveBeenCalled(); |
There was a problem hiding this comment.
This assertion is correct. To make it more precise according to the requirements, you could also verify that the callback is called with no arguments. You can do this by changing the assertion to expect(first).toHaveBeenCalledWith();.
| it('should invoke second cb if falsy condition', () => { | ||
| ifElse(falsyCondition, first, second); | ||
|
|
||
| expect(second).toHaveBeenCalled(); |
There was a problem hiding this comment.
Similar to the previous test, you can make this check more specific by verifying that the second callback is called with no arguments, as required. Using expect(second).toHaveBeenCalledWith(); would accomplish this.
src/ifElse.test.js
Outdated
| }); | ||
|
|
||
| it('should invoke one of functions in any case', () => { | ||
| falsyCondition.mockReturnValue(!!(Math.random() > 0.5)); |
There was a problem hiding this comment.
This mock can now return true, which makes the variable name falsyCondition a bit misleading in this test case. It's often clearer to define a new mock for a different behavior. Also, the !! is redundant here, as the comparison Math.random() > 0.5 already results in a boolean.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on these tests! You're very close to a complete solution. You've correctly added checks to ensure the first and second callbacks are called without arguments.
However, this requires one final change. The tests are still missing a verification that the condition callback is also called with no arguments, which is a core requirement of the task. You can add this assertion to your existing tests for both the truthy and falsy paths using toHaveBeenCalledWith().
Once you add that check, this will be ready for approval!
✨ 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
| }).toThrow(); | ||
| }); | ||
|
|
||
| it('should invoke first or second functions without args', () => { |
There was a problem hiding this comment.
This is a great step forward! You're now correctly checking that first and second are called without arguments.
To fully meet the requirements, you also need to verify that the condition callback is called with no arguments.
A good way to organize this would be to merge these checks into the tests for the truthy and falsy paths. For example, the 'should invoke first cb' test could verify that both truthyCondition and first were called with .toHaveBeenCalledWith(), which would make this separate test case unnecessary.
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. You've successfully addressed the feedback from the previous review by adding the test to verify that the condition callback is called with no arguments. The test suite is now comprehensive and fully covers all requirements. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.