nodejs / nodejs/undici

MockInterceptor: `.reply` should be LIFO

Open
#2,637 4 comments 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
JavaScript
Stars
7.7k
Forks
880
Avg merge
2d 16h
Merged PRs (30d)
68

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the addMockDispatch entry point and reproduce the issue with the persistent and temporary interceptors shown in the report. Check the MockInterceptor behavior and relevant tests, then verify that the later reply takes precedence for matching requests while the persistent interceptor remains usable afterward.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
testing-qa
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.