Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: add support for chainable mockImplementationOnce #49088

16 changes: 14 additions & 2 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -1318,13 +1318,18 @@ test('changes a mock behavior', (t) => {
added:
- v19.1.0
- v18.13.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49088
description: this now return the context as well as appending when not providing the onCall argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: this now return the context as well as appending when not providing the onCall argument.
description: This now supports chaining calls.

Copy link
Member Author

@rluvaton rluvaton Aug 12, 2023

Choose a reason for hiding this comment

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

I wanted to emphasize that it append the implementation as it wasn't before, and instead it just replaced it when mocking multiple times before calling the function

-->

* `implementation` {Function|AsyncFunction} The function to be used as the
mock's implementation for the invocation number specified by `onCall`.
* `onCall` {integer} The invocation number that will use `implementation`. If
the specified invocation has already occurred then an exception is thrown.
**Default:** The number of the next invocation.
**Default:** The number of the next invocation than is not mocked by `mockImplementationOnce`.
rluvaton marked this conversation as resolved.
Show resolved Hide resolved
* Returns: {MockFunctionContext} the context to allow chaining.
rluvaton marked this conversation as resolved.
Show resolved Hide resolved

This function is used to change the behavior of an existing mock for a single
invocation. Once invocation `onCall` has occurred, the mock will revert to
Expand Down Expand Up @@ -1352,8 +1357,15 @@ test('changes a mock behavior once', (t) => {
const fn = t.mock.fn(addOne);
rluvaton marked this conversation as resolved.
Show resolved Hide resolved

assert.strictEqual(fn(), 1);
fn.mock.mockImplementationOnce(addTwo);
assert.strictEqual(fn(), 2);
rluvaton marked this conversation as resolved.
Show resolved Hide resolved
fn.mock
.mockImplementationOnce(() => 17)
.mockImplementationOnce(() => 77, 5)
.mockImplementationOnce(() => 42);
assert.strictEqual(fn(), 17);
assert.strictEqual(fn(), 42);
assert.strictEqual(fn(), 3);
assert.strictEqual(fn(), 77);
assert.strictEqual(fn(), 4);
});
```
Expand Down
20 changes: 17 additions & 3 deletions lib/internal/test_runner/mock/mock.js
rluvaton marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,23 @@ class MockFunctionContext {
mockImplementationOnce(implementation, onCall) {
validateFunction(implementation, 'implementation');
const nextCall = this.#calls.length;
const call = onCall ?? nextCall;
validateInteger(call, 'onCall', nextCall);
this.#mocks.set(call, implementation);
if (onCall) {
cjihrig marked this conversation as resolved.
Show resolved Hide resolved
validateInteger(onCall, 'onCall', nextCall);
this.#mocks.set(onCall, implementation);

return this;
}

const mocksSize = this.#mocks.size;
for (let i = 0; i < mocksSize; i++) {
if (!this.#mocks.has(i + nextCall)) {
this.#mocks.set(i + nextCall, implementation);
return this;
}
}
Comment on lines +72 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is right either (although I haven't tested this variation). I think you want a counter, and you want to loop over all of the existing entries in this.#mocks to see if each entry should be included in the counter or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

This solution find the first empty implementation when you go incrementally, and then put it there

I don't see a problem with this and I would appreciate if you have a concrete example 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change:

test('foo', (t) => {
  let cnt = 0;
  function addOne() {
    cnt++;
    return cnt;
  }

  const fn = t.mock.fn(addOne);

  fn.mock.mockImplementationOnce(() => 999, 0);
  fn.mock.mockImplementationOnce(() => 888, 1);
  fn.mock.mockImplementationOnce(() => 777);

  console.log(fn());
  console.log(fn());
  console.log(fn());
});

This currently prints 777, 888, 1. With the changes in this PR, it now prints 999, 888, 777 (note that I'm not even using chaining here).

Copy link
Member Author

@rluvaton rluvaton Aug 12, 2023

Choose a reason for hiding this comment

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

I know this is a breaking change this is why I wrote that in the PR description, what do you propose?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't notice that you had updated the PR description recently.

I'm not sure if chaining can be implemented without a breaking change. I personally prefer the simplicity of mockImplementationOnce(() => 777) always meaning "mock the next call" to now needing to mentally keep track of which calls are mocked in order to support chaining.

Copy link
Member Author

@rluvaton rluvaton Aug 12, 2023

Choose a reason for hiding this comment

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

I think it can be implemented (only adding chaining) but I think it's gonna be unintuitive DX...

Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change:

test('foo', (t) => {
  let cnt = 0;
  function addOne() {
    cnt++;
    return cnt;
  }

  const fn = t.mock.fn(addOne);

  fn.mock.mockImplementationOnce(() => 999, 0);
  fn.mock.mockImplementationOnce(() => 888, 1);
  fn.mock.mockImplementationOnce(() => 777);

  console.log(fn());
  console.log(fn());
  console.log(fn());
});

This currently prints 777, 888, 1. With the changes in this PR, it now prints 999, 888, 777 (note that I'm not even using chaining here).

I got that it's a breaking change but this behavior doesn't look right.

I love the chaining possibility and I believe it's a great addition!

In the algorithm, shouldn't we just enqueue all mockImpl so we know the exact order?

Copy link
Member Author

Choose a reason for hiding this comment

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

we do "enqueue" it by putting that in the next available location

Copy link
Member

Choose a reason for hiding this comment

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

we do "enqueue" it by putting that in the next available location

hmm, why is the order reversed on @cjihrig 's example? is it because of the second param?

Copy link
Member Author

Choose a reason for hiding this comment

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

to show the breaking change

this.#mocks.set(mocksSize + nextCall, implementation);

return this;
}

restore() {
Expand Down
49 changes: 49 additions & 0 deletions test/parallel/test-runner-mocking.js
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,55 @@ test('mock implementation can be changed dynamically', (t) => {
assert.strictEqual(fn.mock.callCount(), 12);
});

rluvaton marked this conversation as resolved.
Show resolved Hide resolved
test('mockImplementationOnce chaining', (t) => {
let cnt = 0;
function addOne() {
cnt++;
return cnt;
}

const fn = t.mock.fn(addOne);
assert.strictEqual(fn(), 1);
assert.strictEqual(fn(), 2);
fn.mock
.mockImplementationOnce(() => 77, 5)
.mockImplementationOnce(() => 17)
.mockImplementationOnce(() => 42);
assert.strictEqual(fn(), 17);
assert.strictEqual(fn(), 42);
assert.strictEqual(fn(), 3);
assert.strictEqual(fn(), 77);
assert.strictEqual(fn(), 4);
assert.strictEqual(fn(), 5);

fn.mock.mockImplementationOnce(() => 52);
assert.strictEqual(fn(), 52);
assert.strictEqual(fn(), 6);
});

test('mockImplementationOnce chaining with default value', (t) => {
let cnt = 0;
function addOne() {
cnt++;
return cnt;
}

const fn = t.mock.fn(addOne);
assert.strictEqual(fn(), 1);
fn.mock
.mockImplementationOnce(() => 17)
.mockImplementationOnce(() => 42)
.mockImplementation(() => 99);
assert.strictEqual(fn(), 17);
assert.strictEqual(fn(), 42);
assert.strictEqual(fn(), 99);
assert.strictEqual(fn(), 99);

fn.mock.mockImplementationOnce(() => 52);
assert.strictEqual(fn(), 52);
assert.strictEqual(fn(), 99);
});

test('local mocks are auto restored after the test finishes', async (t) => {
const obj = {
foo() {},
Expand Down
1 change: 1 addition & 0 deletions typings/primordials.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ declare namespace primordials {
export const ArrayPrototypeLastIndexOf: UncurryThis<typeof Array.prototype.lastIndexOf>
export const ArrayPrototypePop: UncurryThis<typeof Array.prototype.pop>
export const ArrayPrototypePush: UncurryThis<typeof Array.prototype.push>
export const ArrayPrototypeAt: UncurryThis<typeof Array.prototype.at>
export const ArrayPrototypePushApply: UncurryThisStaticApply<typeof Array.prototype.push>
export const ArrayPrototypeReverse: UncurryThis<typeof Array.prototype.reverse>
export const ArrayPrototypeShift: UncurryThis<typeof Array.prototype.shift>
Expand Down