fix: mocking of getters/setters on automatically mocked classes

[staplespeter]

This is a new pull request to complete #13398.

[staplespeter]

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]

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]

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

[nuragic]

Are this new changes/tests also covering #13466 ?

[SimenB]

Should add a test for it at least

[SimenB]

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

[SimenB]

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

[staplespeter]

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]

@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]

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]

@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]

@SimenB thanks, the comment is appreciated.

[github-actions]

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]

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]

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]

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.