MockInterceptor: `.reply` should be LIFO

[Dzieni]

This would solve...

Let's imagine a following test suite:

import assert from 'node:assert'
import {test} from 'node:test'
import {MockAgent, setGlobalDispatcher, fetch} from 'undici'

// the API fetch helper we want to test
const getFolder = async id => {
    const res = await fetch(`https://api.example.com/folders/${id}`)
    if (!res.ok) {
        // TODO: add retry handling
        throw new Error(`HTTP ${res.status} - ${res.statusText}`)
    }
    return await res.json()
}

// the mocked API
const mockAgent = new MockAgent()
mockAgent.disableNetConnect()
setGlobalDispatcher(mockAgent)

const mockPool = mockAgent.get('https://api.example.com')
mockPool
    .intercept({
        path: '/folders/123',
    })
    .reply(200, {id: 123, name: 'mocked folder'})
    .persist()
mockPool.intercept({path: '/folders/456'}).reply(404)

// tests
test('successful fetch', async () => {
    const folder = await getFolder(123)
    assert.deepEqual(folder, {id: 123, name: 'mocked folder'})
})

test('error handling', async () => {
    const error = await getFolder(456).catch(e => e)
    assert.equal(error.message, `HTTP 404 - Not Found`)
})

// should fail, since the retry handling is not implemented yet
test('retry handling', async () => {
    // let's simulate a temporary downtime
    mockPool
        .intercept({
            path: '/folders/123',
        })
        .reply(502)
        .delay(1000)
        .times(2)

    const folder = await getFolder(123)
    assert.deepEqual(folder, {id: 123, name: 'mocked folder'})
})

The retry handling test should fail, but it does not, because once you set a persistent interceptor, there is no way to override it or delete it. It's counterintuitive to the mocks known from the test runners, where you have methods like mockImplementationOnce:

import assert from 'node:assert'
import {test, mock} from 'node:test'

const foo = mock.fn(() => 'bar')

test('mocks', () => {
    assert.strictEqual(foo(), 'bar')

    foo.mock.mockImplementation(() => 'baz')
    assert.strictEqual(foo(), 'baz')

    foo.mock.mockImplementationOnce(() => 'qux')
    assert.strictEqual(foo(), 'qux')

    assert.strictEqual(foo(), 'baz')

    foo.mock.restore()
    assert.strictEqual(foo(), 'bar')
})

The implementation should look like...

I suppose that addMockDispatch should use .unshift instead of .push, so that new mocks have a top priority.

I have also considered...

...adding an equivalent of mock.restore() from Node test runner. Now I don't see any way to delete, temporarily disable, or unpersist the mock. It could be a part of the MockScope class.

[mcollina]

I think FIFO is the most correct.

The problem in your test is that you are not creating a new mock for each test.

[Dzieni]

My real-life use case is a series of integration tests that cover a broad range of API requests (a few dozen). I don't want to think which requests will exactly be executed in a certain test, so I'd rather just take a full API mock and add needed overrides.

While a workaround is indeed fairly easy:

const createMockAgent = afterInit => {
    const mockAgent = new MockAgent()
    mockAgent.disableNetConnect()
    const mockPool = mockAgent.get('https://api.example.com')

    afterInit?.(mockPool) // a hook for adding overrides

    // defaults
    mockPool
        .intercept({
            path: '/folders/123',
        })
        .reply(200, {id: 123, name: 'mocked folder'})
        .persist()
    mockPool.intercept({path: '/folders/456'}).reply(404).persist()

    return mockAgent
}

const mockAgent = createMockAgent(mockPool => {
    // let's simulate a temporary downtime
    mockPool
        .intercept({
            path: '/folders/123',
        })
        .reply(502)
        .delay(1000)
        .times(2)
})

I just think that it's not what the users are used to, at least the ones familiar with function mocking. The pattern of overriding an initial mock behavior is shown in the docs of Vitest (test 'should get with a mock') and Jest. Sinon.JS doesn't show it in the docs, but you can also change the return value of a stub in an imperative way.

Additionally, those libraries offer the way of resetting the mock/stub behavior (.mockReset(), .resetBehavior()). In Undici, after setting a persistent interceptor, there's no way back.

I see that Undici's MockAgent is heavily inspired by Nock, where behavior is exactly the same... Although there you can at least .deleteInterceptor() :smile: (kinda hacky though).

[Dzieni]

Actually, when I'm thinking about it, it's not about FIFO vs LIFO per se, but rather the .persist() having the same priority as one-time interceptors.

From the linked fragment of Jest docs:

When the mocked function runs out of implementations defined with .mockImplementationOnce(), it will execute the default implementation set with jest.fn(() => defaultValue) or .mockImplementation(() => defaultValue) if they were called:

And then it follows the intuition - on one hand, you can chain multiple calls of mockImplementationOnce and it'll be FIFO, but on the other hand you don't have to ensure that the default implementation is provided as the last one.

Maybe .persist() should allow setting the lowest priority? It could be an argument or a separate method.

[metcoder95]

I'm not really in sync with changing the behaviour to sync with other test runners. Overall the intention of Undici mocks seems to be as declarative as possible and scoped to single test scenarios (even if there are a few dozen, but that's more about test strategy and that's another topic).

That being said, I do see value in having a way to reset the mocks from a fake Agent.