-
-
Notifications
You must be signed in to change notification settings - Fork 395
Manchester | 26-ITP-May | Szidonia Bodo | Sprint 3 | Implement and review test #1520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f0ca3d7
5ad372d
5c0d472
b28e30e
241ef30
f0b6faa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,11 @@ | |
| // execute the code to ensure all tests pass. | ||
|
|
||
| function isProperFraction(numerator, denominator) { | ||
| // TODO: Implement this function | ||
| if (numerator <= denominator && numerator > 0) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // The line below allows us to load the isProperFraction function into tests in other files. | ||
|
|
@@ -31,3 +35,10 @@ function assertEquals(actualOutput, targetOutput) { | |
|
|
||
| // Example: 1/2 is a proper fraction | ||
| assertEquals(isProperFraction(1, 2), true); | ||
| assertEquals(isProperFraction(0, 4), false); | ||
| assertEquals(isProperFraction(5, 5), true); | ||
| assertEquals(isProperFraction(-8, 2), false); | ||
| assertEquals(isProperFraction(4, 0), false); | ||
| assertEquals(isProperFraction(0, 0), false); | ||
| assertEquals(isProperFraction(-2, -2), false) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you were to check the case of (Things get a little awkwardly mathsy with negative fractions, I recognise) |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,16 @@ | |
| // execute the code to ensure all tests pass. | ||
|
|
||
| function getCardValue(card) { | ||
| // TODO: Implement this function | ||
| let removeSuit = card.slice(0, -1); | ||
| if (removeSuit === "A") { | ||
| return 11; | ||
| } else if (removeSuit === "J" || removeSuit === "Q" ||removeSuit === "K") { | ||
| return 10; | ||
| } else if (removeSuit > 1 && removeSuit < 11) { | ||
| return Number(removeSuit); | ||
| } else { | ||
| throw new Error("Error") | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A great solution, very readable 👌 Just one point, you've declared |
||
| } | ||
|
|
||
| // The line below allows us to load the getCardValue function into tests in other files. | ||
|
|
@@ -40,15 +49,27 @@ function assertEquals(actualOutput, targetOutput) { | |
| // TODO: Write tests to cover all outcomes, including throwing errors for invalid cards. | ||
| // Examples: | ||
| assertEquals(getCardValue("9♠"), 9); | ||
| assertEquals(getCardValue("A♠"), 11); | ||
| assertEquals(getCardValue("J♦"), 10); | ||
| assertEquals(getCardValue("7♥"), 7); | ||
| assertEquals(getCardValue("A♥"), 11); | ||
| assertEquals(getCardValue("8♥"), 8); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When writing a test suite, we want to try and cover all possible "behaviours" of our function. Here I can see you've covered:
Looking at that list, what have you not covered? |
||
|
|
||
| // Handling invalid cards | ||
| try { | ||
| getCardValue("invalid"); | ||
|
|
||
| // This line will not be reached if an error is thrown as expected | ||
| console.error("Error was not thrown for invalid card 😢"); | ||
| } catch (e) { | ||
| console.log("Error thrown for invalid card 🎉"); | ||
| } | ||
|
|
||
| // What other invalid card cases can you think of? | ||
| try { | ||
| getCardValue("15♥"); | ||
| // This line will not be reached if an error is thrown as expected | ||
| console.error("Error was not thrown for invalid card 😢"); | ||
| } catch (e) { | ||
| console.log("Error thrown for invalid card 🎉"); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,33 @@ test(`should return "Acute angle" when (0 < angle < 90)`, () => { | |
| }); | ||
|
|
||
| // Case 2: Right angle | ||
| test(`should return "Right angle" when (angle = 90)`, () => { | ||
| // Test various acute angles, including boundary cases | ||
| expect(getAngleType(90)).toEqual("Right angle"); | ||
| }); | ||
| // Case 3: Obtuse angles | ||
| test(`should return "Obtuse angle" when (angle < 180, angle > 90)`, () => { | ||
| // Test various acute angles, including boundary cases | ||
| expect(getAngleType(91)).toEqual("Obtuse angle"); | ||
| expect(getAngleType(179)).toEqual("Obtuse angle"); | ||
| expect(getAngleType(99)).toEqual("Obtuse angle"); | ||
| }); | ||
| // Case 4: Straight angle | ||
| test(`should return "Straight angle" when (angle = 180)`, () => { | ||
| // Test various acute angles, including boundary cases | ||
| expect(getAngleType(180)).toEqual("Straight angle"); | ||
| }); | ||
| // Case 5: Reflex angles | ||
| test(`should return "Reflex angle" when (angle < 360, angle > 180)`, () => { | ||
| // Test various acute angles, including boundary cases | ||
| expect(getAngleType(189)).toEqual("Reflex angle"); | ||
| expect(getAngleType(359)).toEqual("Reflex angle"); | ||
| expect(getAngleType(199)).toEqual("Reflex angle"); | ||
| }); | ||
| // Case 6: Invalid angles | ||
| test(`should return "Invalid angle" when (angle > 361, angle < 0)`, () => { | ||
| // Test various acute angles, including boundary cases | ||
| expect(getAngleType(505)).toEqual("Invalid angle"); | ||
| expect(getAngleType(0)).toEqual("Invalid angle"); | ||
| expect(getAngleType(699)).toEqual("Invalid angle"); | ||
| }); | ||
|
Comment on lines
+17
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've left a comment in all these tests which doesn't seem relevant to them - get rid! |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,3 +8,17 @@ const isProperFraction = require("../implement/2-is-proper-fraction"); | |
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(1, 0)).toEqual(false); | ||
| }); | ||
| test(`should return true when denominator is smaller or equal to the numerator`,() =>{ | ||
| expect(isProperFraction(2, 4)).toEqual(true); | ||
| expect(isProperFraction(3, 3)).toEqual(true); | ||
| expect(isProperFraction(5, 10)).toEqual(true); | ||
| expect(isProperFraction(2, 2)).toEqual(true); | ||
| }); | ||
|
|
||
| test(`should return false when denominator is bigger than the numerator`, () => { | ||
| expect(isProperFraction(5, -2)).toEqual(false); | ||
| expect(isProperFraction(-1, 0)).toEqual(false); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case, which test does it belong in? What grouping of behaviours does it fall into? |
||
| expect(isProperFraction(12, 4)).toEqual(false); | ||
|
|
||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,12 +9,39 @@ test(`Should return 11 when given an ace card`, () => { | |
| expect(getCardValue("A♠")).toEqual(11); | ||
| }); | ||
|
|
||
| test(`Should return 10 when given a Joker card`, () => { | ||
| expect(getCardValue("J♥")).toEqual(10); | ||
| }); | ||
|
|
||
| test(`Should return 2 when given the 2 card`, () => { | ||
| expect(getCardValue("2♠")).toEqual(2); | ||
| }); | ||
|
|
||
| test(`Should return 10 when given a King card`, () => { | ||
| expect(getCardValue("K♠")).toEqual(10); | ||
| }); | ||
|
|
||
| test(`Should return 5 when given the 5 card`, () => { | ||
| expect(getCardValue("5♠")).toEqual(5); | ||
| }); | ||
|
|
||
| test(`Should return 8 when gine the 8 card`, () => { | ||
| expect(getCardValue("8♠")).toEqual(8); | ||
| }); | ||
|
|
||
| test(`Should return 10 when given the Queen card`, () => { | ||
| expect(getCardValue("Q♠")).toEqual(10); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests are pretty decent (maybe worth considering my above comment for whether there's a couple more you could add), but I want to talk about the test descriptions at the moment. Generally speaking, we don't always want to be so specific as to describe exactly what's happening in the assertion. Think about the previous function, your test descriptions were great! You separated it into
Rather than saying "returns true when numerator is 4 and denominator is 6", which would be too specific. Some of these tests are understandably individualised, such as testing for |
||
| // Suggestion: Group the remaining test data into these categories: | ||
| // Number Cards (2-10) | ||
| // Face Cards (J, Q, K) | ||
| // Invalid Cards | ||
|
|
||
| // To learn how to test whether a function throws an error as expected in Jest, | ||
| // please refer to the Jest documentation: | ||
| // https://jestjs.io/docs/expect#tothrowerror | ||
|
|
||
| // https://jestjs.io/docs/exåpect#tothrowerror | ||
| test(`Should not return any card should throw an error message`, () => { | ||
| expect(() => { | ||
| getCardValue("QQ♥"); | ||
| }).toThrow(); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you ever find yourself with a solution that looks like
And the
conditionitself is a boolean value (i.e. it evaluates totrueorfalse), there's a more succinct way we can write the code. Can you think what it might be?