London | 26-ITP-Jan | Angela McLeary | Sprint 1 | Sprint 1#1129
London | 26-ITP-Jan | Angela McLeary | Sprint 1 | Sprint 1#1129AngelaMcLeary wants to merge 18 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| test("given an array with decimal float numbers, it should return the total sum", () => { | ||
| expect(sum([1.5, 2.5, 3.5])).toEqual(7.5); | ||
| }); |
There was a problem hiding this comment.
Decimal numbers in most programming languages (including JS) are internally represented in "floating point number" format. Floating point arithmetic is not exact. For example, the result of 46.5678 - 46 === 0.5678 is false because 46.5678 - 46 only yield a value that is very close to 0.5678. Even changing the order in which the program add/subtract numbers can yield different values.
So the following could happen
expect( 1.2 + 0.6 + 0.005 ).toEqual( 1.805 ); // This fail
expect( 1.2 + 0.6 + 0.005 ).toEqual( 1.8049999999999997 ); // This pass
expect( 0.005 + 0.6 + 1.2 ).toEqual( 1.8049999999999997 ); // This fail
console.log(1.2 + 0.6 + 0.005 == 1.805); // false
console.log(1.2 + 0.6 + 0.005 == 0.005 + 0.6 + 1.2); // falseCan you find a more appropriate way to test a value (that involves decimal number calculations) for equality?
Suggestion: Look up
- Checking equality in floating point arithmetic in JavaScript
- Checking equality in floating point arithmetic with Jest
There was a problem hiding this comment.
Hi @cjyuan, Thank you for your feedback. I understand now why decimal values need special handling. toBeCloseTo() is a better choice because it checks that the result is close enough within a small margin, which avoids issues caused by floating‑point rounding.
There was a problem hiding this comment.
Note: The default value of the second parameter of toBeCloseTo() is only 2
See: https://jestjs.io/docs/expect#tobeclosetonumber-numdigits
For the default value 2, the test criterion is Math.abs(expected - received) < 0.005, which may not be precise enough in some applications.
…NaN correctly and shadow array
cjyuan
left a comment
There was a problem hiding this comment.
Other changes are good. Just one issue left.
| const result = dedupe(originalArray); | ||
| expect(result).toEqual(originalArray); | ||
| expect(result).not.toBe(originalArray); |
There was a problem hiding this comment.
This test can verify the function can return a different array.
However, there is a chance that, even though result has incorrect elements (for example, [1, 1, 1]),
the test on line 29 could still pass. Can you figure out why, and then improve the test accordingly?
Learners, PR Template
Self checklist
Changelist
This PR is about fixing median.js, Implementing dedupe.js, max.js, sum.js and refactoring include.js.