Manchester | 26-ITP-May | Szidonia Bodo | Sprint 3 | Implement and review test by bodoszidi · Pull Request #1520 · CodeYourFuture/Module-Structuring-and-Testing-Data

bodoszidi · 2026-07-16T18:47:17Z

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

Changelist

Created a new pull request only for implementation and review test

Liam310 · 2026-07-22T08:08:35Z

+    return true;
+  } else {
+    return false;
+  }


If you ever find yourself with a solution that looks like

if (condition) { return true; } else { return false; }

And the condition itself is a boolean value (i.e. it evaluates to true or false), there's a more succinct way we can write the code. Can you think what it might be?

Liam310 · 2026-07-22T08:08:41Z

+assertEquals(isProperFraction(-8, 2), false);
+assertEquals(isProperFraction(4, 0), false);
+assertEquals(isProperFraction(0, 0), false);
+assertEquals(isProperFraction(-2, -2), false)


If you were to check the case of isProperFraction(-1, 2), what do we want to see as the answer? And would that test pass?

(Things get a little awkwardly mathsy with negative fractions, I recognise)

Liam310 · 2026-07-22T08:09:13Z

+    return Number(removeSuit);
+  } else {
+    throw new Error("Error")
+  }


A great solution, very readable 👌

Just one point, you've declared let removeSuit - should this be a let or a const? In either case, why?

Liam310 · 2026-07-22T08:09:16Z

+assertEquals(getCardValue("J♦"), 10);
+assertEquals(getCardValue("7♥"), 7);
+assertEquals(getCardValue("A♥"), 11);
+assertEquals(getCardValue("8♥"), 8);


When writing a test suite, we want to try and cover all possible "behaviours" of our function. Here I can see you've covered:

Hearts

Spades

Diamonds

Numbered cards

Jacks

Aces

Looking at that list, what have you not covered?

Liam310 · 2026-07-22T08:10:11Z

+test(`should return "Right angle" when (angle = 90)`, () => {
+  // Test various acute angles, including boundary cases
+  expect(getAngleType(90)).toEqual("Right angle");
+});
 // Case 3: Obtuse angles
+test(`should return "Obtuse angle" when (angle < 180, angle > 90)`, () => {
+  // Test various acute angles, including boundary cases
+  expect(getAngleType(91)).toEqual("Obtuse angle");
+  expect(getAngleType(179)).toEqual("Obtuse angle");
+  expect(getAngleType(99)).toEqual("Obtuse angle");
+});
 // Case 4: Straight angle
+test(`should return "Straight angle" when (angle = 180)`, () => {
+  // Test various acute angles, including boundary cases
+  expect(getAngleType(180)).toEqual("Straight angle");
+});
 // Case 5: Reflex angles
+test(`should return "Reflex angle" when (angle < 360, angle > 180)`, () => {
+  // Test various acute angles, including boundary cases
+  expect(getAngleType(189)).toEqual("Reflex angle");
+  expect(getAngleType(359)).toEqual("Reflex angle");
+  expect(getAngleType(199)).toEqual("Reflex angle");
+});
 // Case 6: Invalid angles
+test(`should return "Invalid angle" when (angle > 361, angle < 0)`, () => {
+  // Test various acute angles, including boundary cases
+  expect(getAngleType(505)).toEqual("Invalid angle");
+  expect(getAngleType(0)).toEqual("Invalid angle");
+  expect(getAngleType(699)).toEqual("Invalid angle");
+});


You've left a comment in all these tests which doesn't seem relevant to them - get rid!

Liam310 · 2026-07-22T08:10:20Z

+
+test(`should return false when denominator is bigger than the numerator`, () => {
+  expect(isProperFraction(5, -2)).toEqual(false);
+  expect(isProperFraction(-1, 0)).toEqual(false);


expect(isProperFraction(-1, 0)).toEqual(false);

This test case, which test does it belong in? What grouping of behaviours does it fall into?

Liam310 · 2026-07-22T08:10:36Z

+
+test(`Should return 10 when given the Queen card`, () => {
+  expect(getCardValue("Q♠")).toEqual(10);
+});


The tests are pretty decent (maybe worth considering my above comment for whether there's a couple more you could add), but I want to talk about the test descriptions at the moment.

Generally speaking, we don't always want to be so specific as to describe exactly what's happening in the assertion. Think about the previous function, your test descriptions were great! You separated it into

When the numerator is bigger than the denominator

When the numerator is smaller than the denominator

Rather than saying "returns true when numerator is 4 and denominator is 6", which would be too specific.

Some of these tests are understandably individualised, such as testing for "Q♠" - there's a very specific input that will only produce one output. But on the contrary, your tests on lines 16, 24, and 28 are all really testing the same behaviour, so probably ought to be one test. My question for you is: what would be a good test description for that test?

Liam310 · 2026-07-22T08:20:12Z

Hey @bodoszidi! 👋

Good work on your first journey into using tests. Writing tests from scratch is not easy, it's a skill that takes a while to develop but there's a lot of good stuff in here.

My comments are mostly about focussing on how to organise our test cases. The main things we want to be doing are:

Trying to achieve good test coverage, which means "have we considered all possible scenarios here?"
Defining tests according to "behaviours" of the function, which means thinking about "how does the function behave with a certain type of input?" and figuring out how to group those inputs when relevant. A good rule of thumb is: if they're all handled by the same bit of logic in the function, they should probably be grouped as part of the same behaviour.

Please do respond to any comments with answers or further questions if you aren't sure what I'm talking about! 😄

bodoszidi added 6 commits July 16, 2026 19:30

Implemented the function and tested

f0ca3d7

added test cases

5ad372d

implemented function and tested

5c0d472

added jest test cases

b28e30e

implemented function and tested

241ef30

added jest test cases

f0b6faa

This comment has been minimized.

Sign in to view

bodoszidi added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Structuring-And-Testing-Data The name of the module. labels Jul 16, 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 Jul 16, 2026

This comment has been minimized.

Sign in to view

bodoszidi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 16, 2026

bodoszidi mentioned this pull request Jul 16, 2026

26-ITP-May | Structuring and testing data | Szidonia Bodo | bodoszidi/My-Coursework-Planner#51

Open

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 Jul 21, 2026

Liam310 reviewed Jul 22, 2026

View reviewed changes

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 Jul 22, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Manchester | 26-ITP-May | Szidonia Bodo | Sprint 3 | Implement and review test#1520

Manchester | 26-ITP-May | Szidonia Bodo | Sprint 3 | Implement and review test#1520
bodoszidi wants to merge 6 commits into
CodeYourFuture:mainfrom
bodoszidi:Sprint-3.1

bodoszidi commented Jul 16, 2026 •

edited

Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Liam310 Jul 22, 2026

Uh oh!

Liam310 Jul 22, 2026 •

edited

Loading

Uh oh!

Liam310 Jul 22, 2026

Uh oh!

Liam310 Jul 22, 2026

Uh oh!

Liam310 Jul 22, 2026

Uh oh!

Liam310 Jul 22, 2026

Uh oh!

Liam310 Jul 22, 2026 •

edited

Loading

Uh oh!

Liam310 commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

bodoszidi commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelist

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Liam310 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Liam310 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Liam310 Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Liam310 commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bodoszidi commented Jul 16, 2026 •

edited

Loading

Liam310 Jul 22, 2026 •

edited

Loading

Liam310 Jul 22, 2026 •

edited

Loading