jest icon indicating copy to clipboard operation
jest copied to clipboard

[Bug]: "mockImplementation" is behaving different than the syntacical-sugar version"mockResolvedValue"

Open mhombach opened this issue 2 years ago • 13 comments

Version

27.5.1

Steps to reproduce

Repro:

const testMock = {
    works: jest.fn().mockImplementation(() => Promise.resolve(undefined)),
    fails: jest.fn().mockResolvedValue(undefined),
};

it('test-mockResolvedValue', fakeAsync(async() => {
    await testMock.fails();
    flush();
    expect(true).toBeTrue();
}));

it('test-mockImplementation', fakeAsync(async() => {
    await testMock.works();
    flush();
    expect(true).toBeTrue();
}));

Expected behavior

Both tests are passing without any error, since both should be exactly the same as the Jest-docs are stating that mockFn.mockResolvedValue(value) is "Syntactic sugar function for" jest.fn().mockImplementation(() => Promise.resolve(value)).

Actual behavior

Test test-mockResolvedValue fails on the flush() due to The code should be running in the fakeAsync zone to call this function: image

Additional context

I feel this problem is linked to https://github.com/facebook/jest/issues/6645 and https://github.com/facebook/jest/issues/11146 .

Environment

System:
    OS: Windows 10 10.0.19043
    CPU: (12) x64 Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
Binaries:
    Node: 16.16.0 - C:\Program Files\nodejs\node.EXE
    npm: 8.11.0 - C:\Program Files\nodejs\npm.CMD
npmPackages:
    jest: ^27.2.3 => 27.5.1

mhombach avatar Sep 02 '22 09:09 mhombach

I can now confirm it has to do with https://github.com/facebook/jest/issues/6645 :

const testMock = {
    works: jest.fn().mockImplementation(() => Promise.resolve(undefined)),
    fails: jest.fn().mockResolvedValue(undefined),
};

it('test-mockImplementation', fakeAsync(async() => {
    console.log(new Promise(() => null) instanceof Promise); // true
    console.log(testMock.works() instanceof Promise); // true
    console.log(testMock.fails() instanceof Promise); // FALSE
    expect(true).toBeTrue();
}));

The mockResolvedValue() does NOT return a normal/real Promise, which buggs out ngZone.

mhombach avatar Sep 02 '22 09:09 mhombach

Any chance this issue will be verified and tackled? It's an easy reproducible bug which should be also somewhat-eaasy to fix.

mhombach avatar Sep 30 '22 11:09 mhombach

I confirm this should be fixed. This is very hard to debug as nothing indicates mockResolved or mockReject could have a different behavior.

For example, waiting for a next tick would finish the promises with mockImplementation but not with mockResolved or mockReject.

yannbriancon avatar Oct 24 '22 08:10 yannbriancon

Reproductions here give me ReferenceError: fakeAsync is not defined, fwiw.

A full standalone reproduction would allow me to verify if #13503 fixed the reported issue. You can also probably just make the change I made there in your own node_modules to verify.

SimenB avatar Oct 24 '22 10:10 SimenB

@mhombach please give https://github.com/facebook/jest/releases/tag/v29.2.2 a try 🙂

SimenB avatar Oct 24 '22 20:10 SimenB

@SimenB big thanks, will test tomorrow and report back :)

mhombach avatar Oct 24 '22 20:10 mhombach

Update:

Following a longer discussion on this issue: https://github.com/facebook/jest/issues/6645

The https://github.com/facebook/jest/issues/6645 tried to fix the problem, but (to my impression) failed to do so.

