Created some tests for the function#250
Created some tests for the function#250GeorgeSavostikov wants to merge 3 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
I am requesting a couple of changes to your tests. You've done a great job setting up the tests and covering the two main execution paths for the ifElse function. However, the tests can be more specific to fully meet the requirements.
First, please add descriptive names to your test cases. For example, instead of it('should ', ...) you could write it('should call the first callback when the condition is true'). This makes it much easier to understand the purpose of each test at a glance.
Second, the requirements state that the callbacks should be run with no arguments. Your tests currently use toHaveBeenCalled(), which only checks that the function was called. To be more precise, please use toHaveBeenCalledWith() to assert that the callbacks were indeed called with zero arguments. These changes will make your tests more robust and ensure they fully validate the function's behavior.
✨ 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/ifElse.test.js
Outdated
| const { ifElse } = require('./ifElse'); | ||
|
|
||
| it('should ', () => {}); | ||
| it('should ', () => { |
There was a problem hiding this comment.
It's a good practice to provide descriptive names for your tests. This helps others understand what the test is supposed to do at a glance. For example: it('should call the first callback when the condition is true', () => { ... });
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on addressing the previous feedback! Your test descriptions are much clearer. However, changes are needed because the tests don't yet verify a core requirement: that the condition callback is actually called. To fix this, please convert the condition in your tests into a mock function using jest.fn() and add an assertion to ensure it's called with no arguments. Also, for checking that a function was not called, it's more precise to use .not.toHaveBeenCalled() instead of .not.toHaveBeenCalledWith(). You're very close to a complete solution!
✨ 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
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! You've successfully addressed all the previous feedback, and your tests are much more thorough. I am approving this solution to help you continue making progress. As a suggestion for future improvement, consider making the condition a mock function (jest.fn()) in all your test cases. This would let you consistently verify that condition is called in both the true and false scenarios, making your test suite even stronger. You're very close to a perfect solution, keep up the great effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| }); | ||
|
|
||
| it('should call the first callback when the condition is true', () => { | ||
| condition = () => true; |
There was a problem hiding this comment.
To keep your tests consistent, consider using jest.fn() for the condition here, similar to your first test. This would let you verify that condition is called in the true path within this same test case.
| }); | ||
|
|
||
| it('should call the second callback when the condition is false', () => { | ||
| condition = () => false; |
There was a problem hiding this comment.
Similar to the test for the true condition, making this condition a mock function would allow you to assert it gets called in the false path as well, making your test suite more robust.
2 participants
No description provided.