Skip to content

Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 1-Implement-and-rewrite-test#1259

Open
Jacknguyen4438 wants to merge 2 commits intoCodeYourFuture:mainfrom
Jacknguyen4438:Jest-test-case-sprint-3A
Open

Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 1-Implement-and-rewrite-test#1259
Jacknguyen4438 wants to merge 2 commits intoCodeYourFuture:mainfrom
Jacknguyen4438:Jest-test-case-sprint-3A

Conversation

@Jacknguyen4438
Copy link

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

Changelist

I have don't basic understand on how to correctly writing normal test and jest test frame work, I am waiting for this work to be review.

Questions

I have no question.

@Jacknguyen4438 Jacknguyen4438 changed the title Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 1- Implement-and-rewrite-test Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 1-Implement-and-rewrite-test Mar 13, 2026
@Jacknguyen4438 Jacknguyen4438 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 Mar 13, 2026
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

  • Function implementation is correct.

Describing tests can be quite challenging. Feel free learn from AI how to keep the test descriptions concise.

Comment on lines +44 to +50
!cardRanks.includes(card.slice(0,-1)) ||
!cardSuits.includes(card.slice(-1))
) {
throw new Error("Invalid card");
}

switch (card.slice(0, -1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider storing the extracted rank and suit character in some variables first.

!cardRanks.includes(rank) || !cardSuits.includes(suit)

is probably more readable than

!cardRanks.includes(card.slice(0,-1)) || !cardSuits.includes(card.slice(-1))

Comment on lines +37 to +40
test(`should return "Invalid angle" when ( 180 < angle < 360)`, () => {
expect(getAngleType(-1)).toEqual("Invalid angle");
expect(getAngleType(-10)).toEqual("Invalid angle");
expect(getAngleType(361)).toEqual("Invalid angle");
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The test description is not quite correct.

  • Why not test both boundary cases?

Comment on lines +20 to +22
test(`should return false when denominator is infinity`, () => {
expect(isProperFraction(1, 23443243n)).toEqual(true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The test description does not quite match the test.

  • A large "bigint" type value is not the same as infinity. The closest thing to infinity in JavaScript is the predefined named constant Infinity -- it is a value of type "number".

Comment on lines +14 to +19
test(`should return false when numerator is negative`, () => {
expect(isProperFraction(-1, 2)).toEqual(false);
});
test(`should return false when denominator is negative`, () => {
expect(isProperFraction(1, -2)).toEqual(false);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

These test descriptions are not quite accurate.

For proper fractions, the condition is, "the absolute value of denominator is larger than the absolute value of the numerator". So we can create a category for proper fractions and describe it as:
should return true when abs(numerator) < abs(denominator).

Comment on lines +14 to +28
test("Should return 2 for '2♠'", () => {
expect(getCardValue("2♠")).toEqual(2);
});

test("Should return 5 for '5♥'", () => {
expect(getCardValue("5♥")).toEqual(5);
});

test("Should return 9 for '9♦'", () => {
expect(getCardValue("9♦")).toEqual(9);
});

test("Should return 10 for '10♣'", () => {
expect(getCardValue("10♣")).toEqual(10);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

When preparing tests, we should ensure the tests cover all possible cases. If we specify a test for individual card, we will need about 53 tests to cover all possible cases. Instead, we could consider classifying all possible values into different categories, and then within each category we test some samples.

For example, one possible category for getCardValue() is, "should return the value of number cards (2-10)", and we can prepare the test as

test("should return the value of number cards (2-10)", () => {
    expect(getCardValue("2♣︎")).toEqual(2);
    expect(getCardValue("5♠")).toEqual(5);
    expect(getCardValue("10♥")).toEqual(10);
    // Note: We could also use a loop to check all values from 2 to 10.
});

Can you practice preparing tests in this fashion?

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Module-Structuring-And-Testing-Data The name of the module. Reviewed Volunteer to add when completing a review with trainee action still to take. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants