test_runner: add support for chainable mockImplementationOnce

[rluvaton]

This also includes appending implementation rather than replacing when using mockImplementationOnce

Fixes: #47718

this is a breaking change I believe

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[cjihrig]

Can you please run the linter. The current diff is very hard to review.

[rluvaton]

Can you please run the linter. The current diff is very hard to review.

Yeah, this is why I set it to draft as WebStorm made some problems, this is now ready for review

[atlowChemi]

I think this PR should include docs updates @rluvaton

[cjihrig]

Suggested change

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

[rluvaton]

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

[cjihrig]

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.

[rluvaton]

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 😀

[cjihrig]

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).

[rluvaton]

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

[cjihrig]

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.

[rluvaton]

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

[ErickWendel]

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?

[rluvaton]

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

[ErickWendel]

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?

[rluvaton]

to show the breaking change

[cjihrig]

#47718 was closed. I propose closing this as well.

[rluvaton]

I think it's a valuable feature but closing for now...