Sheffield | ITP-Jan-26 | Hayriye Saricicek | Sprint 3 | Implement and rewrite tests#1230
Sheffield | ITP-Jan-26 | Hayriye Saricicek | Sprint 3 | Implement and rewrite tests#1230mshayriyesaricicek wants to merge 12 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
mshayriyesaricicek
changed the title
mshayriyesaricicek
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
You edited a file and deleted three files in folders that are not related to the "Implement and rewrite" exercise. Can you revert the changes to those files?
It seems the first commit that started affecting those files were introduced two days ago.
To revert the change,
- Backup your files
- Locate that commit and record its SHA
- Find out from AI
What's the git command to revert all the commits made, starting with the earliest commit with the SHA 1234567?
and follow the instructions.
Note: You will need to replace the SHA accordingly.
| // There could be cards with two numbers rather than a number and a suite | ||
| // These will not be picked up because the code only checks for if starts with 10 or if the first character | ||
| // is a number between 2 and 9, so cards like "22" would be valid because the first number | ||
| // is 2, but the second character is not checked for validity. It is also 2 characters | ||
| // so will not cause an error when the length is checked. | ||
|
|
||
| // Since the second character is not checked it could be 2D which is not a valid card but | ||
| // would be accepted because the first character is 2 and the second character is not checked for validity | ||
|
|
||
| // When the card is checked if it begins with 10 it does check if it has a valid suite | ||
| // as only the first 2 characters are checked so it could be 10DEVON or 10♥♥. |
There was a problem hiding this comment.
These are all good advice and you should pay attention to them. :)
| test(`should return "Invalid angle" when (angle < 0 || angle >= 360)`, () => { | ||
| expect(getAngleType(-1)).toEqual("Invalid angle"); | ||
| expect(getAngleType(360)).toEqual("Invalid angle"); | ||
| }); |
There was a problem hiding this comment.
It is great that you tested one of the boundary cases. Why not test also the other one?
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(1, 2)).toEqual(true); | ||
| }); | ||
|
|
||
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(2, 2)).toEqual(false); | ||
| }); | ||
|
|
||
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(2, 1)).toEqual(false); | ||
| }); | ||
|
|
||
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(0, 0)).toEqual(false); | ||
| }); | ||
|
|
||
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(0, 1)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
These test descriptions do not quite match the tests being carried out.
| test(`Should return 2 when given a 2 card`, () => { | ||
| expect(getCardValue("2♠")).toEqual(2); | ||
| }); | ||
|
|
||
| test(`Should return 10 when given a 10 card`, () => { | ||
| expect(getCardValue("10♥")).toEqual(10); | ||
| }); |
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.
});
Can you practice preparing tests in this fashion?
cjyuan
added
Reviewed
|
The changed files in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the changed files tab at the top of the page, we are only expecting changes in this directory: 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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
The changed files in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the changed files tab at the top of the page, we are only expecting changes in this directory: 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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
2 participants
Self checklist
Changelist
This pull request practices implementing assertions and the use of Jest.
The funciton are written initially with assertions then re-written using jest.