[Split Integer JS-Testing task]#1037
[Split Integer JS-Testing task]#1037Bohdan-Diatliuk wants to merge 6 commits intomate-academy:mainfrom
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the splitInteger function—the implementation is clean and correct. However, changes are needed in your test file to meet all requirements. Your tests are missing the specific examples provided in the task description. Please update src/splitInteger.test.js to include assertions for these cases: splitInteger(8, 1) should return [8], splitInteger(6, 2) should return [3, 3], splitInteger(17, 4) should return [4, 4, 4, 5], and splitInteger(32, 6) should return [5, 5, 5, 5, 6, 6]. Adding these will make your test suite complete and fully align with the project's 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
| test(`should split a number into equal parts | ||
| if a value is divisible by a numberOfParts`, () => { | ||
|
|
||
| expect(splitInteger(6, 3)).toEqual([2, 2, 2]); | ||
| expect(splitInteger(10, 5)).toEqual([2, 2, 2, 2, 2]); | ||
| expect(splitInteger(8, 4)).toEqual([2, 2, 2, 2]); | ||
| }); | ||
|
|
||
| test(`should return a part equals to a value | ||
| when splitting into 1 part`, () => { | ||
|
|
||
| expect(splitInteger(10, 1)).toEqual([10]); | ||
| }); | ||
|
|
||
| test('should sort parts ascending if they are not equal', () => { | ||
|
|
||
| test("should sort parts ascending if they are not equal", () => { | ||
| expect(splitInteger(10, 3)).toEqual([3, 3, 4]); | ||
| expect(splitInteger(11, 4)).toEqual([2, 3, 3, 3]); | ||
| }); | ||
|
|
||
| test('should add zeros if value < numberOfParts', () => { | ||
|
|
||
| test("should add zeros if value < numberOfParts", () => { | ||
| expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]); | ||
| expect(splitInteger(2, 4)).toEqual([0, 0, 1, 1]); | ||
| expect(splitInteger(1, 3)).toEqual([0, 0, 1]); | ||
| }); |
There was a problem hiding this comment.
You've done a good job creating different test cases. However, the task description lists specific examples that should be included in the tests to ensure the function works as required.
Please add tests for these examples:
splitInteger(8, 1)should return[8]splitInteger(6, 2)should return[3, 3]splitInteger(17, 4)should return[4, 4, 4, 5]splitInteger(32, 6)should return[5, 5, 5, 5, 6, 6]
Including these will make your test suite more robust and fully align with the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great progress on this! The core logic in splitInteger.js is solid. However, I'm requesting changes because the test suite isn't quite complete yet. You are missing a required test case from the task description for splitInteger(32, 6). Please add a test to src/splitInteger.test.js that verifies this case. Additionally, the test for splitInteger(8, 1) is duplicated; please remove one of them to keep the tests clean. Once those two things are fixed, 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
| test("splitInteger(32, 6) should return [5, 5, 5, 5, 6, 6]", () => { | ||
| expect(splitInteger(32, 6)).toEqual([5, 5, 5, 5, 6, 6]); | ||
| }); |
There was a problem hiding this comment.
This test case is a duplicate of the one on lines 28-30. It's good practice to have only one test for each specific scenario to keep the test suite clean.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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'm approving your solution. You've done a fantastic job addressing all the feedback from the previous review. The test suite is now robust and covers all the examples from the task description, and the duplicated test has been correctly removed. The function itself is implemented perfectly and meets all requirements. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.