LONDON | ITP_MAY | Mirabel Adom | Module-Structuring-and-Testing-Data | Sprint-3 #665
Conversation
cjyuan
added
the
Review in progress
There was a problem hiding this comment.
Can you implementation pass the tests in this script?
| test("should identify reflex angle (e.g., 181°)", () => { | ||
| expect(getAngleType(181)).toEqual("Reflex angle"); | ||
| }); |
There was a problem hiding this comment.
We could also prepare the test like this:
test("should identify reflex angle (180 < angle < 360)", () => {
expect(getAngleType(300)).toEqual("Reflex angle");
expect(getAngleType(359.999)).toEqual("Reflex angle");
expect(getAngleType(181)).toEqual("Reflex angle");
});
- No need to specify the value being tested in the test description.
- We can check different samples within a test.
| if ( | ||
| typeof numerator !== "number" || | ||
| typeof denominator !== "number" || | ||
| denominator === 0 | ||
| ) { | ||
| return false; // invalid input | ||
| } | ||
|
|
||
| const value = numerator / denominator; | ||
| return value > 0 && value < 1; | ||
| } |
There was a problem hiding this comment.
In mathematics, -A/B == A/-B == -(A/B), and -A/-B == A/B for any integers A and B (B ≠ 0).
They represent a proper fraction if A < B and A ≠ 0 and B ≠ 0.
So isProperFraction(-4, 3) should return false because 4 >= 3.
So isProperFraction(-2, 5) should return true because 2 < 5.
Consider comparing the absolute value of the numerator and the denominator instead.
|
|
||
| let rank = card; | ||
|
|
||
| if (card.startsWith("10")) { |
There was a problem hiding this comment.
This approach will also accept values like "100000♣" or "10ABC♣".
Consider removing the suit character from card and then check the remaining characters.
| test("should return 2 for 2 of Hearts", () => { | ||
| expect(getCardValue("2♥")).toEqual(2); | ||
| }); | ||
|
|
||
| test("should return 9 for 9 of Clubs", () => { | ||
| expect(getCardValue("9♣")).toEqual(9); | ||
| }); | ||
|
|
||
| test("should return 10 for 10 of Diamonds", () => { | ||
| 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.
});
| test("should return 10 for Jack of Clubs", () => { | ||
| expect(getCardValue("J♣")).toEqual(10); | ||
| }); | ||
|
|
||
| test("should return 10 for Queen of Hearts", () => { | ||
| expect(getCardValue("Q♥")).toEqual(10); | ||
| }); | ||
|
|
||
| test("should return 10 for King of Spades", () => { | ||
| expect(getCardValue("K♠")).toEqual(10); | ||
| }); | ||
|
|
There was a problem hiding this comment.
We could combine these tests and place them under the category "should return 10 for face cards (J, Q, K)".
| test("should return 11 for Ace of Diamonds", () => { | ||
| expect(getCardValue("A♦")).toEqual(11); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test is similar to the test at lines 3-6. The function is not expected to check the suit character, so we don't need separate tests to ensure the function works for different suits.
| test("should return 0 for invalid card Z♣", () => { | ||
| expect(getCardValue("Z♣")).toEqual(0); | ||
| }); |
There was a problem hiding this comment.
Can you also add these checks to ensure the function can recognize these values are invalid card?
expect(getCardValue("100♣")).toEqual(0);
expect(getCardValue("3.1416♣")).toEqual(0);
expect(getCardValue("0x03♣")).toEqual(0);
expect(getCardValue("0003♣")).toEqual(0);
expect(getCardValue("QQ♣")).toEqual(0);
| test("should return '2nd' for 2", () => { | ||
| expect(getOrdinalNumber(2)).toEqual("2nd"); | ||
| }); |
There was a problem hiding this comment.
To ensure thorough testing, we need broad scenario coverage. Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories. Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.
For example, we can prepare a test for numbers 2, 22, 132, etc. as
test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
expect( getOrdinalNumber(2) ).toEqual("2nd");
expect( getOrdinalNumber(22) ).toEqual("22nd");
expect( getOrdinalNumber(132) ).toEqual("132nd");
});
Can you update the tests in this scripts so that they can cover all possible valid integers?
cjyuan
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
In this PR, I have fouced on implementing function and creating valid test cases.
Questions
Ask any questions you have for your reviewer.