West Midlands | ITP-SEPT-25 | Georgina Rogers | Sprint 3 | Implement and rewrite tests #846
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
zilinskyte
changed the title
zilinskyte
changed the title
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
zilinskyte
changed the title
…or valid and invalid cards
zilinskyte
added
Needs Review
zilinskyte
changed the title
| if (numerator < denominator) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can you make this function to work also for negative parameters? For example, make isProperFraction(4, -5) evaluates to true.
| if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 10) { | ||
| // checks for number cards 2-10 | ||
| return Number(rank); | ||
| } |
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 or parsed into a valid integers.
Do you want to recognize these string values as valid ranks?
To find out what these strings are, you can ask AI
What kinds of string values would make
Number(rank)evaluate to2in JS?
or
What kind of string values could pass this check
!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 10?
| test("should return false for a negative fraction", () => { | ||
| expect(isProperFraction(-3, 4)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
- Test description is misleading.
| test("should return false for an improper fraction", () => { | ||
| expect(isProperFraction(5, 4)).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
We could test multiple values (including negative numbers) within this test to make the coverage more complete.
| test("should return 7 for 7 of Hearts", () => { | ||
| const sevenOfHearts = getCardValue("7♥"); | ||
| expect(sevenOfHearts).toEqual(7); | ||
| }); | ||
|
|
There was a problem hiding this comment.
When preparing tests, we should ensure the tests cover all possible cases. If we specify a test for individual card, we will need about 53 tests to cover all possible cases. Instead, we could consider classifying all possible values into different categories, and then within each category we test some samples.
For example, one possible category for getCardValue() is, "should return the value of number cards (2-10)", and we can prepare the test as
test("should return the value of number cards (2-10)", () => {
expect(getCardValue("2♣︎")).toEqual(2);
expect(getCardValue("5♠")).toEqual(5);
expect(getCardValue("10♥")).toEqual(10);
// Note: We could also use a loop to check all values from 2 to 10.
});
| test("should return 10 for King of Hearts", () => { | ||
| const kingOfHearts = getCardValue("K♥"); | ||
| expect(kingOfHearts).toEqual(10); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The function is not expected to test the suit character. Describing the suit character in the test description could mislead the person implementing the function into thinking the function needs also to check the suit character.
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Tests implemented and rewritten with Jest.
Questions
The last commit was accidental when trying to fix issue with previous commit which is failing the PR target test. Please advise.