breaking change in 21.7.0 when mocking fetch
hulkish opened this issue · 17 comments
Version
21.7.0
Platform
Darwin Stevens-MacBook-Pro.local 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:54:55 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T8122 arm64
Subsystem
No response
What steps will reproduce the bug?
- Make a test which mocks fetch:
import { describe, it } from 'node:test';
import assert from 'node:assert/strict';
import { fetchSomething } from '../lib/index.mjs';
describe('my test suite', () => {
it('fetch stuff', async (t) => {
const mockValue = { key: 'value' };
const mockFetch = async () => ({
json: async () => mockValue,
status: 200,
});
t.mock.method(global, 'fetch', mockFetch);
assert.deepStrictEqual(await fetchSomething(), mockValue);
t.mock.restoreAll();
});
});which produces:
✖ fetch stuff (0.894292ms)
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'methodName' must be a method. Received undefined
at MockTracker.method (node:internal/test_runner/mock/mock:241:13)
at TestContext.<anonymous> (my.test.mjs:13:12)
at Test.runInAsyncScope (node:async_hooks:206:9)
at Test.run (node:internal/test_runner/test:641:25)
at Suite.processPendingSubtests (node:internal/test_runner/test:382:18)
at Test.postRun (node:internal/test_runner/test:732:19)
at Test.run (node:internal/test_runner/test:690:12)
at async Promise.all (index 0)
at async Suite.run (node:internal/test_runner/test:966:7)
at async startSubtest (node:internal/test_runner/harness:218:3) {
code: 'ERR_INVALID_ARG_VALUE'
}to fix the break, i have to reference global.fetch before mocking it...ie:
fetch;
t.mock.method(global, 'fetch', mockFetch);How often does it reproduce? Is there a required condition?
Consistently
What is the expected behavior? Why is that the expected behavior?
Should not break existing code on a minor or patch semver change of node.
What do you see instead?
It breaks
Additional information
This is the PR which introduced the break: #51598 (comment)
This is another simplified test case:
import { mock } from "node:test";
mock.method(globalThis, "fetch", async () => ({
text: async () => "foo",
}));
const response = await fetch("https://bar.baz/");
const text = await response.text();
console.log(text);
mock.restoreAll();@nodejs/test_runner @joyeecheung
It appears that this issue can be resolved by reintroducing the following code within the setupUndici() function:
if (!getOptionValue('--no-experimental-fetch')) {
defineOperation(globalThis, 'fetch', fetch);
}
As a newcomer, I'm eager to contribute and learn. If there are any errors in my understanding, I would appreciate your guidance. Thank you for your assistance!
It seems to me that it’s t.mock that should be fixed, other wise it would not work with user land objects that has a similar descriptor.
I think the bug comes from the fetch method, because I have the same problem with sinonjs/sinon#2590 (which doesn't use mock).
The descriptor of property fetch is different before and after using it.
console.log(Object.getOwnPropertyDescriptor(globalThis, "fetch"));
fetch;
console.log(Object.getOwnPropertyDescriptor(globalThis, "fetch"));-
Node.js v21.7.0:
{ get: [Function: get], set: [Function: set], enumerable: true, configurable: true } { value: [Function: fetch], writable: true, enumerable: true, configurable: true }
-
Node.js v21.6.2:
{ value: [Function: fetch], writable: true, enumerable: true, configurable: true } { value: [Function: fetch], writable: true, enumerable: true, configurable: true }
-
Chromium 122:
{ writable: true, enumerable: true, configurable: true, value: f fetch() } { writable: true, enumerable: true, configurable: true, value: f fetch() }
You could directly define the fetch function in value (without the intermediate steps of get and set). The descriptor won't change and you won't lose lazy loading.
ObjectDefineProperty(globalThis, 'fetch', {
__proto__: null,
writable: true,
value: function fetch(input, init = undefined) {
// Loading undici alone lead to promises which breaks lots of tests so we
// have to load it really lazily for now.
const { fetch: impl } = require('internal/deps/undici/undici');
return impl(input, init);
},
});At end of the day, it seems to be a design issue with the mocking libraries, as their API forces users to know and differentiate between getters and values even though users may only care about hijacking or recording the access, but don’t care about whether the access is done via a getter or not. For example with Sinon I think you would then need to use a spy with .get(), and this applies to user objects too sinonjs/sinon#1545 That said for properties that are themselves methods, like in the case of fetch, doing the lazy loading inside the value method seems to be a good compromise if it helps users work around this issue with the mocking libraries. I am not sure about FormData and friends because as constructors wrapping them complicates instanceof, but they may be less necessary to mock anyway.
In any case, isn't it still considered a breaking change and should be in a major node version change?
Same problem reproduced with Node v20.12.1
Is there a reason why this issue is marked as closed? The issue still persists into v22.0.0 and wasn't fixed by #52275 . Has the discussion moved somewhere else?