Skip to content

Write tests#1019

Open
oliwialesniak-mate wants to merge 3 commits into
mate-academy:mainfrom
oliwialesniak-mate:main
Open

Write tests#1019
oliwialesniak-mate wants to merge 3 commits into
mate-academy:mainfrom
oliwialesniak-mate:main

Conversation

@oliwialesniak-mate
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Decision: REQUEST_CHANGES — One test contains a critical, blocking mismatch with the task requirements.

  • Critical issue (must fix): In src/splitInteger.test.js, the assertion for splitInteger(3, 5) expects [0, 0, 0, 1, 2] (max–min = 2), which violates the requirement that the difference between max and min must be ≤ 1. This contradicts the description and the implementation’s logic. Update that line to expect [0, 0, 1, 1, 1] so the test aligns with the spec and correctly validates the solution.
  • Why this is wrong: The spec requires an even distribution where parts differ by at most 1; [0, 0, 0, 1, 2] would allow a spread of 2, which is not permitted.
  • How to fix (exact location): In src/splitInteger.test.js, locate the test line asserting expect(splitInteger(3, 5)).toEqual([0, 0, 0, 1, 2]); and change it to expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);.

Minor improvements (optional):

  • Add invariant checks in tests (length equals parts, sum equals value, max - min <= 1, and sorted ascending) to make the suite more robust.

Everything else looks solid: divisible cases, single-part cases, and ordering tests are correct. Once you fix the incorrect expectation, the tests will align with the specification—great work overall!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/splitInteger.test.js Outdated

