Skip to content

all tests passed#1014

Open
emberlast1 wants to merge 3 commits into
mate-academy:mainfrom
emberlast1:develop
Open

all tests passed#1014
emberlast1 wants to merge 3 commits into
mate-academy:mainfrom
emberlast1:develop

Conversation

@emberlast1
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

Your implementation looks solid, but the test suite in src/splitInteger.test.js is missing several explicit requirements from the task description and checklist, which are critical blockers.

Critical issues to fix (with specifics and how to address):

  1. Missing required example tests (task requirement):

    • Add exact example tests using toEqual:
      • Line near your current splitInteger(10, 1) test: add expect(splitInteger(8, 1)).toEqual([8]).
      • Line near your splitInteger(10, 2) test: add expect(splitInteger(6, 2)).toEqual([3, 3]).
      • Add tests for: expect(splitInteger(17, 4)).toEqual([4, 4, 4, 5]) and expect(splitInteger(32, 6)).toEqual([5, 5, 5, 5, 6, 6]).
    • Why: The description explicitly lists these examples and they must be present in the tests.
  2. Missing property-based assertions (task checklist):

    • For multiple inputs (including the required examples), add assertions that verify the general contract:
      • expect(result).toHaveLength(numberOfParts)
      • expect(result.every(Number.isInteger)).toBe(true)
      • Sorted non-decreasing: loop to check result[i] <= result[i+1] for all i
      • expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1)
      • expect(result.reduce((a,b) => a + b, 0)).toBe(value)
    • How: Create a small helper (e.g., assertProperties(value, parts)) and call it inside tests for the examples and a few additional cases like (10,3) and (2,5).
    • Why: The checklist requires explicit property checks; relying only on exact arrays is insufficient.
  3. Test organization (quality, but helpful):

    • Wrap tests in describe blocks such as “examples,” “even splits,” “uneven splits,” and “more parts than value.”
    • Why: Improves readability and aligns with recommended structure.

Note: Do not add invalid-input tests; the task states inputs are always valid.

You’ve done well covering a mix of scenarios and using Jest correctly. Once you add the required example cases and the property-based assertions, the suite will meet the task requirements. Keep it up!


