Skip to content

Cape Town | 26-ITP-Sept | Leigh Ross | Sprint 2 | Implement and rewrite tests - #1683

Open
leigh-ross wants to merge 10 commits into
CodeYourFuture:mainfrom
leigh-ross:coursework/sprint-2-implement-and-rewrite
Open

leigh-ross wants to merge 10 commits into
CodeYourFuture:mainfrom
leigh-ross:coursework/sprint-2-implement-and-rewrite

Conversation

@leigh-ross

Copy link
Copy Markdown

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Task code

CYF-1059

Changelist

Added 3 functions:

  • getAngleType
  • isProperFraction
  • getCardValue
    Added node tests for each Function.
    Rewrote node tests for jest.

@leigh-ross leigh-ross added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 12, 2026
@Liam310 Liam310 added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 12, 2026
Comment thread implement-and-rewrite-tests/implement/2-is-proper-fraction.js Outdated
@Liam310 Liam310 added Reviewed Volunteer to add when completing a review with trainee action still to take. Complete Volunteer to add when work is complete and all review comments have been addressed. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. Reviewed Volunteer to add when completing a review with trainee action still to take. Complete Volunteer to add when work is complete and all review comments have been addressed. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 12, 2026
Comment thread implement-and-rewrite-tests/implement/3-get-card-value.js Outdated
Comment on lines +15 to +17
test(`should return "Right angle" when (angle = 90)`, () => {
expect(getAngleType(90)).toEqual("Right angle");
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be interested to hear that Jest also comes with a describe function! Often when unit testing you might see:

describe("functionName", () => {
  test("should return... ", () => {
    // test body...
  })
  test("should return... ", () => {
    // test body...
  })
})

This is not a required change but I thought you'd be interested to know!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I just realised you used Jest's describe below 😄

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you have brought that up I wanted to know if it is good practice to use the describe function for only 1 test case? I like having my edge cases separate which is why they are alone in the code. What do you think about it?

Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js Outdated
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js Outdated
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js Outdated
@Liam310

Liam310 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@leigh-ross Hi Leigh! Great work overall on this coursework but I have left a few changes you need to look into. In general, you should always make sure to run your tests before committing - you don't want to be committing work that isn't passing tests!

A quick note on your commits: I really appreciate you committing once per function, I think that is a good practice for using commits to track when some unit of work is complete. The commit messages could use a little bit of improvement for you to consider in the future. I would try to write commit messages by stating what the commit will change if it is merged (I know we don't merge PRs at CYF, but in a workplace you would). Imagine they are following on from the phrase "this commit will...", for example instead of:

Angle type: Function logic works, added test cases, all tests pass

You could say

implement getAngleType function with full test suite

You don't need to make these changes as part of this PR but bear it in mind for future :)

@Liam310 Liam310 added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 13, 2026
…dded error for both if conditions do not pass. Added tests in node and jest for the incorrect suit case and error throwing. All tests pass.
@leigh-ross leigh-ross added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants