Repository navigation
Manchester | 26-ITP-Sep | Salah Alsabhi | Sprint 2 | implement and rewrite tests - #1667
salahalsabhi wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
salahalsabhi
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
salahalsabhi
changed the title
salahalsabhi
added
the
Needs Review
salahalsabhi
changed the title
hackertainment
added
Review in progress
hackertainment
left a comment
hackertainment
left a comment
There was a problem hiding this comment.
The implementation looks good. Just need to design more unique/special/boundary test cases. Keep it up.
There was a problem hiding this comment.
While this is correct, I find it may be a bit hard to read. Sometimes using else if may look cleaner.
| test("corrctly get angle type", function(){ | ||
| assert.equal(getAngleType(400), "Invalid angle"); | ||
| }); |
There was a problem hiding this comment.
There should be two boundary cases for invalid angle. Can you think of them? Please add them back.
| test("Basic proper fraction", () => { | ||
| assert.equal(isProperFraction(-1, 2), true); | ||
| }); |
There was a problem hiding this comment.
Good coverage for positive numbers. However, there should be more test cases for negative numbers. Please add them. Thank you.
There was a problem hiding this comment.
There should be more test cases for negative numbers. Please add them. Thank you.
| }); | ||
|
|
||
|
|
||
| // TODO: What other invalid card cases can you think of? |
There was a problem hiding this comment.
What other invalid card cases can you think of? Please add them. Thank you.
| test(`Should return numeric value for number cards`, () => { | ||
| expect(getCardValue("2♥")).toEqual(2); | ||
| expect(getCardValue("9♠")).toEqual(9); | ||
| expect(getCardValue("10♦")).toEqual(10); | ||
| }); | ||
|
|
||
| test(`Should return 10 for face cards`, () => { | ||
| expect(getCardValue("J♣")).toEqual(10); | ||
| expect(getCardValue("Q♠")).toEqual(10); | ||
| expect(getCardValue("K♥")).toEqual(10); | ||
| }); |
There was a problem hiding this comment.
There should be one more special case for valid card. Please add it. Thank you.
| test(`Should throw for invalid cards`, () => { | ||
| expect(() => getCardValue("invalid")).toThrow( | ||
| /Expected a number followed by a suit, but got "invalid"/ | ||
| ); | ||
| expect(() => getCardValue("A")).toThrow( | ||
| /Expected a number followed by a suit/ | ||
| ); |
There was a problem hiding this comment.
There should be more invalid cards to test. Please add them. Thank you.
hackertainment
added
Reviewed
|
Thank you for the review, I did most of the notes you mentioned hope all good. |
salahalsabhi
added
the
Needs Review
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1059
Changelist
Implemented three functions and covered them with tests in both node:test and Jest.
Questions
No questions