London | 25-ITP-May | Hendrine Zeraua | Sprint 3 | Structuring and Testing Data #661
Conversation
- Return 11 for Ace ("A")
- Return numeric value for cards 2-10
- Return 10 for face cards ("J", "Q", "K")
- Throw error for invalid card ranks
- Added assertEquals helper and step-by-step tests for each case
cjyuan
added
the
Review in progress
| 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 so that they can cover all valid integers?
There was a problem hiding this comment.
I grouped the ordinal number tests by suffix categories to cover all valid integers with fewer, clearer cases. The function stays the same since it already works correctly.
| test("validates a correct card number: 9999777788880000", () => { | ||
| expect(validateCreditCard("9999777788880000")).toBe(true); | ||
| }); | ||
|
|
||
| test("validates a correct card number: 6666666666661666", () => { | ||
| expect(validateCreditCard("6666666666661666")).toBe(true); | ||
| }); |
There was a problem hiding this comment.
These two tests can be combined and placed under the category "should return true for valid card number".
We don't usually describe the specific value we are testing unless such value is a unique value.
There was a problem hiding this comment.
HI CJ,
Thanks for the suggestion! I've grouped the valid card tests under one case for clarity and maintainability. All tests now focus on meaningful categories instead of specific values.
Thanks again for reviewing my work.
cjyuan
added
Complete
| test("appends 'th' to all other numbers", () => { | ||
| expect(getOrdinalNumber(4)).toBe("4th"); | ||
| expect(getOrdinalNumber(9)).toBe("9th"); | ||
| expect(getOrdinalNumber(100)).toBe("100th"); | ||
| expect(getOrdinalNumber(111)).toBe("111th"); | ||
| }); |
There was a problem hiding this comment.
When a test fails with the message "... all other numbers", it may be unclear what "other numbers" actually refers to. A more specific and informative message, such as "... numbers ending in 0 or 4–9", would make the output easier to understand and act on.
There was a problem hiding this comment.
Updated the final test description to be more specific ("numbers ending in 0 or 4–9") as suggested. No changes to test logic.
Thank you very much.
cjyuan
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
1- key-implement
2- mandatory-rewrite
3- mandatory-practice
4- stretch-investigate
Questions
No questions at the moment.