Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mocking of getters/setters on automatically mocked classes #13398

Merged
merged 8 commits into from Oct 7, 2022

Conversation

staplespeter
Copy link
Contributor

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

…ed classes (jestjs#13145)""

This reverts commit 52d45be.

# Conflicts:
#	CHANGELOG.md
… 'get'/'set' functions being mocked as accessor objects. Added extra tests for this, and for some other conditions. Corrected expected test error messages due to the refactored code for the fix to this bug.
jest.config.mjs Outdated
Comment on lines 60 to 62
'/packages/jest-mock/src/__tests__/class-mocks-types.ts',
'/packages/jest-mock/src/__tests__/TestClass.ts',
'/packages/jest-mock/src/__tests__/SuperTestClass.ts',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jest.config.mjs Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks!

@SimenB SimenB merged commit f126336 into jestjs:main Oct 7, 2022
@staplespeter
Copy link
Contributor Author

Glad to help dude

@SimenB
Copy link
Member

SimenB commented Oct 14, 2022

@nuragic
Copy link

nuragic commented Oct 14, 2022

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
Copy link
Contributor Author

staplespeter commented Oct 14, 2022

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
Copy link
Contributor Author

staplespeter commented Oct 14, 2022

@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
Copy link

devcorraliza commented Oct 14, 2022

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
Copy link
Contributor Author

staplespeter commented Oct 15, 2022

@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
Copy link

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
Copy link
Contributor Author

@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
Copy link
Member

SimenB commented Oct 15, 2022

Happy to take a PR with a fix 🙂

@SimenB
Copy link
Member

SimenB commented Oct 15, 2022

@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
Copy link

nuragic commented Oct 17, 2022

@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 () { /* … */ }

);
}
} else if (typeof descriptor.value !== 'function') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

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 added a commit to SimenB/jest that referenced this pull request Oct 18, 2022
@SimenB
Copy link
Member

SimenB commented Oct 18, 2022

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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants