Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on the tests for the splitInteger function. You've covered the main functionality and an important edge case well. However, I am requesting changes because your test suite is missing coverage for one of the examples from the task description. Please add a test for splitInteger(32, 6) to ensure all requirements are fully covered. Once that's added, your solution should be good to go!
✨ 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
| test('should add zeros if value < numberOfParts', () => { | ||
|
|
||
| expect(splitInteger(4, 5)).toEqual([0, 1, 1, 1, 1]); | ||
| }); |
There was a problem hiding this comment.
This is a great test for an edge case not listed in the requirements. However, your test suite is missing coverage for one of the examples from the description: splitInteger(32, 6). Please add a test for this case to ensure all requirements are covered.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback by adding the test case for splitInteger(32, 6)! Your test suite is almost complete. However, changes are needed because one of the required test cases from the task description is still missing. Please add a test to verify that splitInteger(17, 4) correctly returns [4, 4, 4, 5]. Once you add this final test case, your solution will fully meet all the requirements. Keep up the great work!
✨ 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
| test('should add zeros if value < numberOfParts', () => { | ||
|
|
||
| expect(splitInteger(4, 5)).toEqual([0, 1, 1, 1, 1]); | ||
| }); |
There was a problem hiding this comment.
You're almost there! You've covered most of the required examples. Please also add a test case for splitInteger(17, 4) as specified in the task description to complete the task.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! You've successfully added the final required test case for splitInteger(17, 4), and your solution now meets all the requirements. I am approving this submission to help you keep moving forward.
For future reference, I noticed a small area for improvement in src/splitInteger.test.js. Two tests share the same description: 'should sort parts ascending if they are not equal'. It's a great practice to make test descriptions unique, as it helps you quickly identify which specific scenario is failing. For example, you could change one to should split 17 into 4 parts.
Overall, this is a fantastic submission. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| expect(splitInteger(32, 6)).toEqual([5, 5, 5, 5, 6, 6]); | ||
| }); | ||
|
|
||
| test('should sort parts ascending if they are not equal', () => { |
There was a problem hiding this comment.
This test has the same description as the one on line 15. It's a good practice to give each test a unique and descriptive name. This helps identify exactly which test fails if there's an issue. For example, you could change this to something like should split 17 into 4 parts.
2 participants
No description provided.