Birmingham | 26-ITP-Jan | Tasleem Adedokun | Sprint 3 | Coursework 3 implement and rewrite#1170
Conversation
…s, including boundary test and invalid test.
tasleemadedokun
added
📅 Sprint 3
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
Functions are correct
-
Tests are also quite comprehensive
-
Some test descriptions could be made clearer
Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js
Outdated
Show resolved
Hide resolved
Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js
Outdated
Show resolved
Hide resolved
Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js
Outdated
Show resolved
Hide resolved
| test("Should throw an error for invalid cards", () => { | ||
| expect(() => getCardValue(null)).toThrow(); | ||
| expect(() => getCardValue(undefined)).toThrow(); | ||
| }) |
There was a problem hiding this comment.
Why not test also strings that are not in the expected format?
There was a problem hiding this comment.
Why not test also strings that are not in the expected format?
I have now tested for strings.
cjyuan
added
Reviewed
| test(`should return true for positive proper fractions (numerator < denominator)`, () => { | ||
| expect(isProperFraction(1, 2)).toEqual(true); | ||
| expect(isProperFraction(2, 4)).toEqual(true); | ||
| }) | ||
|
|
||
| // Case 3: negative proper fractions | ||
| test(`should return true for negative proper fractions (numerator < denominator)`, () => { | ||
| expect(isProperFraction(-1, -2)).toEqual(true); | ||
| expect(isProperFraction(-2, -4)).toEqual(true); | ||
| }) | ||
|
|
||
| // Case 4: improper fractions (numerator >= denominator) | ||
| test(`should return false for improper fractions`, () => { | ||
| expect(isProperFraction(4, 2)).toEqual(false); | ||
| expect(isProperFraction(4, 4)).toEqual(false); | ||
| expect(isProperFraction(-6, 4)).toEqual(false); | ||
| expect(isProperFraction(7, -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.
We can use pseudo-code and notations like
abs(...)or| ... |in the descriptions to more concisely describe the conditions (the "when" part).
I have done that.
There was a problem hiding this comment.
@cjyuan Thank you very much always. I always appreciate your reviews, and I have implemented all the changes. Thank you very much.
tasleemadedokun
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. I will mark this PR as complete first.
| // Invalid Cards | ||
| test("Should throw an error for invalid cards", () => { | ||
| expect(() => getCardValue(null)).toThrow(); | ||
| expect(() => getCardValue(undefined)).toThrow(); |
There was a problem hiding this comment.
Your code didn't actually checked if the value of card is string or not.
Your function would throw a TypeError (kind of an unexpected error) instead of the controlled "Invalid card" error.
Including these tests in the test script may send the wrong message that the function should properly check the type of the argument and classify any value that is not a string as "Invalid card".
| expect(() => getCardValue("11")).toThrow(); | ||
| expect(() => getCardValue("happy")).toThrow(); |
There was a problem hiding this comment.
Could also consider testing these cases:
- Valid rank + invalid suit character
- Valid rank + no suit
- Empty string
- Valid suit + invalid rank
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Brief;
I have done all the coursework to the best of my ability, and I made several commits to effect changes.