It added this unit-test (https://github.com/SimenB/jest/blob/464b45a015f98485d4037f1b4d4c5da750e39890/e2e/mock-functions/tests/index.js) to make sure that jest.fn().mockResolvedValue('hello')() is an instance of Promise.

test('instanceof promise', async () => {
  const resolvedPromise = jest.fn().mockResolvedValue('hello')();
  const rejectedPromise = jest.fn().mockRejectedValue('hello')();

  expect(resolvedPromise).toBeInstanceOf(Promise);
  expect(rejectedPromise).toBeInstanceOf(Promise);

  await expect(resolvedPromise).resolves.toBe('hello');
  await expect(rejectedPromise).rejects.toBe('hello');
});

This is the code written in the unit test in https://github.com/facebook/jest/issues/6645 . A working unit test like expect(resolvedPromise).toBeInstanceOf(Promise); should fail if I negate the unit test to expect(resolvedPromise).not.toBeInstanceOf(Promise);. But this is not the case. The unit test always silently fails, regardless of what is expected and so it "passes" with errors.

Also, if we do a console.log on resolvedPromise instanceOf Promise we get FALSE. Since the whole purpose of the fix that https://github.com/facebook/jest/issues/6645 implemented was precisely about resolvedPromise instanceOf Promise beeing TRUE, I would consider that PR-fix failed/flawed since it

  1. doesn't change the resolvedPromise instanceOf Promise expression to be TRUE and
  2. it implements unit tests that can never make the test suit fail and are, by that, useless.

@SimenB maybe we can continue our discussion here? I think I have provided sufficient evidence and explained my case in detail. A minimum repro would be:

test('instanceof promise', async () => {
      const resolvedPromise = jest.fn().mockResolvedValue('hello')();
      const rejectedPromise = jest.fn().mockRejectedValue('hello')();
    
      expect(resolvedPromise).not.toBeInstanceOf(Promise);
      expect(rejectedPromise).not.toBeInstanceOf(Promise);

      console.log(resolvedPromise instanceof Promise);
      console.log(rejectedPromise instanceof Promise);
    
      await expect(resolvedPromise).resolves.toBe('hello');
      await expect(rejectedPromise).rejects.toBe('hello');
    });

mhombach avatar Nov 02 '22 10:11 mhombach

@SimenB any feedback on this? Again, my assumption is that the fix you implemented does not work and the unit tests added are not passing and just silently erroring out.

mhombach avatar Nov 14 '22 09:11 mhombach

Issue is still not fixed

mhombach avatar Nov 25 '22 10:11 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Dec 25 '22 10:12 github-actions[bot]

Issue is not fixed yet

mhombach avatar Dec 25 '22 12:12 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Jan 24 '23 12:01 github-actions[bot]

Issue is not fixed yet

mhombach avatar Jan 25 '23 16:01 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Feb 24 '23 16:02 github-actions[bot]

Issue is not fixed yet

mhombach avatar Feb 24 '23 23:02 mhombach

@SimenB any chance you can have a look at this again? See my last comment here where I provided more insights. thx

mhombach avatar Feb 24 '23 23:02 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Mar 26 '23 23:03 github-actions[bot]

Issue is not fixed yet. Seriously, this issue is now open for over half a year and it's ignored?

mhombach avatar Mar 27 '23 09:03 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Apr 26 '23 10:04 github-actions[bot]

Issue needs a fix.

mhombach avatar Apr 26 '23 13:04 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar May 26 '23 14:05 github-actions[bot]

Issue still needs a fix.

mhombach avatar May 28 '23 09:05 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Jun 27 '23 10:06 github-actions[bot]

Issue is not fixed yet. Seriously, this issue is now open for almost 1 full year and it's ignored?

mhombach avatar Jun 27 '23 17:06 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Jul 27 '23 18:07 github-actions[bot]

@SimenB Almost 1 year has passed and there is no response or feedback regarding this issue. Could you at least explain why this bug, which isn't affecting only me and is hard to debug/find (as others here can confirm), is not even looked at by you or others? :/

mhombach avatar Jul 27 '23 18:07 mhombach

is not even looked at by you or others?

Can you proof that?

mrazauskas avatar Jul 27 '23 18:07 mrazauskas

@mrazauskas No communication since Nov 2, 2022 for a bug that is silently giving a PASS to any affected unit-test while the test should be failing (in a repo that is used in a gazillion of apps) is sadly something that I consider is not even looked, yes. I provided multiple ready-to-use examples for the bug and also provided my own insights of research (even if it wasn't a deepdive). The last statement was that there was a fix which, as I explained in detail, is having a bug on its own. Also, no reaction to this flawed unit-test aka "fix". I'm merely trying to get a dangerous bug fixed ...

mhombach avatar Jul 27 '23 19:07 mhombach

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Aug 26 '23 20:08 github-actions[bot]

Issue is not fixed yet

mhombach avatar Aug 26 '23 22:08 mhombach