Skip to content

Conversation

@Abrsh100
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 made all the requerd change so that it will return properly

Questions

No questions

@Abrsh100 Abrsh100 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 29, 2025
@LonMcGregor LonMcGregor added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Nov 4, 2025
Comment on lines +20 to +33
test("should count multiple occurrences of a character", () => {
const str = "banana";
const char = "a";
const count = countChar(str, char);
expect(count).toEqual(3);
});

test("should count multiple occurrences of a character", () => {
const str = "ananas";
const char = "a";
const count = countChar(str, char);
expect(count).toEqual(3);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

How are these two tests different from the test on lines 13-18? They are all testing "should count multiple occurrences of a character". If they belong to the same category, we could test them in the following manner:

    expect( countChar("aaaaa", "a") ).toEqual(5);
    expect( countChar("ananas", "a") ).toEqual(3);
    ...

Can you think of different cases which we could test?

Copy link
Author

Choose a reason for hiding this comment

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

yes, i forget the last criteria when there is no char in the string and the output is 0.

Comment on lines 14 to 37

test("should return '2nd' for 2", () => {
expect(getOrdinalNumber(2)).toEqual("2nd");
});

test("should return '3rd' for 3", () => {
expect(getOrdinalNumber(3)).toEqual("3rd");
});

test("should return '11th' for 11", () => {
expect(getOrdinalNumber(11)).toEqual("11th");
});

test("should return '12th' for 12", () => {
expect(getOrdinalNumber(12)).toEqual("12th");
});

test("should return '13th' for 13", () => {
expect(getOrdinalNumber(13)).toEqual("13th");
});

test("should return '24th' for 14", () => {
expect(getOrdinalNumber(24)).toEqual("24th");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure thorough testing, we need broad scenarios that cover all possible cases.
Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories.
Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.

For example, we can prepare a test for numbers 2, 22, 132, etc. as

test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
    expect( getOrdinalNumber(2) ).toEqual("2nd");
    expect( getOrdinalNumber(22) ).toEqual("22nd");
    expect( getOrdinalNumber(132) ).toEqual("132nd");
});

Copy link
Author

Choose a reason for hiding this comment

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

ok, got it thank you

Comment on lines 29 to 50
// case: Negative Count:
// Given a target string str and a negative integer count,
// When the repeat function is called with these inputs,
// Then it should throw an error or return an appropriate error message, as negative counts are not valid.
test("should repeat the string count times", () => {
const str = "hello";
const count = -1;
const repeatedStr = repeat(str, count);
expect(repeatedStr).toEqual("hellohellohello");
});
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 match the case suggested in the comment.

  • Can your function pass this test?

To test if a function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)

Copy link
Author

Choose a reason for hiding this comment

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

yes it was not working, I fellow the instruction again and add if cases to pass the test

@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 Nov 6, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Nov 6, 2025

@LonMcGregor Sorry. I missed the "Review in progress" label.

@cjyuan cjyuan removed the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Nov 6, 2025
@Abrsh100 Abrsh100 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 11, 2025
Comment on lines 10 to 22
test("append 'st' to numbers ending in 1, except those ending in 11 which is 11th", () => {
expect( getOrdinalNumber(1) ).toEqual("1st");
expect( getOrdinalNumber(11) ).toEqual("11th");
expect( getOrdinalNumber(21) ).toEqual("21st");
expect( getOrdinalNumber(221) ).toEqual("221st");
});

test("append 'nd' to numbers ending in 2, except those ending in 12 which is 12th", () => {
expect( getOrdinalNumber(2) ).toEqual("2nd");
expect( getOrdinalNumber(12) ).toEqual("12th");
expect( getOrdinalNumber(22) ).toEqual("22nd");
expect( getOrdinalNumber(132) ).toEqual("132nd");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests being performed are and the test description do not quite match.

Comment on lines 27 to 39
// Then it should return an empty string, ensuring that a count of 0 results in an empty output.
test("should repeat the string count times", () => {
const str = "hello";
const count = 0;
const repeatedStr = repeat(str, count);
expect(repeatedStr).toEqual(" ");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

" " is not an empty string.

Comment on lines 2 to 4
if (typeof n !== 'number' || n < 0) {
return "Count must be a non-negative integer";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How would the caller distinguish the result of the following two function calls?

  1. repeat("Count must be a non-negative integer", 1)
  2. repeat("", -1)

Both function calls return the same value.

Comment on lines 45 to 50
test("should repeat the string count times", () => {
const str = "hello";
const count = -1;
const repeatedStr = repeat(str, count);
expect(repeatedStr).toEqual("Count must be a non-negative integer");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If you modified repeat() to throw an error when count is negative, and you wanted to test if the function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)

Comment on lines 39 to 44
test("should count multiple occurrences of a character", () => {
const str = "hello";
const char = "a";
const count = countChar(str, char);
expect(count).toEqual(0);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Test description does not quite match the test being performed.

@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 Nov 11, 2025
@Abrsh100 Abrsh100 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 12, 2025
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.

Other changes look good.

Comment on lines 10 to 14
// Then the function should return "1st"
test("append 'st' to numbers ending in 1, except those ending in 11 which is 11th", () => {
test("append 'st' to numbers ending in 1, except those ending in 11", () => {
expect( getOrdinalNumber(1) ).toEqual("1st");
expect( getOrdinalNumber(11) ).toEqual("11th");
expect( getOrdinalNumber(21) ).toEqual("21st");
expect( getOrdinalNumber(221) ).toEqual("221st");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the test in this script more comprehensive? Ideally the test should include categories that can cover all valid numbers.

@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 Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants