jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.2k stars 6.46k forks source link

[Bug]: createMockFromModule not mocking class correctly #14178

Open dominiktopp opened 1 year ago

dominiktopp commented 1 year ago

Version

29.5.0

Steps to reproduce

  1. Clone my repo at https://github.com/dominiktopp/jestCreateMockFromModuleIssue
  2. npm install
  3. npm test
  4. tests will fail

Expected behavior

I expect the test cases to succeed.

Actual behavior

Tests are failing because jest.createMockFromModule only mocks constructor and functions for classes but no other properties (also no getters). The example for jest.createMockFromModule does not mock an exported class but an instance of a class: class: new (class Bar {...})()

Additional context

No response

Environment

System:
  OS: Windows 10 10.0.19045
  CPU: (12) x64 Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
  Memory: 10.67 GB / 31.76 GB
Binaries:
  Node: 16.15.0 - C:\Program Files\nodejs\node.EXE
  npm: 8.5.5 - C:\Program Files\nodejs\npm.CMD
github-actions[bot] commented 1 year ago

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.

dominiktopp commented 1 year ago

not stale!

KhaledElmorsy commented 1 year ago

I think the example in the docs combines two different mocking procedures, classes(ish) and objects. It implies the class is being mocked, when it's actually an instance (object) of that class. It needs to be edited or clarified to differentiate the result of mocking an instance and class constructor.

Currently, mock classes are constructor functions that produce objects that have a mocked constructor and mocked methods up the prototype chain. There are no properties because the class isn't instantiated when mocked.

Mocked objects, like the next example in the docs, maintain their property keys since their shape is known and just has to be maintained.

Mocking an instance produces a object with its current shape (mocked object), mocked methods from its prototype chain, and a constructor that acts as a mock class. Instantiating new objects with that constructor, will produces instances that don't have the properties in the original instance. (mocked class)

I can see two ways to go about this:

  1. Update the docs.
    • Add an actual class constructor function mock example
    • Highlight the lack of properties and why [classes aren't instantiated]
    • Add alternatives
      • Mock an instance
      • Manual mocks
  2. Update the class mocking logic to include properties but it comes with caveats
    • Instantiating a class when mocking can have side effects and/or rely on initial conditions to decide the shape
    • Static analysis can be done but will be limited to properties defined in the class declaration to circumvent the effect of initial conditions.

I can work on either approach but let's first decide the direction we want to move in.

dominiktopp commented 1 year ago

Hi Khaled,

Thanks for your help!

With 1 (the current state) there is a lot of boilerplate code: Either we have to define all properties after jest.createMockFromModule one by one (and remember to add new properties in the tests when adding to the class) or we have to use getter/setter pattern backed with private properties so that jest can just mock the getters/setters (like Java POJOs)

Way 2 sounds better for our daily usage of jest but instantiating and calling the real constructor before mocking is indeed a problem. Can you explain what you mean with the limitation when using static analysis?

KhaledElmorsy commented 1 year ago

With 1, there will definitely be some extra boilerplate. Defined getters and setters for certain properties would constrain the user too much for testing imo (unless they want to know if a property was accessed), with JS classes, they're also in the prototype not the instance so it'd be harder to follow different instances.

With static analysis it'll be tough to match an actual instance because properties can be added conditionally in the constructor. Example with bad code:

class Foo {
  constructor(bar) {
     this[bar] = 'please';
     if (bar) {
        this.buzz = 'stop'
     } else {
        Object.assign(this, killableWithFire)
     }
  }
}

There's no way we can predict the actual instance's shape. To circumvent this we can limit the mocked properties to fields defined in the class declaration.

class Foo {
  // Mock these
  bar;
  buzz = [1,2,3]

  // Ignore completely
  constructor () {
     this.level = 999;
     Object.assign(this, mixin);
  }
}

The caveat is that the user will need to declare the properties (arguably a good thing) if they want them to be mocked. This could get annoying with mixins especially.

dominiktopp commented 1 year ago

Thank you. For our use case the fields defined in the class declaration would be enough: Since we are using Typescript the first example does not even compile (with no implicit anys enabled) Fortunately we are not using mixins. We rely on inheritance where needed.

KhaledElmorsy commented 1 year ago

One more thing to consider is how to set the values of the fields if they're not defined or if they're set to the values of other variables. Should they be set to undefined?

With JS, types will also be a problem, like, an array property than gets set in the constructor based on a parameter value wouldn't be defined as an empty array in the class declaration. We could also set it to undefined, but it feels like users could find that unintuitive.

The more I think about this, the more I feel like we should somehow mock an instance that the user passes. The side effect implication is made clear to them as they instance the class, and they get to decide the initial conditions.

dominiktopp commented 1 year ago

It's not trivial. If the user has to pass an instance, than he could also use this instance as the mock object, couldn't he?

KhaledElmorsy commented 1 year ago

Yeah, they could . One small improvement that could be done if we settle for an approach like that is to use the instance to make a more complete constructor that produces fully mocked instances with the same shape instead of ones with just methods.

For a dev workflow, we could minimize this to a secondary file or folder with those base instances.

src/
├── Foo.js
└── class-mocks/
    └── Foo.js
// src/Foo.js
export default class {
   constructor(lang) {
     this.lang = lang;
   }
}
// sec/class-mocks/Foo.js
import Foo from '../Foo'
export default new Foo('JS')

Then in a test:

// Bar.test.js
const Foo = createMockFromModule('src/class-mocks/Foo').constructor
const testFoo = new Foo();
// testFoo.lang === 'JS'

It's very similar to the current approach with the only difference being the constructor now creating instances with properties.