✨ 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 split a number into equal parts
if a value is divisible by a numberOfParts`, () => {
const result = splitInteger(10, 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.

Issue: This test uses splitInteger(10, 2) but the task description requires an example for splitInteger(6, 2) (expected [3, 3]). Add the required example test (6, 2) using toEqual([3, 3]) or include it as an additional assertion. The description explicitly lists specific example inputs that must be present in the tests.

Comment thread src/splitInteger.test.js Outdated

test(`should return a part equals to a value
when splitting into 1 part`, () => {
const result = splitInteger(10, 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.

Problem: The test here checks splitInteger(10, 1) but the required example in the description is splitInteger(8, 1) (expected [8]). Add a test for that exact example. Example-based checks should match the description.

Comment thread src/splitInteger.test.js Outdated
});

test('should sort parts ascending if they are not equal', () => {
const result = splitInteger(10, 3);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Observation: This test uses splitInteger(10, 3). The description lists additional example cases that are missing: splitInteger(17, 4) and splitInteger(32, 6). Add tests for those examples (toEqual with their exact expected arrays). Also, beyond exact examples, prefer adding property-based assertions for general constraints rather than relying only on a single exact distribution.

Comment thread src/splitInteger.test.js Outdated
});

test('should add zeros if value < numberOfParts', () => {
const result = splitInteger(2, 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.

Note: The "value < numberOfParts" test (2, 5) is fine to keep. However, you still need to add explicit property checks across tests: length equals numberOfParts, sum equals the original value, all items are integers, array sorted non-decreasing, and max-min <= 1. Add a test (or helper) that asserts these properties for multiple inputs (including the required examples). Do NOT add tests for invalid inputs (task states inputs are always valid).

Comment thread src/splitInteger.test.js Outdated
if a value is divisible by a numberOfParts`, () => {
const result = splitInteger(10, 2);

expect(result).toEqual([5, 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.

Missing general/property assertions: the test at this expect only asserts a specific array for a specific input. The checklist requires tests that assert the general properties of the result (length, integers, sorted order, max-min <= 1, sum equals value). Add assertions such as expect(result).toHaveLength(numberOfParts), expect(result.every(Number.isInteger)).toBeTruthy(), a sorted-order check, expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1), and expect(result.reduce((a,b)=>a+b,0)).toBe(value) in one or more tests.

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

I’m requesting changes because two checklist-level requirements are not met and CI is currently blocked.

Critical issues to fix:

  • ESLint max-len failure blocks CI from running tests. In src/splitInteger.test.js line 5 exceeds 80 chars. Shorten the test title (e.g., should have correct length, order and max diff <= 1) or add // eslint-disable-next-line max-len above that line so the job can proceed.
  • Missing required property-based assertions across tests. The checklist requires explicit assertions for length, integer-ness, sorted non-decreasing order, max-min <= 1, and sum for multiple inputs. Currently only the first test partially covers these; tests like splitInteger(6, 2), splitInteger(8, 1), splitInteger(17, 4), and splitInteger(2, 5) rely solely on toEqual(...).

How to fix (recommended):

  • Add a helper at the top of the test file:
    • function assertProperties(value, numberOfParts) { const result = splitInteger(value, numberOfParts); expect(result).toHaveLength(numberOfParts); expect(result.every(Number.isInteger)).toBe(true); for (let i = 0; i < result.length - 1; i++) { expect(result[i]).toBeLessThanOrEqual(result[i+1]); } expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1); expect(result.reduce((a,b)=>a+b,0)).toBe(value); return result; }
  • In each example test, call the helper and keep the exact-array toEqual where appropriate. For example, for splitInteger(6, 2) first call assertProperties(6, 2) then assert toEqual([3, 3]).

Minor (optional): group tests with describe blocks for readability. Once these changes are in and lint passes, this should meet the task requirements. Nice work including all example cases; adding the shared property checks will make the suite robust and compliant.


✨ 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
const splitInteger = require('./splitInteger');

test(`should have current length, sort order, and max difference between numbers should be <= 1`, () => {
const result = splitInteger(32, 6);
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: Add a small helper near the top of this test file that asserts the general properties for a given input. Example helper (place before the first test):

function assertProperties(value, numberOfParts) {
  const result = splitInteger(value, numberOfParts);
  expect(result).toHaveLength(numberOfParts);
  expect(result.every(Number.isInteger)).toBe(true);
  for (let i = 0; i < result.length - 1; i++) {
    expect(result[i]).toBeLessThanOrEqual(result[i + 1]);
  }
  expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1);
  expect(result.reduce((a, b) => a + b, 0)).toBe(value);
  return result;
}

Then call assertProperties(6, 2), assertProperties(8, 1), etc. This ensures the checklist property assertions are present consistently and reduces duplication. The task description requires these property checks across cases .

Comment thread src/splitInteger.test.js Outdated
Comment on lines +8 to +11
expect(result).toHaveLength(6);
expect(result).toEqual([5, 5, 5, 5, 6, 6]);
expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1);
expect(result.reduce((a, b) => a + b, 0)).toBe(32);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this first test you already assert length, exact array, max-min difference and sum (good). Add an explicit integer-ness check and an explicit sortedness assertion (adjacent comparisons) to make this test cover the full checklist. For example: expect(result.every(Number.isInteger)).toBe(true) and a loop asserting result[i] <= result[i+1]. The checklist requires those property assertions across inputs, not only exact equality .

Comment thread src/splitInteger.test.js Outdated
Comment on lines +16 to +18
const result = splitInteger(6, 2);

expect(result).toEqual([3, 3]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test for splitInteger(6, 2) currently only contains expect(result).toEqual([3, 3]). Add the general property assertions (length 2, integer-ness, sortedness, max-min <= 1, and sum equals 6) or call the helper suggested above. The checklist requires property-based checks for multiple inputs, not just exact-array assertions .

Comment thread src/splitInteger.test.js Outdated
Comment on lines +23 to +25
const result = splitInteger(8, 1);

expect(result).toEqual([8]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test for splitInteger(8, 1) only asserts toEqual([8]). Please add the general assertions (length, integer-ness, sorted order, max-min <= 1, and sum equals 8) or call the shared helper. These property checks are required by the task checklist for example cases .

Comment thread src/splitInteger.test.js Outdated
Comment on lines +29 to +31
const result = splitInteger(17, 4);

expect(result).toEqual([4, 4, 4, 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.

The splitInteger(17, 4) test currently asserts only the exact array. Add the property assertions (length 4, integer-ness, sorted non-decreasing, max-min <= 1, sum equals 17) or call the helper. Tests should verify both specific examples and the general contract across inputs .

Comment thread src/splitInteger.test.js Outdated
Comment on lines +35 to +37
const result = splitInteger(2, 5);

expect(result).toEqual([0, 0, 0, 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.

For the value < numberOfParts case (splitInteger(2, 5)) you assert the exact expected array which is good as an example. Still, add the property assertions (length 5, integer-ness, sortedness, max-min <= 1, and sum equals 2) or call the helper. The checklist expects property checks for edge-ish cases as well .

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 forward while highlighting a few improvements to strengthen your tests.

What you did well:

  • Centralized property checks in assertProperties (integers, length, ascending order, max-min <= 1, sum) — clean and effective.

What to improve:

  • In src/splitInteger.test.js, add an explicit array assertion before using array methods. For example, at the top of assertProperties (before result.every(...)), add: expect(Array.isArray(result)).toBe(true);. This meets the requirement to assert the return type and avoids .every throwing if result isn’t an array.
  • Ensure the Array.isArray check executes before result.every(Number.isInteger) (same file) to produce clear failures when the return type is wrong.
  • Add the missing example test for splitInteger(32, 6) and assert the exact array: expect(result).toEqual([5, 5, 5, 5, 6, 6]);.
  • Add the edge-case test where value === numberOfParts, e.g., splitInteger(5, 5) should yield [1, 1, 1, 1, 1].

Minor tip: if your test titles risk max-len lint errors, shorten them or place an // eslint-disable-next-line max-len above the long line. Great structure overall — you’re very close to fully meeting the checklist!


✨ 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 assertProperties(value, numberOfParts) {
const result = splitInteger(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.

Add an explicit check that the result is an array before using array methods. The task requires the function to return an array; currently you call result.every(...) which will throw if result is not an array. Add expect(Array.isArray(result)).toBe(true); before the every assertion.

Comment thread src/splitInteger.test.js
function assertProperties(value, numberOfParts) {
const result = splitInteger(value, numberOfParts);

expect(result.every(Number.isInteger)).toBe(true);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

result.every(Number.isInteger) is called before asserting the return type. If result is not an array this call will throw and the test won't give a clear failure about return type. Move or add the Array.isArray assertion so it runs before this line.

Comment thread src/splitInteger.test.js
test('should add zeros if value < numberOfParts', () => {
const result = assertProperties(2, 5);

expect(result).toEqual([0, 0, 0, 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.

Two tests are still missing from the checklist:

  1. The example splitInteger(32, 6) === [5, 5, 5, 5, 6, 6] — add a test that calls const result = assertProperties(32, 6); expect(result).toEqual([5, 5, 5, 5, 6, 6]);.

  2. The edge case where value === numberOfParts (array of ones). e.g. const result = assertProperties(5, 5); expect(result).toEqual([1, 1, 1, 1, 1]);.

Add these tests (after the existing ones) so all provided examples and required edge cases are covered.

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