ifElse – dodanie testów dla wszystkich scenariuszy działania funkcji by JoAnna578 · Pull Request #222 · mate-academy/qa_if-else-function

JoAnna578 · 2025-10-13T09:34:36Z

Dodano komplet testów do funkcji ifElse, sprawdzających wywołanie callbacków w zależności od wartości warunku oraz brak zwracanej wartości. Testy obejmują przypadki true, false, dynamiczne warunki i poprawność działania funkcji zgodnie ze specyfikacją.

Wprowadzono główną logikę funkcji ifElse zgodnie ze specyfikacją. Poprawiono eksport modułu, aby pasował do formatu importu w testach (module.exports = ifElse). Funkcja teraz poprawnie wywołuje pierwszy lub drugi callback w zależności od wyniku funkcji warunkowej.

Dodano komplet testów do funkcji ifElse, sprawdzających wywołanie callbacków w zależności od wartości warunku oraz brak zwracanej wartości. Testy obejmują przypadki true, false, dynamiczne warunki i poprawność działania funkcji zgodnie ze specyfikacją.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are critical blockers that will prevent the tests from running and from validating the required behavior.

Blocking export/import mismatch:
- In src/ifElse.js, you export the function directly (module.exports = ifElse;), but in src/ifElse.test.js line 3 you import using destructuring (const { ifElse } = require('./ifElse');). This results in ifElse being undefined at runtime. Fix by either exporting a named object (module.exports = { ifElse };) or changing the import to const ifElse = require('./ifElse');.
Tests not asserting required call counts/args:
- The requirements state the condition must be invoked exactly once and without arguments, and that only one of the callbacks is invoked exactly once and without arguments. Your tests currently use plain functions for conditionTrue/conditionFalse, so Jest cannot verify call counts/arguments. Convert them to mocks: conditionTrue = jest.fn(() => true); and conditionFalse = jest.fn(() => false);.
- Replace loose assertions like toHaveBeenCalled() with precise checks: toHaveBeenCalledTimes(1) and toHaveBeenCalledWith() for the condition and the called callback, and ensure the non-called callback uses not.toHaveBeenCalled() or toHaveBeenCalledTimes(0).

The core implementation logic looks solid—calling condition() and then first() or second() with no return—so once the export shape and test assertions are corrected, this should pass. Nice work on covering true/false/dynamic cases and verifying the function returns undefined.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.js

 }

-module.exports = { ifElse };
+module.exports = ifElse;


Export mismatch (blocking): The tests import using destructuring: const { ifElse } = require('./ifElse'); which expects the module to export an object with an ifElse property. Here you export the function directly (module.exports = ifElse;), so ifElse will be undefined in the tests. Update the export to a named export object, for example:

module.exports = { ifElse };

or change the tests to const ifElse = require('./ifElse'); so the import matches the current export. This must be fixed for the tests to run.

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

@@ -1,11 +1,46 @@
 'use strict';

+const { ifElse } = require('./ifElse');


Import/export mismatch: const { ifElse } = require('./ifElse'); expects a named export (an object with an ifElse property). The implementation exports the function directly (module.exports = ifElse), so this destructuring will result in ifElse being undefined and the tests will fail to run. Fix either by changing the import to const ifElse = require('./ifElse'); or by exporting an object from the implementation (module.exports = { ifElse };).

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

