London | 26-ITP-Jan | Ebrahim Moqbel | Sprint 3 | Implement and Rewrite Tests#1194
London | 26-ITP-Jan | Ebrahim Moqbel | Sprint 3 | Implement and Rewrite Tests#1194Ebrahim-Moqbel wants to merge 7 commits intoCodeYourFuture:mainfrom
Conversation
Ebrahim-Moqbel
added
Complete
favourO
added
the
Review in progress
favourO
left a comment
favourO
left a comment
There was a problem hiding this comment.
Nice work,
I left a few comments on specific blocks where the implementation and tests don’t quite match yet, and where there are a couple of small syntax or logic issues.
Once you address them, the PR should be in a good place.
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
| } | ||
| if (!["♠", "♥", "♦", "♣"].includes(cardFace)) { | ||
| } | ||
| throw new Error(`Invalid card rank: ${rank}`); |
There was a problem hiding this comment.
Can you confirm that the error messages in the implementation match what the tests expect?
There was a problem hiding this comment.
Thank you favourO. I have matched the function message thron error to the tests expectation.
| // Suggestion: Group the remaining test data into these categories: | ||
| // Number Cards (2-10) | ||
| test(`Should return the correct value for number cards (2-10)`), () => { | ||
| expect(getCardValue("2♠")).toEqual(2); |
There was a problem hiding this comment.
The test call here looks slightly mis-structured, the callback function appears to be outside the test(...) call.
It might be worth checking the syntax so the test runner can execute this block correctly.
| expect(getCardValue("Q♠")).toEqual(10); | ||
| expect(getCardValue("K♠")).toEqual(10); | ||
| } | ||
| // Invalid Card suits |
There was a problem hiding this comment.
Nice test cases.
Can you confirm that the implementation throw the same error messages that the tests expect here? we want to keep the behaviour predictable
also can you check if you properly closed the test block for this test(Should throw an error when given an invalid card suit)
favourO
added
Reviewed
…nstead of invalid angle.
favourO
added
the
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
completed the implement and rewrite test as follows:
-implemented getCardValue with validation and worte jest test using .toThrow() for invalid input