Manchester | 26-ITP-Sep | Salah Alsabhi | Sprint 2 | implement and rewrite tests by salahalsabhi · Pull Request #1667 · CodeYourFuture/Module-Structuring-and-Testing-Data

salahalsabhi · 2026-10-09T12:43:44Z

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

Implemented three functions and covered them with tests in both node:test and Jest.

Questions

No questions

hackertainment

The implementation looks good. Just need to design more unique/special/boundary test cases. Keep it up.

hackertainment · 2026-10-09T20:00:36Z

While this is correct, I find it may be a bit hard to read. Sometimes using else if may look cleaner.

hackertainment · 2026-10-09T20:02:09Z

+test("corrctly get angle type", function(){
+    assert.equal(getAngleType(400), "Invalid angle");
+});


There should be two boundary cases for invalid angle. Can you think of them? Please add them back.

hackertainment · 2026-10-09T20:03:46Z

Good coverage of test cases.

hackertainment · 2026-10-09T20:05:57Z

+test("Basic proper fraction", () => {
+  assert.equal(isProperFraction(-1, 2), true);
+});


Good coverage for positive numbers. However, there should be more test cases for negative numbers. Please add them. Thank you.

For positive number cases, you have:

numerator smaller than denominator

numerator equal to denominator

numerator greater than denominator

For zero cases, you have:

numerator is zero

denominator is zero

For mixed of positive and negative numbers, you have (-1, 2) and (1, -2) which means there are two combinations still not tested yet;
and for negative number cases, you just have (-1, -2) which also means one test is still missing.

You are almost there, so just add them back. Please let me know here in the comment if you cannot think of them. Thank you.

hackertainment · 2026-10-09T20:06:46Z

There should be more test cases for negative numbers. Please add them. Thank you.

Same as https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/pull/1667/changes/f8013884556d98db75c28911ac3d769f56cc59c5..432760649a19a4342ba768fe73a20a4ecc485703#r4239273199, and please add the missing test cases.

hackertainment · 2026-10-09T20:09:10Z

 });

+
 // TODO: What other invalid card cases can you think of?


What other invalid card cases can you think of? Please add them. Thank you.

hackertainment · 2026-10-09T20:11:03Z

+test(`Should return numeric value for number cards`, () => {
+  expect(getCardValue("2♥")).toEqual(2);
+  expect(getCardValue("9♠")).toEqual(9);
+  expect(getCardValue("10♦")).toEqual(10);
+});
+
+test(`Should return 10 for face cards`, () => {
+  expect(getCardValue("J♣")).toEqual(10);
+  expect(getCardValue("Q♠")).toEqual(10);
+  expect(getCardValue("K♥")).toEqual(10);
+});


There should be one more special case for valid card. Please add it. Thank you.

hackertainment · 2026-10-09T20:11:33Z

+test(`Should throw for invalid cards`, () => {
+  expect(() => getCardValue("invalid")).toThrow(
+    /Expected a number followed by a suit, but got "invalid"/
+  );
+  expect(() => getCardValue("A")).toThrow(
+    /Expected a number followed by a suit/
+  );


There should be more invalid cards to test. Please add them. Thank you.

salahalsabhi · 2026-10-10T14:29:12Z

Thank you for the review, I did most of the notes you mentioned hope all good.

hackertainment · 2026-10-10T21:36:53Z

All other updates looked good except the following two issues (which are basically the same):

Keep it up!

salahalsabhi · 2026-10-11T05:47:30Z

Thanks for the feedback. I’ve added the missing cases:
mixed signs improper: (-5, 2) and (5, -2) → false
both negative improper: (-3, -2) → false
Updated in both node:test and Jest. Please take another look.

hackertainment · 2026-10-11T16:49:34Z

Well done. I hope you would now get a better idea on how to design good tests with maximum coverage but minimum number of cases. Just in case not, try to book a mentored coding session with a volunteer and discuss further about the unit test design process.
Congratulation, you have completed this PR successfully.

salahalsabhi added 4 commits October 9, 2026 09:45

Implement getAngleType with node:test coverage

0b66d44

Implement isProperFraction with node:test coverage

bcc603b

Implement getCardValue with node:test coverage

f8d6b5b

Rewrite implement tests with Jest

f801388

This comment has been minimized.

Sign in to view

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2026

This comment has been minimized.

Sign in to view

github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2026

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2026

hackertainment 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 9, 2026

hackertainment reviewed Oct 9, 2026

View reviewed changes

hackertainment mentioned this pull request Oct 9, 2026

Get Code Review and Iterate hackertainment/My-Coursework-Planner#8

Open

hackertainment 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 9, 2026

Address PR review: expand edge and invalid test cases

4327606

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 10, 2026

hackertainment removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 10, 2026

Add missing mixed-sign and negative improper fraction tests

1917427

salahalsabhi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 11, 2026

Conversation

salahalsabhi commented Oct 9, 2026

Learners, PR Template

Task code

Changelist

Questions

Uh oh!

This comment has been minimized.

This comment has been minimized.

hackertainment left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salahalsabhi commented Oct 10, 2026

Uh oh!

hackertainment commented Oct 10, 2026

Uh oh!

salahalsabhi commented Oct 11, 2026

Uh oh!

hackertainment commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hackertainment commented Oct 11, 2026 •

edited

Loading