London | 26-ITP-May | Zadri Abdule | Sprint 3 | Implement & rewrite tests#1329
London | 26-ITP-May | Zadri Abdule | Sprint 3 | Implement & rewrite tests#1329Zadri415 wants to merge 4 commits into
Conversation
Zadri415
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Very solid function implementation and tests.
| const num = Number(rank); | ||
| if (Number.isInteger(num) && num >= 2 && num <= 10) return num; |
There was a problem hiding this comment.
In JavaScript, strings that represent valid numeric literals in the language can be safely converted to equivalent numbers. For examples, "0x02", "2.000", or "0002".
Does your function return the value you expected from each of the following function calls?
getCardValue("0x02♠");
getCardValue("2.000♠");
getCardValue("0002♠");
| test(`should return "Invalid angle" when invalid`, () => { | ||
| expect(getAngleType(-1)).toEqual("Invalid angle"); | ||
| expect(getAngleType(0)).toEqual("Invalid angle"); | ||
| expect(getAngleType("90")).toEqual("Invalid angle"); | ||
| expect(getAngleType(1000)).toEqual("Invalid angle"); | ||
| expect(getAngleType(9999.999)).toEqual("Invalid angle"); | ||
| }); |
There was a problem hiding this comment.
-
When a test fails with the message "... when invalid", it may be unclear what "invalid" actually refers to.
Can you revise the test description to make it more informative? -
Could also test the other boundary case, 360.
-
Regarding the test
expect(getAngleType("90")).toEqual("Invalid angle"):- It is not immediately clear why
"90"is considered an invalid angle - Why
"90"is considered invalid, but"91"is not? - It may imply the function is expected to check the type of the parameter
- It is not immediately clear why
| expect(isProperFraction(0, -100)).toEqual(true); | ||
| }); | ||
|
|
||
| test(`should return true when absolute value of numerator is less than absolute value of denominator`, () => { |
There was a problem hiding this comment.
Note: Using pseudocode and notations like abs(...) or | ... | is a common and widely accepted practice for describing conditions in a concise manner.
|
|
||
| // Suggestion: Group the remaining test data into these categories: | ||
| // Number Cards (2-10) | ||
| test(`Should return the numeric value for number cards`, () => { |
There was a problem hiding this comment.
Could also include a test to show that the function is expected to check the suit character.
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Added Jest tests for Sprint-3 implement and rewrite (exercises 1–3)