test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(3, 5)).toEqual([0, 0, 0, 1, 2]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The assertion for splitInteger(3, 5) contradicts the task description requirement that max - min ≤ 1. The test expects [0, 0, 0, 1, 2], but according to the description and the usual distribution logic the correct expected value is [0, 0, 1, 1, 1]. Update this assertion so the tests match the specification (or update the spec if the test is authoritative) .

Comment thread src/splitInteger.test.js Outdated
test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(3, 5)).toEqual([0, 0, 0, 1, 2]);
expect(splitInteger(1, 4)).toEqual([0, 0, 0, 1]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The expectation for splitInteger(1, 4) is correct — it should be [0, 0, 0, 1] (one 1 and three 0s), which satisfies sum and max-min ≤ 1.

Comment thread src/splitInteger.test.js Outdated
Comment on lines +7 to +8
expect(splitInteger(6, 2)).toEqual([3, 3]);
expect(splitInteger(20, 5)).toEqual([4, 4, 4, 4, 4]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: the tests for divisible cases verify equal partitioning (e.g. splitInteger(6, 2) and splitInteger(20, 5)). These align with the description's examples and check the core behavior.

Comment thread src/splitInteger.test.js Outdated
Comment on lines +13 to +14
expect(splitInteger(8, 1)).toEqual([8]);
expect(splitInteger(100, 1)).toEqual([100]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: single-part tests (splitInteger(8, 1), splitInteger(100, 1)) correctly assert that splitting into 1 part returns [value].

Comment thread src/splitInteger.test.js Outdated
Comment on lines +18 to +20
const result = splitInteger(17, 4); // → [4, 4, 4, 5]
expect(result).toEqual([4, 4, 4, 5]);
expect(result).toEqual(result.slice().sort((a, b) => a - b));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: the sorting test checks both the expected result and that the returned array is sorted ascending. This verifies the ordering requirement explicitly.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Decision: REQUEST_CHANGES

You’ve implemented splitInteger correctly and covered the key scenarios in tests, but there’s a critical blocker that will cause the test run to fail.

  • Critical blockers to fix:

    1. In src/splitInteger.test.js, you call checkInvariants(result, 3, 5) but checkInvariants is not defined in this file (it currently lives inside src/splitInteger.js). This will throw ReferenceError: checkInvariants is not defined and break the test run. Tests must not depend on helpers defined inside the implementation.
      • How to fix: Move the entire checkInvariants helper from src/splitInteger.js into src/splitInteger.test.js (or inline those assertions directly in the test). Then remove the helper from the implementation file so there are no Jest expect calls in production code.
    2. In src/splitInteger.js, remove all Jest expect calls. Assertions belong in the test suite, not in the implementation.
  • Minor improvements (non-blocking):

    • Consolidate the two duplicate tests named ‘should add zeros if value < numberOfParts’ in src/splitInteger.test.js into a single test that asserts the explicit expected array and checks the invariants (via the moved helper).

Once you move the helper into the test file and clean the implementation of test assertions, your tests should run cleanly. The core logic and most expectations look great—nice work on the corrected [0, 0, 1, 1, 1] case and the sorted checks.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/splitInteger.js Outdated
Comment on lines +22 to +36
function checkInvariants(result, value, numberOfParts) {
// length must equal numberOfParts
expect(result.length).toBe(numberOfParts);

// sum must equal value
expect(result.reduce((a, b) => a + b, 0)).toBe(value);

// must be sorted ascending
expect(result).toEqual([...result].sort((a, b) => a - b));

// max - min <= 1
const min = Math.min(...result);
const max = Math.max(...result);
expect(max - min).toBeLessThanOrEqual(1);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: checkInvariants is defined in the implementation file and uses Jest expect. Tests call checkInvariants(result, 3, 5) but this helper is not exported from the module, so the test run will fail with a ReferenceError. Move checkInvariants into src/splitInteger.test.js (or inline those expectations in the test) and remove it from the implementation file. See the helper definition and usage in the codebase for context .

Comment thread src/splitInteger.js Outdated
Comment on lines +24 to +27
expect(result.length).toBe(numberOfParts);

// sum must equal value
expect(result.reduce((a, b) => a + b, 0)).toBe(value);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Implementation file should not contain Jest assertions. The lines using expect (assertions) belong in tests. Remove these expect calls from the implementation and place them in the test helper inside src/splitInteger.test.js instead.

Comment thread src/splitInteger.js Outdated
Comment on lines +29 to +35
// must be sorted ascending
expect(result).toEqual([...result].sort((a, b) => a - b));

// max - min <= 1
const min = Math.min(...result);
const max = Math.max(...result);
expect(max - min).toBeLessThanOrEqual(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The helper asserts multiple invariants (length, sum, sorted, max-min <= 1). That's fine, but keep that helper in the test file so it can be reused across tests without leaking test code into production source.

Comment thread src/splitInteger.test.js
test('should add zeros if value < numberOfParts', () => {
const result = splitInteger(3, 5);
expect(result).toEqual([0, 0, 1, 1, 1]);
checkInvariants(result, 3, 5);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: You call checkInvariants(result, 3, 5) here but checkInvariants is not defined in this test file. This will throw a ReferenceError when the tests run. Test helpers that call Jest expect must live in the test file (or be exported from a test utility), not inside the implementation module. Move the helper into this test file or replace this call with inline assertions (length, sum, sorted ascending, and max-min <= 1).

Comment thread src/splitInteger.test.js
Comment on lines 19 to +27
test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);
expect(splitInteger(1, 4)).toEqual([0, 0, 0, 1]);
});
test('should add zeros if value < numberOfParts', () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Duplicate test: There are two tests with the same name ('should add zeros if value < numberOfParts') and overlapping assertions. The first (lines 23–26) already asserts the expected arrays; the second (lines 27–31) repeats the same expectation and calls the missing helper. Consolidate these into one test that both asserts the explicit expected array and checks invariants (either inline or via a helper defined in this file).

Comment thread src/splitInteger.test.js Outdated

test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: The expectation for splitInteger(3, 5) is correct ([0, 0, 1, 1, 1]). This matches the task requirement that the difference between max and min is <= 1 and is consistent with the implementation behavior — well fixed compared to the earlier incorrect expectation.

Comment thread src/splitInteger.test.js Outdated
test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);
expect(splitInteger(1, 4)).toEqual([0, 0, 0, 1]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: The test for splitInteger(1, 4) correctly expects [0, 0, 0, 1], covering the case where value < numberOfParts.

Comment thread src/splitInteger.test.js Outdated
Comment on lines +18 to +20
const result = splitInteger(17, 4); // → [4, 4, 4, 5]
expect(result).toEqual([4, 4, 4, 5]);
expect(result).toEqual(result.slice().sort((a, b) => a - b));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: The non-divisible case splitInteger(17, 4) asserts the exact expected array ([4, 4, 4, 5]) and also verifies that the result is sorted. This aligns with the description and the checklist requirement to check ordering.

Comment thread src/splitInteger.test.js Outdated
Comment on lines +13 to +29
expect(splitInteger(8, 1)).toEqual([8]);
expect(splitInteger(100, 1)).toEqual([100]);
});

test('should sort parts ascending if they are not equal', () => {

const result = splitInteger(17, 4); // → [4, 4, 4, 5]
expect(result).toEqual([4, 4, 4, 5]);
expect(result).toEqual(result.slice().sort((a, b) => a - b));
});

test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);
expect(splitInteger(1, 4)).toEqual([0, 0, 0, 1]);
});
test('should add zeros if value < numberOfParts', () => {
const result = splitInteger(3, 5);
expect(result).toEqual([0, 0, 1, 1, 1]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: After moving/defining the checkInvariants helper here, use it across the relevant tests (divisible, non-divisible, and value < numberOfParts) to consistently assert the invariants (length equals parts, sum equals value, sorted ascending, and max - min <= 1). That will make the suite DRY and easier to maintain.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Educational approval: moving you forward while highlighting a couple of required test updates to fully meet the checklist.

What you did well:

  • Strong checkInvariants helper verifying length, sum, sorted order, and max - min <= 1 across tests.
  • Good coverage of core examples like splitInteger(8, 1) and splitInteger(17, 4), plus the edge case where value < numberOfParts.

What needs improvement (actionable):

  • At src/splitInteger.test.js, inside checkInvariants (near the other assertions), add the integer-type check per the checklist: expect(result.every(Number.isInteger)).toBe(true); This ensures no fractional values can pass.
  • Add two missing example tests required by the task description:
    • splitInteger(6, 2) should equal [3, 3]; then call checkInvariants(result, 6, 2).
    • splitInteger(32, 6) should equal [5, 5, 5, 5, 6, 6]; then call checkInvariants(result, 32, 6).

Nice-to-have:

  • Consider a small table-driven loop over several (value, parts) pairs to reuse checkInvariants and increase robustness.

Great progress—these small additions will complete the checklist and further strengthen your test suite.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/splitInteger.test.js
function checkInvariants(result, value, numberOfParts) {
// length must equal numberOfParts
expect(result.length).toBe(numberOfParts);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add an explicit assertion that every element in the returned array is an integer. The task checklist requires verifying the output is an array of integers. A simple assertion to include inside checkInvariants (near this spot) is: expect(result.every(Number.isInteger)).toBe(true);. This guarantees no non-integer values slip through the implementation.

Comment thread src/splitInteger.test.js

const result = splitInteger(20, 5);
expect(result).toEqual([4, 4, 4, 4, 4]);
checkInvariants(result, 20, 5);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add a test that covers the example splitInteger(6, 2) with expected result [3, 3]. The task description requires that example to be present in the test suite. Place it near other example tests and call checkInvariants(result, 6, 2) to reuse invariant checks.

Comment thread src/splitInteger.test.js

const result = splitInteger(3, 5);
expect(result).toEqual([0, 0, 1, 1, 1]); // fixed expectation
checkInvariants(result, 3, 5);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add a test for the example splitInteger(32, 6) with expected result [5, 5, 5, 5, 6, 6]. Also consider adding a small table-driven or randomized test here that iterates several (value, numberOfParts) pairs and asserts all invariants via checkInvariants to increase coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants