vi.fn() performs an extra _await_ leading to mismatching behaviour when compared with normal function

[dubzzz]

Describe the bug

Replacing vi.fn(async () => {}) by async () => {} may lead to different results.

In some cases, the ordering of asynchronous tasks might be key. In fast-check, we have a scheduler able to help our users to automatically detect potential race conditions leading to bugs in their code. In order to test it, we used to rely on Jest but recently we moved to Vitest to benefit from ES Module support.

While playing with vi.fn() I encountered some issues related to when promises resolved: the order order is not consistent with a version not relying on vi.fn().

Reproduction

Here is an example highlighting the difference I encounter (diff between no vi.fn() or manual version of it and vi.fn()):

import { vi, test, expect } from "vitest";

// Passes!
test("should pass without vi.fn()", async () => {
  let obj = { done: false };
  const f = async () => {};
  Promise.resolve()
    .then(() => {})
    .then(() => (obj.done = true));
  await f();
  expect(obj.done).toBe(false);
});

// It should pass, but it fails!
test("should pass with vi.fn()", async () => {
  let obj = { done: false };
  const f = vi.fn(async () => {});
  Promise.resolve()
    .then(() => {})
    .then(() => (obj.done = true));
  await f();
  expect(obj.done).toBe(false);
});

System Info

System:
    OS: Windows 11 10.0.22621
    CPU: (4) x64 Intel(R) Core(TM) i5-3570K CPU @ 3.40GHz
    Memory: 1.25 GB / 7.89 GB
  Binaries:
    Node: 20.8.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.21 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 10.2.3 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (123.0.2420.97)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    vitest: ^1.5.3 => 1.5.3

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[dubzzz]

The source of the issue seems to be in tinyspy:

function I(e) {
  R(u("function", e) || u("undefined", e), "cannot spy on a non-function value");
  let t = function(...s) {
    let r = A(t);
    r.called = !0, r.callCount++, r.calls.push(s);
    let m = r.next.shift();
    if (m) {
      r.results.push(m);
      let [l, o] = m;
      if (l === "ok")
        return o;
      throw o;
    }
    let p, d = "ok";
    if (r.impl)
      try {
        new.target ? p = Reflect.construct(r.impl, s, new.target) : p = r.impl.apply(this, s), d = "ok";
        //                                                          ^ It calls my function and puts its
        //                                                            result in p without any extra wrapping 
      } catch (l) {
        throw p = l, d = "error", r.results.push([d, l]), l;
      }
    let a = [d, p];
    if (b(p)) {
      let l = p.then((o) => a[1] = o).catch((o) => {
        throw a[0] = "error", a[1] = o, o;
      });
      Object.assign(l, p), p = l;
      //                   ^ It assigns l to p, l being p with extra .then.catch
    }
    return r.results.push(a), p;
  };
  i(t, "_isMockFunction", !0), i(t, "length", e ? e.length : 0), i(t, "name", e && e.name || "spy");
  let n = A(t);
  return n.reset(), n.impl = e, t;
}

See my comments in the minified code

[dubzzz]

Without the minified code, the source of the issue is the rewrapping of the Promise done by:

    if (isPromise(result)) {
      const newPromise = result
        .then((r: any) => (resultTuple[1] = r))
        .catch((e: any) => {
          // @ts-expect-error TS for some reasons narrows down the type
          resultTuple[0] = 'error'
          resultTuple[1] = e
          throw e
        })
      // we need to reassign it because if it fails, the suite will fail
      // see `async error` test in `test/index.test.ts`
      Object.assign(newPromise, result)
      result = newPromise
    }

Result is not anymore the original result but a value derived from it and paying the cost of an extra .then. This extra .then is the reason why the code with vi.fn() does not behave the same way.

[dubzzz]

I'm attempting to come up with a fix (if feasible).

[dubzzz]

Fix sent: tinylibs/tinyspy#45

[dubzzz]

Is it worth adding a dedicated test on Vitest's side once the fix gets released on tinyspy? If so I can work on adding such test

[dubzzz]

Thanks you so much @sheremet-va, you are doing an awesome job!