+  beforeEach(() => {
+    firstMock = jest.fn();
+    secondMock = jest.fn();
+    conditionTrue = () => true;


Make conditionTrue a mock so you can assert it was called exactly once and with no arguments. The task requires verifying the condition is invoked exactly once and without arguments; plain functions (() => true) cannot be inspected by Jest for call counts/args. Change to something like: conditionTrue = jest.fn(() => true);

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

+    firstMock = jest.fn();
+    secondMock = jest.fn();
+    conditionTrue = () => true;
+    conditionFalse = () => false;


Make conditionFalse a mock for the same reason as above. Change to something like: conditionFalse = jest.fn(() => false); so tests can assert it was called exactly once and with no args in the false case.

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

+  });
+
+  it('should call first callback if condition returns true', () => {
+    ifElse(conditionTrue, firstMock, secondMock);


In the "should call first callback if condition returns true" test you should assert the condition was called exactly once and with no arguments (requirement: "condition is invoked exactly once and without arguments"). After converting conditionTrue to a mock, add expect(conditionTrue).toHaveBeenCalledTimes(1); and expect(conditionTrue).toHaveBeenCalledWith(); before asserting which branch ran.

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

+  it('should call first callback if condition returns true', () => {
+    ifElse(conditionTrue, firstMock, secondMock);
+
+    expect(firstMock).toHaveBeenCalled();


Update these assertions to check exact call count and that firstMock was called without arguments. The task requires first be called exactly once and with no arguments when condition returns true. Replace expect(firstMock).toHaveBeenCalled(); with:

expect(firstMock).toHaveBeenCalledTimes(1);

expect(firstMock).toHaveBeenCalledWith();
Keep the assertion that secondMock was not called (or use toHaveBeenCalledTimes(0)).

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js


-  it('should ', () => {
+  it('should call second callback if condition returns false', () => {
+    ifElse(conditionFalse, firstMock, secondMock);


In the "should call second callback if condition returns false" test you likewise need to assert the condition was called exactly once and with no args. After converting conditionFalse to a mock, add expect(conditionFalse).toHaveBeenCalledTimes(1); and expect(conditionFalse).toHaveBeenCalledWith();.

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

+  it('should call second callback if condition returns false', () => {
+    ifElse(conditionFalse, firstMock, secondMock);

+    expect(secondMock).toHaveBeenCalled();


Update the assertions for the false case to verify secondMock was called exactly once and without arguments. Replace expect(secondMock).toHaveBeenCalled(); with:

expect(secondMock).toHaveBeenCalledTimes(1);

expect(secondMock).toHaveBeenCalledWith();
And keep/replace expect(firstMock).not.toHaveBeenCalled(); with expect(firstMock).toHaveBeenCalledTimes(0); if you want an explicit count assertion.

mateacademy-ai-mentor · 2025-10-13T09:38:09Z

src/ifElse.test.js

+    const dynamicCondition = jest.fn(() => true);
+    ifElse(dynamicCondition, firstMock, secondMock);
+
+    expect(dynamicCondition).toHaveBeenCalled();


Dynamic condition test: you already use jest.fn(() => true) for dynamicCondition which is good, but the assertions are loose. Assert the condition was called exactly once and with no args, and assert firstMock was called exactly once and with no args. Replace toHaveBeenCalled() calls with toHaveBeenCalledTimes(1) and add toHaveBeenCalledWith() where appropriate. Also ensure secondMock is asserted as not called (not.toHaveBeenCalled() or toHaveBeenCalledTimes(0)).

mateacademy-ai-mentor · 2025-10-13T09:38:10Z

src/ifElse.test.js

+
+  it('should not return any value', () => {
+    const result = ifElse(conditionTrue, firstMock, secondMock);
+    expect(result).toBeUndefined();


The test checking that the function returns no value (expect(result).toBeUndefined();) already addresses the requirement that ifElse does not return a value. Keep this assertion as-is.

Zmiana eksportu z module.exports = ifElse na module.exports = { ifElse } umożliwia poprawną destrukturyzację w pliku testowym (const { ifElse } = require('./ifElse')) i usuwa błąd „TypeError: ifElse is not a function”.

mateacademy-ai-mentor

Review Summary

I am requesting changes because the tests do not fully enforce the required behavior for call counts and arguments, which is a core checklist item. The implementation of ifElse is correct, but in src/ifElse.test.js the conditionTrue/conditionFalse in beforeEach are plain functions rather than mocks, preventing assertions on invocation counts and arguments. Please change them to mocks: conditionTrue = jest.fn(() => true) and conditionFalse = jest.fn(() => false).

Critical fixes needed:

In the “true branch” test: assert expect(conditionTrue).toHaveBeenCalledTimes(1) and toHaveBeenCalledWith(), and similarly expect(firstMock).toHaveBeenCalledTimes(1) and toHaveBeenCalledWith(), while keeping expect(secondMock).not.toHaveBeenCalled().
In the “false branch” test: assert expect(conditionFalse).toHaveBeenCalledTimes(1) and toHaveBeenCalledWith(), expect(secondMock).toHaveBeenCalledTimes(1) and toHaveBeenCalledWith(), and keep expect(firstMock).not.toHaveBeenCalled().
In the dynamic condition test: tighten assertions with toHaveBeenCalledTimes(1) and toHaveBeenCalledWith() for dynamicCondition and the selected callback.
In the “should not return any value” test: use a mocked condition and assert it was called exactly once.

What you did well: the function implementation is correct, exports align with the import, and the tests cover both branches and undefined return. Tightening these assertions will make the suite robust and fully compliant. Keep going—this is close!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-10-13T09:44:32Z

src/ifElse.test.js

+    firstMock = jest.fn();
+    secondMock = jest.fn();
+    conditionTrue = () => true;
+    conditionFalse = () => false;


In beforeEach you set conditionTrue and conditionFalse to plain functions. The checklist requires using Jest mocks/spies so you can assert call counts and arguments. Replace these with mocks, for example:
conditionTrue = jest.fn(() => true) and conditionFalse = jest.fn(() => false).
This change lets you assert toHaveBeenCalledTimes(1) and toHaveBeenCalledWith() for the condition (required behavior).

mateacademy-ai-mentor · 2025-10-13T09:44:32Z

src/ifElse.test.js

+    expect(firstMock).toHaveBeenCalled();
+    expect(secondMock).not.toHaveBeenCalled();


The 'should call first callback if condition returns true' test only checks that firstMock was called and secondMock was not. Per the checklist you must also assert that the condition mock was called exactly once and with no arguments, and that firstMock was called exactly once and with no arguments. Replace loose assertions with precise ones, e.g.:
expect(conditionTrue).toHaveBeenCalledTimes(1)
expect(conditionTrue).toHaveBeenCalledWith()
expect(firstMock).toHaveBeenCalledTimes(1)
expect(firstMock).toHaveBeenCalledWith()
Keep expect(secondMock).not.toHaveBeenCalled() to ensure the other callback was not invoked.

mateacademy-ai-mentor · 2025-10-13T09:44:32Z

src/ifElse.test.js

+    expect(secondMock).toHaveBeenCalled();
+    expect(firstMock).not.toHaveBeenCalled();


The 'should call second callback if condition returns false' test also uses loose assertions. Make the same upgrades as above: ensure the condition mock was called exactly once with no args, ensure secondMock was called exactly once with no args, and assert firstMock was not called. Use toHaveBeenCalledTimes(1) and toHaveBeenCalledWith() for the positive checks.

mateacademy-ai-mentor · 2025-10-13T09:44:32Z

src/ifElse.test.js

+    expect(dynamicCondition).toHaveBeenCalled();
+    expect(firstMock).toHaveBeenCalled();
+    expect(secondMock).not.toHaveBeenCalled();


The dynamic condition test correctly uses jest.fn() for dynamicCondition, which is good. However its assertions are loose. Change them to exact checks:
expect(dynamicCondition).toHaveBeenCalledTimes(1) and expect(dynamicCondition).toHaveBeenCalledWith();
expect(firstMock).toHaveBeenCalledTimes(1) and expect(firstMock).toHaveBeenCalledWith();
and keep expect(secondMock).not.toHaveBeenCalled().
These confirm exactly one invocation and that no arguments were passed (checklist).

mateacademy-ai-mentor · 2025-10-13T09:44:32Z

src/ifElse.test.js

+    const result = ifElse(conditionTrue, firstMock, secondMock);
+    expect(result).toBeUndefined();


The 'should not return any value' test asserts the return is undefined which meets the return requirement. To make this test stronger, call ifElse with a mocked condition (e.g. jest.fn(() => true)) and also assert the condition was called exactly once. Right now conditionTrue is a plain function (see beforeEach); if you change it to a mock as suggested, this test will also be able to assert the condition call count.

mateacademy-ai-mentor · 2025-10-13T09:44:32Z

src/ifElse.test.js

@@ -1,11 +1,46 @@
 'use strict';

+const { ifElse } = require('./ifElse');


Import line uses destructuring (const { ifElse } = require('./ifElse');) which matches the current implementation export module.exports = { ifElse } in src/ifElse.js. No change needed to the import/export shape.

Zmieniono conditionTrue i conditionFalse na mocki (jest.fn) w beforeEach, aby możliwe było sprawdzenie liczby wywołań i argumentów. Każdy test teraz używa toHaveBeenCalledTimes i toHaveBeenCalledWith dla callbacków i warunków, a niewywołany callback jest sprawdzany przez not.toHaveBeenCalled(). Test 'should not return any value' również weryfikuje liczbę wywołań condition. Kompletna poprawka zgodna z checklistą AI.

mateacademy-ai-mentor

Review Summary

I am approving your solution. The implementation in src/ifElse.js is clean and correctly calls condition without arguments, then invokes either first or second, and returns undefined as required. In src/ifElse.test.js, you verify the true/false branches, argument-less calls, and undefined return.

Educational override: To help you progress, I’m approving while noting one improvement area in tests.

Critical improvement: Add call-order assertions to ensure condition runs before the selected branch (requirement: call order). For example in the true-branch test (around the existing assertions), assert expect(conditionTrue.mock.invocationCallOrder[0]).toBeLessThan(firstMock.mock.invocationCallOrder[0]);. Similarly, in the false-branch test, add expect(conditionFalse.mock.invocationCallOrder[0]).toBeLessThan(secondMock.mock.invocationCallOrder[0]);, and in the dynamic-condition test, expect(dynamicCondition.mock.invocationCallOrder[0]).toBeLessThan(firstMock.mock.invocationCallOrder[0]);.
Minor improvement: In the “should not return any value” test, also assert the condition was called with no args: expect(conditionTrue).toHaveBeenCalledWith().

Great job on the clear implementation and solid baseline tests—these small additions will make your tests fully enforce the required behavior.

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

Rate AI review example ↩

mateacademy-ai-mentor · 2025-10-13T10:07:06Z

src/ifElse.test.js

+
+    expect(firstMock).toHaveBeenCalledTimes(1);
+    expect(firstMock).toHaveBeenCalledWith();
+


Add an assertion that conditionTrue was called before firstMock in the true-branch test. The task requires that condition is invoked before deciding which branch to execute (call order). One approach using Jest's mock internals is to compare invocationCallOrder values. Example to add (after the existing assertions for conditionTrue and firstMock):

// ensure condition ran before first expect(conditionTrue.mock.invocationCallOrder[0]).toBeLessThan(firstMock.mock.invocationCallOrder[0]);

Place this near the true-branch assertions so the test verifies ordering as well as call counts/arguments.

mateacademy-ai-mentor · 2025-10-13T10:07:07Z

src/ifElse.test.js

+    expect(conditionFalse).toHaveBeenCalledWith();

-  it('should ', () => {
+    expect(secondMock).toHaveBeenCalledTimes(1);


In the false-branch test, add an assertion that conditionFalse was called before secondMock. The implementation detail requires condition to be executed prior to the selected branch. You can use the same invocationCallOrder technique. Example to add (before or after the secondMock assertions):

expect(conditionFalse.mock.invocationCallOrder[0]).toBeLessThan(secondMock.mock.invocationCallOrder[0]);

This will ensure the test verifies the required call order for the false branch.

mateacademy-ai-mentor · 2025-10-13T10:07:07Z

src/ifElse.test.js

+
+    expect(dynamicCondition).toHaveBeenCalledTimes(1);
+    expect(dynamicCondition).toHaveBeenCalledWith();
+


For the dynamic-condition test, add the same ordering assertion to ensure dynamicCondition runs before firstMock (since the dynamic condition returns true in this test). Example:

expect(dynamicCondition.mock.invocationCallOrder[0]).toBeLessThan(firstMock.mock.invocationCallOrder[0]);

This confirms the implementation consistently calls the condition before the chosen branch even for dynamic functions.

mateacademy-ai-mentor · 2025-10-13T10:07:07Z

src/ifElse.test.js

+    const result = ifElse(conditionTrue, firstMock, secondMock);
+
+    expect(result).toBeUndefined();
+    expect(conditionTrue).toHaveBeenCalledTimes(1);


In the "should not return any value" test, you already check result is undefined and that conditionTrue was called once. Also assert that conditionTrue was called with no arguments to fully satisfy the requirement that callbacks are invoked without arguments. Add:

expect(conditionTrue).toHaveBeenCalledWith();

This strengthens the test to verify both call count and that no args were passed to condition when ifElse is invoked.

JoAnna578 added 2 commits October 13, 2025 11:30

mateacademy-ai-mentor suggested changes Oct 13, 2025

View reviewed changes

JoAnna578 requested a review from mateacademy-ai-mentor October 13, 2025 09:41

mateacademy-ai-mentor suggested changes Oct 13, 2025

View reviewed changes

JoAnna578 requested a review from mateacademy-ai-mentor October 13, 2025 10:04

mateacademy-ai-mentor approved these changes Oct 13, 2025

View reviewed changes

		@@ -1,11 +1,46 @@
		'use strict';

		const { ifElse } = require('./ifElse');

		expect(firstMock).toHaveBeenCalled();
		expect(secondMock).not.toHaveBeenCalled();

		expect(secondMock).toHaveBeenCalled();
		expect(firstMock).not.toHaveBeenCalled();

		const result = ifElse(conditionTrue, firstMock, secondMock);
		expect(result).toBeUndefined();


		expect(firstMock).toHaveBeenCalledTimes(1);
		expect(firstMock).toHaveBeenCalledWith();


		expect(dynamicCondition).toHaveBeenCalledTimes(1);
		expect(dynamicCondition).toHaveBeenCalledWith();

Conversation

JoAnna578 commented Oct 13, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants