breaking change in 21.7.0 when mocking fetch

[hulkish]

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?

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

[regseb]

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();

[targos]

@nodejs/test_runner @joyeecheung

[YCChenVictor]

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!

[joyeecheung]

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.

[regseb]

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()
  }

[aduh95]

@regseb That was done in #51598 for performance reasons. Your best course of actions is to trigger all the getters before your tests start.

[aduh95]

@regseb That was done in #51598 for performance reasons. Your best course of actions is to trigger all the getters before your tests start.

[regseb]

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);
  },
});

[joyeecheung]

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.

[hulkish]

In any case, isn't it still considered a breaking change and should be in a major node version change?

[regseb]

There's the same problem with Node v20.12.0:

• bd2a3c1 lib: define FormData and fetch etc. in the built-in snapshot
• v20.12.0 proposal #52212

[yeoffrey]

Same problem reproduced with Node v20.12.1