Repository navigation
Conversation
habohlin
added
the
Needs Review
Liam310
added
Review in progress
| test(`should return true when 1 and 2 is passed`, () => { | ||
| expect(isProperFraction(1, 2)).toEqual(true); | ||
| }); | ||
|
|
||
| test(`should return false when 4 and 2 is passed`, () => { | ||
| expect(isProperFraction(4, 2)).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
In general, when writing test description we want to try to avoid just repeating what the exact inputs, but instead actually describing the behaviours we are testing.
You did this really well in the implement-and-rewrite-tests/implement-tests-with-node-test/2-is-proper-fraction.test.js file - how could you refactor your other test descriptions (not just limited to the ones I've highlighted here) to be more descriptive and not just references to the function inputs?
| if (fraction >= 0 && fraction < 1) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Any time we have a setup like
if (condition) {
return true;
}
return false;And our condition it itself a boolean value, there is a shorter and neater way to write this portion of code. Can you think of what that would be?
Liam310
added
Reviewed
| /Expected a number followed by a suit, but got "s♦"/, | ||
| "Expected clear error" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I don't think your examples here are "arbitrary" - they're quite specific! You have a lot of different error tests, and it's great that you're being comprehensive, but I think it's worth considering:
- What are the behaviours you're testing?
- Are you repeating behaviour tests?
On the 2nd point, when writing a new test it's worth asking "is this testing the same bit of function logic as another test?" and if the answer is yes, that test probably isn't necessary.
In summary: your testing is very comprehensive, but I think the descriptions could be better and it is occasionally repetitive. If you think about fixing the descriptions, e.g. the first test in this section I've highlighted could be more accurately described as "errors for rank outside acceptable range" (19 is not within 2-9). you should think about how you can better describe and organise these tests 🙂
| const suitString = "♠♥♦♣"; | ||
| const rankString = "JQK"; | ||
|
|
||
| if (card) { | ||
| const suit = card[card.length - 1]; | ||
| if (suitString.includes(suit)) { | ||
| const rank = card.slice(0, -1); | ||
| if (rank === "10") { | ||
| return 10; | ||
| } else if (rank.length === 1) { | ||
| if (rank === "A") { | ||
| return 11; | ||
| } | ||
|
|
||
| if (rankString.includes(rank)) { | ||
| return 10; | ||
| } | ||
|
|
||
| const rankNum = Number(rank); | ||
|
|
||
| if (rankNum >= 2) { | ||
| return rankNum; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw new Error(`/Expected a number followed by a suit, but got "${card}"/`); |
There was a problem hiding this comment.
I like the structure of this - having the error being thrown at the bottom of the function is quite clean, as it only requires a single exception throwing statement.
Can you think of any drawbacks to this approach? Or how you might structure the function differently in terms of input validation?
(note: this is not a change request, just a question to prompt some thinking!)
| test(`Should throw error when given '19♠'`, () => { | ||
| expect(() => { | ||
| getCardValue("19♠"); | ||
| }).toThrow(/Expected a number followed by a suit, but got "19♠"/); | ||
| }); | ||
|
|
||
| test(`Should throw error when given 'P♦'`, () => { | ||
| expect(() => { | ||
| getCardValue("P♦"); | ||
| }).toThrow(/Expected a number followed by a suit, but got "P♦"/); | ||
| }); | ||
|
|
||
| test(`Should throw error when given 's♦'`, () => { | ||
| expect(() => { | ||
| getCardValue("s♦"); | ||
| }).toThrow(/Expected a number followed by a suit, but got "s♦"/); | ||
| }); |
There was a problem hiding this comment.
Remember - when you change above tests according to my earlier comments, make sure you change the Jest ones too!
|
Hi @habohlin ! Great work on this PR, your testing is very thorough. There is just some work to do in terms of thinking carefully about which tests to write and how to describe them. Additionally, when making commits, you don't need to be so granular. A commit should represent a completed unit of work, such as: "rewrite getAngleType tests in Jest", rather than having a single commit for each test. There's a bit of an art in getting this right, feel free to ask if you're not sure still 🙂 |
Liam310
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1059
Changelist
Task Implement and rewrite tests, three functions to implement and write node and jest tests for. Part of sprint 2 coursework.