Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work setting up the test file and covering a variety of scenarios for the splitInteger function. However, this requires changes because the current assertions are not specific enough to guarantee the function is working correctly. The tests check properties like array length but fail to verify that the sum of the resulting parts equals the original input value, which is a critical requirement.
For example, in your first test for splitInteger(8, 2), an incorrect result like [3, 3] would pass the checks, but its sum is 6, not 8. A more robust approach is to assert the exact expected output:
expect(result).toStrictEqual([4, 4]);This single check validates the length, content, order, and sum all at once. Please update your tests to use this stricter form of assertion.
✨ 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/splitInteger.test.js
Outdated
| const min = Math.min(...result); | ||
| const max = Math.max(...result); | ||
|
|
||
| expect(max - min).toBeLessThanOrEqual(1); | ||
| expect(result).toHaveLength(2); |
There was a problem hiding this comment.
These assertions check some of the requirements, but they aren't strict enough. For example, an incorrect result like [3, 3] would pass these checks, but the sum of its parts is 6, not 8. A more robust approach is to assert the exact expected output:
expect(result).toStrictEqual([4, 4]);This single check validates the length, content, order, and sum all at once.
src/splitInteger.test.js
Outdated
| const min = Math.min(...result); | ||
| const max = Math.max(...result); | ||
|
|
||
| expect(max - min).toBeLessThanOrEqual(1); | ||
| expect(result).toStrictEqual(sorted); | ||
| expect(result).toHaveLength(3); |
There was a problem hiding this comment.
Similar to the first test, these assertions don't verify that the sum of the parts equals the original value. An array like [1, 1, 2] would pass these checks, but its sum is 4, not 5. It's great that you're checking if the array is sorted, but you should also ensure the total value is correct. The most direct way is to check against the expected array:
expect(result).toStrictEqual([1, 2, 2]);
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've done a great job updating most of your tests to use toStrictEqual! This makes your test suite much more robust. However, I'm requesting one small change before this can be approved.
The final test case in src/splitInteger.test.js for splitInteger(6, 8) still uses multiple assertions instead of a single strict one. This approach doesn't fully verify that the array is sorted or that the values are distributed correctly.
Please update this last test to use a single toStrictEqual assertion, just like you did for the others. The expected output should be [0, 0, 1, 1, 1, 1, 1, 1].
Once that's fixed, this will be ready 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
src/splitInteger.test.js
Outdated
| expect(result).toHaveLength(8); | ||
| expect(countZeros(result)).toBe(2); | ||
| expect(result.reduce((a, b) => a + b, 0)).toBe(6); |
There was a problem hiding this comment.
It's great that you've updated the other tests to use toStrictEqual. This last test case should also be updated to use a single, strict assertion. This ensures you're checking all the requirements at once: the exact content, the length, the sum, and the ascending order of the elements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I'm approving this submission to help you keep moving forward, but please review the feedback below for your next task. You've done a great job using toStrictEqual to make your tests more robust, which was a key piece of feedback from last time!
There are two main areas to focus on for improvement:
- Missing Test Cases: Your test suite is missing a few required examples from the task description, specifically for
splitInteger(6, 2),splitInteger(17, 4), andsplitInteger(32, 6). Be sure to include all required tests. - Redundant Assertions: In several tests, you have extra checks that are already covered by
toStrictEqual. For example, when you asserttoStrictEqual([4, 4]), you don't need separate checks for the array's sum or the difference between its values, because the main assertion already guarantees this. Simplifying your tests to a single, powerful assertion makes them cleaner and more focused.
Excellent progress! Keep these points in mind, and you'll be writing even better tests in no time.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const min = Math.min(...result); | ||
| const max = Math.max(...result); | ||
|
|
||
| expect(max - min).toBeLessThanOrEqual(1); |
There was a problem hiding this comment.
These calculations and the assertion on line 12 are redundant. The toStrictEqual([4, 4]) on line 13 is a comprehensive check that already guarantees the difference between the max and min values is 0, which satisfies the requirement.
| when splitting into 1 part`, () => { | ||
| const result = splitInteger(6, 1); | ||
|
|
||
| expect(result[0]).toBe(6); |
There was a problem hiding this comment.
This assertion is redundant because toStrictEqual([6]) on the next line already verifies that the first (and only) element is 6.
| const sorted = [...result].sort((a, b) => a - b); | ||
|
|
||
| const min = Math.min(...result); | ||
| const max = Math.max(...result); | ||
|
|
||
| expect(max - min).toBeLessThanOrEqual(1); | ||
| expect(result).toStrictEqual(sorted); |
There was a problem hiding this comment.
These checks are helpful for understanding the requirements, but they are redundant. The toStrictEqual([1, 2, 2]) assertion on line 33 already confirms that the array is sorted and that the difference between the max and min values is 1.
| function countZeros(arr) { | ||
| let count = 0; | ||
|
|
||
| for (const a of arr) { | ||
| if (a === 0) { | ||
| count++; | ||
| } | ||
| } | ||
|
|
||
| return count; | ||
| } | ||
|
|
||
| expect(result).toStrictEqual([0, 0, 1, 1, 1, 1, 1, 1]); | ||
| expect(countZeros(result)).toBe(2); | ||
| expect(result.reduce((a, b) => a + b, 0)).toBe(6); |
There was a problem hiding this comment.
The toStrictEqual assertion on line 51 is a great addition! It's a comprehensive check that makes the assertions on lines 52 and 53 unnecessary. A single, powerful assertion is all you need here, which fully addresses the feedback from the previous review. The countZeros function also becomes unnecessary and can be removed.
2 participants
No description provided.