Repository navigation
London | 26-ITP-Sep | Alan Mak | Sprint 2 | Implement and rewrite tests - #1695
AlanGit-debug2604 wants to merge 18 commits into
Conversation
…ction's purpose and expected behavior
…tion's purpose and expected behavior
…d edge cases: not a number, not a finite number, 0 as denominator
… type of card ; define Rank and Suit composing value of card, define validRanks and valid Suits;
…oper fraction, 0 numerator, 0 denominator, non-numeric numerator , non-numeric denominator, not a number (NaN), infinity denominator
…d rank and suit inputs
…rank and suit inputs
…g for invalid inputs
…r edge cases and error handling
AlanGit-debug2604
added
📅 Sprint 2
2553f70 to
01e3987
Compare
webmonch
added
Review in progress
| @@ -18,4 +18,33 @@ | |||
|
|
|||
| export function getAngleType(angle) { | |||
There was a problem hiding this comment.
What would this function return for 360 and 0 degree angles?
There was a problem hiding this comment.
The function returns Reflex angle for 360, and Acute angel for 0 degree respectively.
|
|
||
| // Special case: numerator is zero | ||
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(1, 0)).toEqual(false); |
There was a problem hiding this comment.
Is there a particular reason why you updated the provided test here?
There was a problem hiding this comment.
I chose .toThrow(Error) rather than .toEqual(false) in this particular context where the denominator is zero because my understanding to fraction in mathematics, a zero denominator leads to infinitive outcome also known as an Error, therefore I chose .toThrow(Error) to preserve this mathematical implication.
| assert.equal(getCardValue("9♠"), 9); | ||
| }); | ||
|
|
||
| test("Arbitrary non-card string", () => { |
There was a problem hiding this comment.
I removed this test because "Invalid Rank" and "Invalid Suit" tests serve the purpose to tell user invalid character input in each and respective scenarios, in this case this test ("both characters invalid") then seems to be redundant.
My initial intention was to write test case which will tell user AT ONCE both characters are invalid for input like "1x"("non-card string" or "not-a-valid-card-string"). And originally, I placed this test below "Invalid Suit" at the bottom.
I realized, (in the case this test place below "Invalid Rank") the code will throw error from Invalid Rank and stop even prior to run "both characters invalid" test.
Then, even I tried to place "both characters invalid" BEFORE "Invalid" tests (as the sequence now you notice), this test does not return result as expected or stuck the whole piece of code.
Therefore, I removed this test as a trade-off and rather break the error into two separate tests for user to explore invalid input in each and respective scenarios.
|
Good job! Very thorough test coverage and the planning comments show a clear thought process. Just a couple of boundary cases in getAngleType to have another look at, plus a few tests that were changed from the original task. Apart from that, everything looks solid! |
webmonch
added
Reviewed
AlanGit-debug2604
added
the
Needs Review
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1059
Changelist
Write implementation, node tests, and Jest tests for 1-get-angle; 2-is-proper-fraction; and 3-get-card-values.
Tested and passed tests.
Questions
Housekeeping question: followed by npm installation in Prep, should package.json stay with coursework folder and commit. The reason of this question: in package.json configuration `"devdependencies" : "jest : "^30.5.2" has set up. Is the placement of this file essential to run jest test for implementation files?