test_runner: mock.mockImplementationOnce only works for the last call

[ErickWendel]

Version

v21.0.0-pre

Platform

Darwin MacBook-Pro-4.local 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:19:52 PDT 2022; root:xnu-8020.140.49~2/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

const { describe, it, mock } = require('node:test')
const { strictEqual } = require('node:assert')

describe('my test', () => {
	it('hello world', () => {
		const instance = { sum: (arg1, arg2) => arg1 + arg2 }
		const m = mock.method(
			instance,
			instance.sum.name
		)
		m.mock.mockImplementationOnce(() => 1)
		m.mock.mockImplementationOnce(() => 2)

		strictEqual(instance.sum(1, 1), 1)
		strictEqual(instance.sum(5, 1), 2)
		strictEqual(instance.sum(2, 1), 3)
	})
})

./node --test erick-test/index.test.js
▶ my test
  ✖ hello world (1.817708ms)
    AssertionError: Expected values to be strictly equal:
    
    2 !== 1
    
        at TestContext.<anonymous> (/Users/erickwendel/Downloads/projetos/node/erick-test/index.test.js:14:3)
        at Test.runInAsyncScope (node:async_hooks:206:9)
        at Test.run (node:internal/test_runner/test:568:25)
        at Test.start (node:internal/test_runner/test:481:17)
        at node:internal/test_runner/test:799:71
        at node:internal/per_context/primordials:482:82
        at new Promise (<anonymous>)
        at new SafePromise (node:internal/per_context/primordials:450:29)
        at node:internal/per_context/primordials:482:9
        at Array.map (<anonymous>) {
      generatedMessage: false,
      code: 'ERR_ASSERTION',
      actual: 2,
      expected: 1,
      operator: 'strictEqual'
    }

▶ my test (2.8215ms)

ℹ tests 1
ℹ suites 1
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 75.543917

✖ failing tests:

✖ hello world (1.817708ms)
  AssertionError: Expected values to be strictly equal:
  
  2 !== 1
  
      at TestContext.<anonymous> (/Users/erickwendel/Downloads/projetos/node/erick-test/index.test.js:14:3)
      at Test.runInAsyncScope (node:async_hooks:206:9)
      at Test.run (node:internal/test_runner/test:568:25)
      at Test.start (node:internal/test_runner/test:481:17)
      at node:internal/test_runner/test:799:71
      at node:internal/per_context/primordials:482:82
      at new Promise (<anonymous>)
      at new SafePromise (node:internal/per_context/primordials:450:29)
      at node:internal/per_context/primordials:482:9
      at Array.map (<anonymous>) {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: 2,
    expected: 1,
    operator: 'strictEqual'
  }

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

mock.mockImplementationOnce should be used to mock each individual call.

given

mock.mockImplementationOnce(() => 1)
mock.mockImplementationOnce(() => 2)
mock.mockImplementationOnce(() => 3)

Each call result should be returned given the order configured by the mockImplementation Once

What do you see instead?

Only the last mock.mockImplementationOnce is applied.

Additional information

I'd also enable sequence calls like:

mock
.mockImplementationOnce(() => 1)
.mockImplementationOnce(() => 2)
.mockImplementationOnce(() => 3)

@nodejs/test_runner

[cjihrig]

I think this is working as intended/documented. mockImplementationOnce() by default only changes the behavior of the next invocation. There is an option second parameter, onCall, that allows you to change that default behavior.

[ErickWendel]

I think this is working as intended/documented. mockImplementationOnce() by default only changes the behavior of the next invocation. There is an option second parameter, onCall, that allows you to change that default behavior.

Hmm got it! Why don't put an internal counter to avoid having this:

m.mock.mockImplementationOnce(() => 1, 0)
m.mock.mockImplementationOnce(() => 2, 1)

to this

m.mock.mockImplementationOnce(() => 1)
m.mock.mockImplementationOnce(() => 2)

and even enable nested calls such as:

m.mock
    .mockImplementationOnce(() => 1)
    .mockImplementationOnce(() => 2)

Other mocking libraries such as Jest work like this so I think for dev experience would be best to follow the same idea.

WDYT?

[cjihrig]

I'm not opposed if someone can make it work well, but there are edge cases that will need to be addressed. I don't think it's as straightforward as incrementing a counter when mockImplementationOnce() is called:

const m = mock.method(...);
m(); // Original mock provided by mock.method()
m.mock.mockImplementationOnce(() => 1);
m.mock.mockImplementationOnce(() => 2);
m(); // Returns 1
m(); // Returns 2
m(); // Original mock provided by mock.method()
m(); // Original mock provided by mock.method()

// What should the next line do?
m.mock.mockImplementationOnce(() => 3);
m();

In this example, I would expect the final mockImplementationOnce() to mock the next invocation. If the counter were only incremented by calls to mockImplementationOnce() then the invocation would have already passed (which throws an exception). The proposed counter would need to be carefully managed.

Jest work like this so I think for dev experience would be best to follow the same idea

Side note: To be clear, just because Jest does something a particular way does not mean we should copy it.

[ErickWendel]

I'm not opposed if someone can make it work well, but there are edge cases that will need to be addressed. I don't think it's as straightforward as incrementing a counter when mockImplementationOnce() is called:

const m = mock.method(...);
m(); // Original mock provided by mock.method()
m.mock.mockImplementationOnce(() => 1);
m.mock.mockImplementationOnce(() => 2);
m(); // Returns 1
m(); // Returns 2
m(); // Original mock provided by mock.method()
m(); // Original mock provided by mock.method()

// What should the next line do?
m.mock.mockImplementationOnce(() => 3);
m();

In this example, I would expect the final mockImplementationOnce() to mock the next invocation. If the counter were only incremented by calls to mockImplementationOnce() then the invocation would have already passed (which throws an exception). The proposed counter would need to be carefully managed.

Jest work like this so I think for dev experience would be best to follow the same idea

Side note: To be clear, just because Jest does something a particular way does not mean we should copy it.

Nice I got it!

does not mean we should copy it

Yes, I agree with you. I only mention them as I like how the API works and thought it'd be easier to follow the same pattern.

I'm gonna try doing a PoC and open a draft PR for it so we could discuss along the way

[rluvaton]

Side note: possible implementation is having a queue for the mocks, this way this example would work as expected

const m = mock.method(...);
m(); // Original mock provided by mock.method()
m.mock.mockImplementationOnce(() => 1);
m.mock.mockImplementationOnce(() => 2);
m(); // Returns 1
m(); // Returns 2
m(); // Original mock provided by mock.method()
m(); // Original mock provided by mock.method()

// What should the next line do?
m.mock.mockImplementationOnce(() => 3);
m();

[rluvaton]

I think it's a valuable feature