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 #13460

Closed
wants to merge 19 commits into from

Conversation

staplespeter
Copy link
Contributor

This is a new pull request to complete #13398.

staplespeter and others added 13 commits October 6, 2022 12:22
…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.
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
# Conflicts:
#	CHANGELOG.md
…e currently only test for functions exported as data properties, not as accessors/getters
…restoring of accessor properties. Test for function exports via getters.
@staplespeter
Copy link
Contributor Author

I added a test for module exports that are imported via getters, based upon @SimenB code.

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

The test,'can mock a function export', is here. The restoring of the original property fails and is not tested. Can anyone tell me how to make this a valid test?

@SimenB
Copy link
Member

SimenB commented Oct 17, 2022

The test,'can mock a function export', is here. The restoring of the original property fails and is not tested. Can anyone tell me how to make this a valid test?

@mrazauskas any idea?

Did it work before? If not it shouldn't block

@mrazauskas
Copy link
Contributor

Looks complicated. Maybe mockRestore() is buggy? Only guessing (;

@nuragic
Copy link

nuragic commented Oct 18, 2022

Are this new changes/tests also covering #13466 ?

@SimenB
Copy link
Member

SimenB commented Oct 18, 2022

Should add a test for it at least

@SimenB
Copy link
Member

SimenB commented Oct 18, 2022

I need to make a release for Node 19 today btw, so I might have to revert (again 😅)

@SimenB
Copy link
Member

SimenB commented Oct 18, 2022

I reverted the original PR in #13472 (but kept a bunch of tests). I'll merge in main here

@staplespeter
Copy link
Contributor Author

Looks complicated. Maybe mockRestore() is buggy? Only guessing (;

It was the defineProperty() during the restore that resulted in the property being undefined. Not sure why as the defineProperty() during the test worked.

@staplespeter
Copy link
Contributor Author

@SimenB I've reapplied the automatic mocking tests that were removed, and reapplied the original spyOn method structures. Apologies again for the hassle caused.

@staplespeter
Copy link
Contributor Author

staplespeter commented Oct 19, 2022

The dispatchEvent bug #13466 was caused by the window.dispatchEvent property not having a [public at least??] descriptor and not being visible in the debugger, though window['dispatchEvent'] returns the function definition. The latter was used in he original code whilst my well-intended refactoring relied on property descriptors. It became a little complicated to try to fall back to the property read if the descriptor was not present, and the original code seems to cause less issues.

That being said the original code const original = object[methodKey]; fails to get a function definition in spyOn() in the test below - undefined is returned instead and spyOn() throws an exception. This also occurred in the original code when automatically mocking a getter/setter, which was one of the reasons to try to use property descriptors exclusively in spyOn(), as property reads seemed unreliable.

In the test below, in my version of spyOn() the property descriptor is returned and the mock applied. Calling defineProperty() to restore the original accessor property definition resulted in an undefined property, which then caused the assertion to fail. Alas, as mentioned, there exist objects that have properties with no available property descriptors (??).

It seems there are edge cases for either implementation so detecting both may be necessary at some point. As use of property descriptors seems to cause more real-world hassle I'll stick with the original code for now.

It would be nice to know why these anomalies occur, if anyone has the knowledge.

it('can mock a function export', () => {
    const exports = {
      __esModule: true,
      fn: () => {},
    };
    Object.defineProperty(exports, 'fn', {
      configurable: true,
      enumerable: true,
      get: () => {
        () => 'testFunction';
      },
    });

    jest.spyOn(exports, 'fn').mockImplementation(() => 'mockTestFunction');
    expect(exports.fn()).toBe('mockTestFunction');

    // mockFn.mockRestore();
    // expect(exports.fn()).toBe('testFunction');
  });

@SimenB
Copy link
Member

SimenB commented Oct 19, 2022

@SimenB I've reapplied the automatic mocking tests that were removed, and reapplied the original spyOn method structures. Apologies again for the hassle caused.

Thanks! And don't worry about the regressions too much - reverting is easy enough 🙂 The real issue is our tests not covering the super valid use cases people have, not your changes breaking them 👍

@staplespeter
Copy link
Contributor Author

@SimenB thanks, the comment is appreciated.

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Jan 17, 2023
@github-actions
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Feb 16, 2023
@github-actions
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Feb 16, 2023
@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 Mar 19, 2023
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.

5 participants