London | 26-ITP-January | Khaliun Baatarkhuu | Sprint 3 | Implement and rewrite tests.#1171
Conversation
Implemented the getAngleType function to determine angle types and added tests for various cases.
Implement getCardValue function to determine card value based on rank and suit, including error handling for invalid cards.
Added checks for zero denominator and updated test cases for proper fractions.
khaliun-dev
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
Functions are correctly implemented
-
Tests are comprehensive
-
Some test descriptions could be made clearer
Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js
Outdated
Show resolved
Hide resolved
| test(`should return true when numerator is smaller than denominator`, () => { | ||
| expect(isProperFraction(1, 2)).toEqual(true); | ||
| expect(isProperFraction(3, 4)).toEqual(true); | ||
| }); | ||
|
|
||
| // Equal numbers → not proper | ||
| test(`should return false when numerator equals denominator`, () => { | ||
| expect(isProperFraction(2, 2)).toEqual(false); | ||
| }); | ||
|
|
||
| // Improper fractions | ||
| test(`should return false when numerator is greater than denominator`, () => { | ||
| expect(isProperFraction(5, 4)).toEqual(false); | ||
| expect(isProperFraction(10, 3)).toEqual(false); | ||
| }); | ||
|
|
||
| // Zero numerator | ||
| test(`should return true when numerator is zero and denominator is non-zero`, () => { | ||
| expect(isProperFraction(0, 5)).toEqual(true); | ||
| }); | ||
|
|
||
| // Negative numbers | ||
| test(`should handle negative numbers correctly`, () => { | ||
| expect(isProperFraction(-1, 2)).toEqual(true); | ||
| expect(isProperFraction(1, -2)).toEqual(true); | ||
| expect(isProperFraction(-3, -2)).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
We can use pseudo-code and notations like abs(...) or | ... | in the descriptions to more concisely describe the conditions (the "when" part).
There was a problem hiding this comment.
test("|n| < |d| with negative signs → true, |n| > |d| → false", () => {
expect(isProperFraction(-1, 2)).toBe(true);
expect(isProperFraction(1, -2)).toBe(true);
expect(isProperFraction(-3, -2)).toBe(false);
});
There was a problem hiding this comment.
With this category, when one of the tests fail, it may not be easy to find out whether the function fails to identify proper fraction or improper fraction.
There was a problem hiding this comment.
test("|n| < |d| with mixed negative signs → true", () => {
expect(isProperFraction(-1, 2)).toBe(true);
expect(isProperFraction(1, -2)).toBe(true);
});
test("|n| > |d| with both negative → false", () => {
expect(isProperFraction(-3, -2)).toBe(false);
});
A strategy of separating tests by logic as much as possible helps pinpoint the test failure point better?
There was a problem hiding this comment.
-
Strategy is sound.
-
The format of the test description is quite uncommon. Normal practice is to describe them in the form:
should <expected_behavior> when <condition>.
For example,"should return true when |numerator| < |denominator|"(more natural to English-speaking people)
Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js
Show resolved
Hide resolved
cjyuan
added
Reviewed
khaliun-dev
added
the
Needs Review
|
The proposed changes look good. Did you update your code according? I don't see any new commits on this PR branch. |
cjyuan
removed
the
Needs Review
Refactor tests for negative numbers to improve clarity and consistency.
|
Good evening @cjyuan I have updated my code with the approved changes. Thanks. |
khaliun-dev
added
the
Needs Review
|
Did you run |
cjyuan
removed
the
Needs Review
2 participants
Self checklist
Changelist
Completed all the exercises in implement and rewrite, and performed the necessary tests.
Questions
None so far.