qa array reduce method solution#98
qa array reduce method solution#98AnnNikiforova 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
Great start on the test suite for your custom reduce method! You've correctly tested some important cases. However, this requires a few changes to make the tests more comprehensive and accurate.
Here are the key points to address:
- Incorrect Error Handling Test: Your test for reducing an empty array without an initial value incorrectly expects
undefined. According to the specification, this scenario should throw aTypeError. Please update this test to use Jest's.toThrow()matcher to assert that an error is thrown. - Missing Return Value Assertions: Several tests are missing a crucial assertion for the final accumulated value returned by
reduce2(). The main purpose ofreduceis to produce this value, so it's essential to verify it's correct in your tests. - Ineffective Immutability Test: The assertion
expect(array).toEqual(array)will always pass. To properly test that the original array is not mutated, you should create a copy of it before callingreduce2and then compare the original array to that copy afterward.
Addressing these points will ensure your test suite correctly validates the behavior of the reduce method.
✨ 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/reduce.test.js
Outdated
|
|
||
| array.reduce2(callback, 0); | ||
|
|
||
| expect(array).toEqual(array); |
There was a problem hiding this comment.
This assertion expect(array).toEqual(array) will always be true and doesn't effectively test for immutability. To properly test that the array wasn't mutated, you should compare it against a copy that was made before calling reduce2.
| it('should run callback with correct arguments', () => { | ||
| const array = [1, 2, 3, 4]; | ||
|
|
||
| array.reduce2(callback, 0); | ||
|
|
||
| expect(callback).toHaveBeenNthCalledWith(1, 0, 1, 0, array); | ||
| expect(callback).toHaveBeenNthCalledWith(2, 1, 2, 1, array); | ||
| expect(callback).toHaveBeenNthCalledWith(3, 3, 3, 2, array); | ||
| expect(callback).toHaveBeenNthCalledWith(4, 6, 4, 3, array); | ||
| }); |
There was a problem hiding this comment.
While it's good to check that the callback is called with the correct arguments, you're missing a check for the most important part of reduce: the final accumulated value that it returns. Consider storing the result of array.reduce2() and adding an assertion for it.
src/reduce.test.js
Outdated
| it('should return undefined if array is empty and no initial value', () => { | ||
| const array = []; | ||
| const result = array.reduce2(callback); | ||
|
|
||
| expect(result).toBe(undefined); ; |
There was a problem hiding this comment.
According to the MDN documentation, calling reduce on an empty array without an initial value should throw a TypeError. This test incorrectly expects undefined. You should use Jest's toThrow() or toThrowError() matchers to assert that an error is thrown.
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, especially on the immutability test! However, a couple of changes are needed to meet the core requirements. First, your test for reducing an empty array without an initial value needs to assert that a TypeError is thrown, as required by the specification. You can use Jest's .toThrow() for this. Second, a few tests are still missing assertions to verify that reduce2 returns the correct final accumulated value. Please ensure every test that produces a result checks that the result is correct. You're very close to finishing this!
✨ 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/reduce.test.js
Outdated
| it('should run callback array`s length times if initialValue is', () => { | ||
| const array = [1, 2, 3, 4]; | ||
|
|
||
| array.reduce2(callback, 0); |
There was a problem hiding this comment.
This test correctly checks the number of times the callback is called, but it's also important to verify that reduce2 returns the correct final accumulated value. Please add an assertion for the return value.
src/reduce.test.js
Outdated
| it('should run callback array`s length - 1 times if no initialValue', () => { | ||
| const array = [1, 2, 3, 4]; | ||
|
|
||
| array.reduce2(callback); |
There was a problem hiding this comment.
Similar to the test above, this one is missing an assertion for the final return value of reduce2. Verifying the accumulated result is a key part of testing reduce.
src/reduce.test.js
Outdated
| it('should not run callback if array is empty', () => { | ||
| const array = []; | ||
|
|
||
| array.reduce2(callback); | ||
|
|
||
| expect(callback).toHaveBeenCalledTimes(0); ; | ||
| }); |
There was a problem hiding this comment.
According to the Array.prototype.reduce specification, calling reduce on an empty array without providing an initial value must throw a TypeError. This test should be updated to assert that an error is thrown, for example by using Jest's .toThrow() matcher.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding assertions to verify the return values in your tests! That's a huge improvement and makes your test suite much more robust.
I am approving your solution as an educational override so you can keep making progress. However, there is one critical test case still missing from src/reduce.test.js. According to the specification, calling reduce on an empty array without providing an initial value must throw a TypeError. Please add a test to verify this behavior. You can use Jest's expect(() => ...).toThrow(TypeError) to check for the thrown error.
Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const result = array.reduce2(callback, initialValue); | ||
|
|
||
| expect(result).toBe(initialValue); ; | ||
| }); |
There was a problem hiding this comment.
According to the Array.prototype.reduce specification, calling reduce on an empty array without providing an initial value should result in a TypeError. Please add a test case to verify this behavior. You can use Jest's expect(() => ...).toThrow(TypeError) to assert that an error is thrown.
2 participants
No description provided.