Manchester |25-ITP-Sep|Mahtem T. Mengstu |Sprint 3|coursework/sprint-3-implement-and-rewrite #851
Conversation
…ssary functions added
…ote tests in jest
Mahtem
added
📅 Sprint 3
Mahtem
changed the title
Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js
Outdated
Show resolved
Hide resolved
| else if (!isNaN(rank )) { | ||
| 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. For examples, "0x02", "2.1", or "0002". Do you want to consider these strings as valid ranks?
There was a problem hiding this comment.
Thank you, for bringing that insight. I have tried to modify the if statement and add necessary test.
| test("should identify reflex angle (240)", () => { | ||
| expect(getAngleType(240)).toEqual("Reflex angle"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
To ensure thorough testing, we need broad scenarios that cover all possible cases.
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,
test("should identify reflex angle (180 < angle < 360)", () => {
expect(getAngleType(300)).toEqual("Reflex angle");
expect(getAngleType(359.999)).toEqual("Reflex angle");
expect(getAngleType(180.001)).toEqual("Reflex angle");
});
There was a problem hiding this comment.
Thank you, I have modified the tests to cover broader scenarios.
| test("should return true for a negative fraction", () => { | ||
| expect(isProperFraction(-2, 3)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
The test description is a bit misleading because not all negative fractions are proper fractions.
For example, -4/3 is an improper fraction.
There was a problem hiding this comment.
Yes, that is right, I have changed the misleading description into "should return true for a proper fraction with negative numerator".
| test("should number cards for (2-10)", () => { | ||
| const aceofSpades = getCardValue("6♠"); | ||
| expect(aceofSpades).toEqual(6); | ||
| }); |
There was a problem hiding this comment.
-
The test description is incomplete.
-
We could test multiple values within a test. The boundary case, 2 and 10, are good candidates to test.
There was a problem hiding this comment.
Thank you, I have completed the incomplete description and added a test for multiple values such as boundary cases 2 and 10 in line 12-17
| // test("should return 11 for Ace of Spades", () => { | ||
| // const aceofSpades = getCardValue("A♠"); | ||
| // expect(aceofSpades).toEqual(11); | ||
| // }); |
There was a problem hiding this comment.
-
We could remove code that's no longer needed (to keep the code clean).
-
The function is not expected to validate the suit character. On line 5, mentioning 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.
There was a problem hiding this comment.
Good, that's right, I have removed misleading description. and changed it.
cjyuan
added
Reviewed
…ndle possible edge cases.
|
Can you also address all my inline comments? You can see them on #851 (Sorry, thought you had finished making changes) |
…ement to handle valid numeric literals such as '2.1' and '002'.
…lues such as boundr values 2 and 10 in line 12-17
I have addressed all your inline comments. |
2 participants
Learners, PR Template
Self checklist
Changelist
Jest testing was performed.
Questions
Any question will be asked in Slack.