fix: mocking of getters/setters on automatically mocked classes

[staplespeter]

This is a new pull request to complete #13145, which was prematurely closed.

[mrazauskas]

Minor detail. Could you move these files to /packages/jest-mock/src/__tests__/__fixtures__/? Recently I added /__fixtures__/ (see above) to this list to make it less noisy.

[staplespeter]

Sure, though I cannot see the /packages/jest-mock/src/__tests__/__fixtures__/ folder. The packages/jest-mock/__typetests__/__fixtures__/ folder does not exist either though I would have expected this to be a candidate for this reorg. Have I missed a commit/made a bad merge?

[mrazauskas]

There is no folder at the moment. Just create it (;

[mrazauskas]

I was trying to say that if you create src/__tests__/__fixtures__/ folder it will get ignored, because __fixtures__ was recently added to the ignore list. All would work without tweaking Jest config. That was the idea.

[staplespeter]

Apologies, I interpreted your initial comment as being that you had already done so, somewhere.

[SimenB]

awesome, thanks!

[staplespeter]

Glad to help dude

[SimenB]

https://github.com/facebook/jest/releases/tag/v29.2.0

[nuragic]

Hello! @SimenB This introduced a breaking change unfortunately... we had some named exports working since 2 hours ago, then we updated to 29.2.0 and suddenly things started breaking. Gist:

Cannot spy the initStuff property because it is not a function; undefined given instead

The line is:

jest.spyOn(myModule, ‘initStuff’);

The import we have is:

import * as myModule from 'my-module'

If I log the value of myModule... initStuff is there.

I see that in this PR there are some changes that could have caused this.

Moreover, if we pin jest-mock version to 29.1.2 it works.

[staplespeter]

Hi @nuragic, apologies that the change caused this issue. I will assume responsibility and try to determine the cause and a fix.

Are you able to provide source code for the object you are trying to mock? The full type hierarchy/prototype chain would be the best. There are tests for the general case of function mocking so the specific objects you are using would be essential to determine the case for the failure. Please provide anonymous code that also recreates the issue if you cannot provide the original source.

[staplespeter]

@nuragic I have tried the following, which worked okay.

class-mocks-types

export function testFunction1() {
  return 'testFunction1';
}

function testFunction() {
  return 'testFunction2';
}
export const testFunction2 = testFunction;

export const testFunction3 = () => {
  return 'testFunction3';
};

class-mocks.test

import * as testTypes from './__fixtures__/class-mocks-types';

it('can mock a directly exported function', () => {
    jest.spyOn(testTypes, 'testFunction1').mockImplementation(() => {
      return 'mockTestFunction';
    });
    expect(testTypes.testFunction1()).toBe('mockTestFunction');
  });

  it('can mock an indirectly exported function', () => {
    jest.spyOn(testTypes, 'testFunction2').mockImplementation(() => {
      return 'mockTestFunction';
    });
    expect(testTypes.testFunction2()).toBe('mockTestFunction');
  });

  it('can mock an indirectly exported anonymous function', () => {
    jest.spyOn(testTypes, 'testFunction3').mockImplementation(() => {
      return 'mockTestFunction';
    });
    expect(testTypes.testFunction3()).toBe('mockTestFunction');
  });

[devcorraliza]

Hi all, same here.

Here you can find and example failing.
Working with 29.1.2
Falling with 29.2.0

I hope it helps

[staplespeter]

@devcorraliza thanks! that's great. Recreated the error. This is specific to module imports where the exports are exposed as accessor properties and retrieved through getters. In module mocking jest.mock() this is picked up with the __esmodule = true flag. I did not consider the case for this in function export mocking and there were no existing tests to catch this, so I effectively removed this functionality.

So I was able to recreate the error in your jestspyonerror repo. However, when I copied your code to the jest repo it passed without any changes, as my similar code did previously (see this commit). In this case the exports are just data properties, not accessors, so the normal function mocking works. So, with my limited experience I can only assume this is a node/Javascript versioning difference/config issueto do with how module contents are exported. I couldn't see any immediate package settings that differed so I will have to leave it to another person to provide an explanation of the root cause. This also leaves the issue of verifying the change in the jest repo. Help here would be appreciated.

I have applied the fix to the jest repo in this commit.

As I could not immediately recreate the issue in the jest repo to verify the changes I tried to use an existing test that brought this case to my attention for automatic mocks (packages/jest-core/src/__tests__/TestScheduler.test.js -> test('works with default value')). In this test the classes were exposed by accessors/getters. I modified this test to see if an individual class export could be mocked with jest.spyOn(). Whilest the class export was exposed with an accessor and the code branched correctly given the changes, the mocking would not complete due the accessor property not being redefinable - error TypeError: Cannot redefine property: CoverageReporter. See this commit. This is not an issue for full mocking of a module as the entire imported object is replaced with a new mock object (based on a metadata tree) and thus no direct property redefinitions occur on the imported object. I am not sure if mocking of individual class imports using spyOn are a valid use case and thus needs dealt with also. In my opinion it is.

Calling on @SimenB , @mrazauskas for assistance.

[devcorraliza]

Hi @staplespeter, maybe the difference is at the transpiler used. In my repo I'm using swc and additionally a plugin to make properties configurable (jest_workaround). Anyway it is working perfectly with 29.1.2

Just to add more information (although i think is not relevant) my node version is 14.19.3

Thanks for all the explanation and commit, just a remainder for people in charge that tests are broken and it is not easy to fix because jest_mock v29.2.0 is automatically installed although you set jest 29.1.2 at package.json. I would recommend to set a fix revision for jest_mock to restore last state till we have a fix for that.

Thx in advance

[staplespeter]

@devcorraliza thanks for the extra info.
I agree that this should either be reverted or the fix applied ASAP as it will likely break a lot of tests. I'm pretty confident that the fix is correct (it's pretty much the code that did this in 29.1.2), though it would need at least visually checked against the working version in the jestspyonerror repo. Omitting a fix for the latter issue I mentioned, of spying on a class export, would not cause an issue as I think this would not have worked in 29.1.2 anyway (haven't checked this)

[SimenB]

Happy to take a PR with a fix 🙂

[SimenB]

@staplespeter this is the transpiled code which cannot be spied upon (passed through prettier)

'use strict';
Object.defineProperty(exports, '__esModule', {
  value: true,
});
function _export(target, all) {
  for (const name in all)
    Object.defineProperty(target, name, {
      enumerable: true,
      get: all[name],
      configurable: true,
    });
}
_export(exports, {
  f1: () => f1,
  f2: () => f2,
});
const f1 = () => {
  console.log('Im f1 calling f2');
  (0, exports.f2)();
};
const f2 = () => console.log('Im f2');

[nuragic]

@staplespeter Hey, you don't have to apologise at all. Thanks for your contributions. We're all here to help. 🤗

@devcorraliza You can temporarily workaround this issue by adding this to your package.json:

“overrides”: {
  “jest-mock”: “29.1.2”
}

Happy to provide more info/help on this.

In our case it also has to do with transpiled code (by TypeScript) which is (expanding on the above example):

var init_stuff_1 = require("./init-stuff");
Object.defineProperty(exports, “initStuff”, { enumerable: true, get: function () { return init_stuff_1.initStuff; } });

Inside ./init-stuff.ts we have:

export function initStuff () { /* … */ }

[nuragic]

@staplespeter What I found by quickly debugging the issue is that, the check we have here is overlooking descriptor.get which in our case is a function; descriptor.value is undefined. I guess we should check both.

[staplespeter]

Thanks @nuragic. I've added a pull request for the fix and actually realised that restore functionality for accessor properties was also missing, so have added that too. One test is incomplete, so if it is okay i will continue discussion on the new pull request.

[SimenB]

I've released https://github.com/facebook/jest/releases/tag/v29.2.1 reverting most of this PR (modulo a bunch of tests that still pass)

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.