-
-
Notifications
You must be signed in to change notification settings - Fork 195
WESTMIDLANDS| ITP-MAY 2025| ROJA ALAGURAJAN| Module Structuring and Testing Data|Sprint-3 #670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
WESTMIDLANDS| ITP-MAY 2025| ROJA ALAGURAJAN| Module Structuring and Testing Data|Sprint-3 #670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good.
The tests in Sprint-3/2-mandatory-rewrite/3-get-card-value.test.js
have rooms for improvement. I am sure you can fix them relatively easily, so I will go ahead and mark this PR as "Completed" first.
/*test("should identify right angle (90°)", () => { | ||
expect(getAngleType(90)).toEqual("Right angle");}); | ||
|
||
test("should identify acute angle (45°)", () => { | ||
expect(getAngleType(45)).toEqual("Acute angle");}); | ||
|
||
test("should identify obtuse angle (120°)", () => { | ||
expect(getAngleType(120)).toEqual("Obtuse angle");}); | ||
|
||
test("should identify straight angle (180°)", () => { | ||
expect(getAngleType(180)).toEqual("Straight angle");}); | ||
|
||
test("should identify reflex angle (220°)", () => { | ||
expect(getAngleType(220)).toEqual("Reflex angle");}); | ||
|
||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to keep this code? Code submitted to a PR is like the final work. As such, it is best practices to keep the code in the PR as clean as possible, including removal of unused code.
test("should return rank for Number Cards", () => { | ||
const fiveofHearts = getCardValue("5♥"); | ||
expect(fiveofHearts).toEqual(5); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"2♥" and "10♥" -- the boundary values usually worth testing.
Given that the set of possible values is small (only 9 different values), we can even use a loop to generate all possible test values.
The same principle applies to the test that deals with the face cards.
test("should return 11 for AceCard", () => { | ||
const AceCard = getCardValue("A♥"); | ||
expect(AceCard).toEqual(11); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is similar to the test at lines 3-7. (This comment was from the previous review).
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.