aelbore / esbuild-jest

A Jest transformer using esbuild
518 stars 51 forks source link

spyOn: Cannot redefine property #26

Open a-h opened 3 years ago

a-h commented 3 years ago

While mocks are working great for me, I'm having some trouble with spies.

I created this code.

import { moduleDependency } from "./dependency";

export const internalDependency = () => "concrete";

export interface Result {
  internal: string;
  module: string;
}

export const subjectUnderTest = (): Result => {
  return {
    internal: internalDependency(),
    module: moduleDependency(),
  };
};

And then a test to exercise it:

import { subjectUnderTest } from ".";
import * as index from "."; 

jest.spyOn(index, "internalDependency").mockReturnValue("mocked");

describe("index.ts", () => {
  it("can replace internal dependencies with spies", () => {
    const result = subjectUnderTest();
    expect(result).toEqual({
      internal: "mocked",
      module: "concrete",
    });
  });
});

However, running the tests produces an error:

$ /Users/adrian/github.com/a-h/esbuild-mock-test % npx jest
 PASS  ./index-withmock.test.ts
 PASS  ./index-withoutmock.test.ts
 FAIL  ./index-withspy.test.ts
  ● Test suite failed to run

    TypeError: Cannot redefine property: internalDependency
        at Function.defineProperty (<anonymous>)

      at ModuleMockerClass.spyOn (node_modules/jest-mock/build/index.js:826:16)
      at Object.<anonymous> (index-withspy.test.ts:21:6)

Test Suites: 1 failed, 2 passed, 3 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        1.423 s
Ran all test suites.

Am I missing something?

A minimal reproduction is available at https://github.com/a-h/esbuild-mock-test

Versions etc.

  "devDependencies": {
    "@types/jest": "^26.0.20",
    "esbuild": "^0.9.3",
    "esbuild-jest": "^0.5.0",
    "jest": "^26.6.3",
    "typescript": "^4.2.3"
  },
jimmed commented 3 years ago

+1 on this particular issue; this is the only thing stopping me migrating a particularly chonky codebase from using ts-jest to esbuild-jest (which would take literal minutes off the test run times).

Happy to provide any additional info, or test potential fixes.

aelbore commented 3 years ago

@a-h maybe you can check this https://github.com/evanw/esbuild/issues/412 after checking on it, esbuild for now doesnt support esm for jest.spy on

also this https://github.com/evanw/jest-esbuild-demo

a-h commented 3 years ago

Thanks for that. I've refactored the code in the example repo to support spies by moving to the commonjs format at https://github.com/a-h/esbuild-mock-test/commit/07131865d6f2bc1fc845f9c8f71b5018c3b2f30d

However, if I run the tsc compiler on the import / export const version, I get a JavaScript test that I can run with npx jest *.js that does work with spies, so I thought it might be possible somehow.

{
  "compilerOptions": {
    "module": "commonjs",
    "noImplicitAny": true,
    "removeComments": true,
    "preserveConstEnums": true,
    "sourceMap": true
  }
}

TypeScript

import * as index from "."; 

//@ts-ignore
jest.spyOn(index, "internalDependency").mockReturnValue("mocked");

describe("index.ts", () => {
  it("can replace internal dependencies with spies", () => {
    const result = index.subjectUnderTest();
    expect(result).toEqual({
      internal: "mocked",
      module: "concrete",
    });
  });
});

tsc output

"use strict";
exports.__esModule = true;
var index = require(".");
jest.spyOn(index, "internalDependency").mockReturnValue("mocked");
describe("index.ts", function () {
    it("can replace internal dependencies with spies", function () {
        var result = index.subjectUnderTest();
        expect(result).toEqual({
            internal: "mocked",
            module: "concrete"
        });
    });
});
//# sourceMappingURL=index-withspy.test.js.map
kaioduarte commented 3 years ago

I just faced this issue and I managed to workaround it by creating a variable with the methods I want to spy

This doesn't work:

import * as niceGrpc from "nice-grpc";

jest.spyOn(niceGrpc, "createClient").mockReturnValue({});
jest.spyOn(niceGrpc, "createChannel").mockReturnValue({});

This works:

import { createChannel, createClient } from "nice-grpc";
const foo = { createChannel, createClient };

jest.spyOn(foo, "createClient").mockReturnValue({});
jest.spyOn(foo, "createChannel").mockReturnValue({});
kevindra commented 3 years ago

I'm trying to use this method with brcypt but didn't have success.

import { compareSync } from 'bcrypt'
const bcrypt = { compareSync }

jest.spyOn(bcrypt, "compareSync").mockReturnValue(true);

Still returns false.. any idea what might be happening here?

kevindra commented 3 years ago

Just sharing what worked for me. To get around it, I had to create my own bcrypt namespace like so:

import originalBcrypt from "bcrypt";
export namespace bcrypt {
  export const compareSync = originalBcrypt.compareSync;
  export const compare = originalBcrypt.compare;
}

and then mocked like so:

import { bcrypt } from "../../src/types"

jest.spyOn(bcrypt, "compareSync").mockReturnValue(true);
kaioduarte commented 3 years ago

@kevindra it worked for me image

akshayr20 commented 2 years ago

Any update on this issue, above workaround seems to be not handling edge case.

x method used in two files for making HTTP request. Spying x method using above workaround fixes type error

Pending Issue: Above workaround do not correctly spy x method, may be due to pointing at incorrect location in the memory

akshayr20 commented 2 years ago

unfortunately mockImplementation on above spy still do not work, can someone guide me around same

a-h commented 2 years ago

@akshayr20 - I think you're more likely to get an answer if you provide a reproduction of the issue - e.g. a minimal reproduction of the issue. I don't think your comment is clear enough on what you're asking for help with.

aecorredor commented 2 years ago

Any update on above issue, or any hint on where should I look into to debug this further?

I think another workaround is using jest.mock like so:

jest.mock('react-i18next', () => ({
  useTranslation: jest.fn()
}));

import { useTranslation } from 'react-i18next';

// some test code below
expect(useTranslation).toHaveBeenCalledWith(...);

This is an example with a different library, but should be the same idea.

alita-moore commented 2 years ago

dir/index.ts

export * from "./file"

dir/file.ts

export foo = () => {}

file.test.ts

import * as Index from "./dir";
import * as File from "./dir/file";

it ("does not work", () => {
  jest.spyOn(Index, "foo")
})

it("works", () => {
  jest.spyOn(File, "foo")
})

🤷‍♀️

EwertonLuan commented 2 years ago

the solution of @aecorredor works if you need mock all module but if you need only one function this worked for me:

import * as Foo from 'foo';

jest.mock('foo', () => ({
  __esModule: true,
    // @ts-ignore
    ...jest.requireActual('foo'),
}));

it('foo', () => {
 jest.spyOn(Foo, 'fooFunction')
})
kirkstrobeck commented 2 years ago
jest.mock('fooModule', () => {
  const originalModule = jest.requireActual('fooModule');

  return {
    ...originalModule,

    doThing: jest.fn().mockReturnValue('works!'),
  };
});
marsonmao commented 2 years ago

The root cause is explained in this SO answer, which explains why these workaround works

import { createChannel, createClient } from "nice-grpc";
const foo = { createChannel, createClient };
jest.spyOn(foo, "createClient").mockReturnValue({});
jest.spyOn(foo, "createChannel").mockReturnValue({});

[X] export * from "./file"
[O] export foo = () => {}

I have no clear way to "fix" this, just sharing the information here.

Also it looks like the issue is related to TS https://github.com/microsoft/TypeScript/issues/43081, probably will be fixed in TS 4.5.0

vividn commented 2 years ago

When this problem occurs with external libraries, you can create and use a passthrough function in your own code, which can then be spied on without problems

assignGroupId.ts

import { assignGroupID } from "algosdk";

// This pass through function is to allow spying in the tests
// Without it get a "Cannot redefine property" error
export default assignGroupID;

someSrcFile.ts

import { /* otherFunctionsFromLib */ } from "algosdk";
import assignGroupId from "src/group/assignGroupId";
...
assignGroupId(txns);

someSrcFile.test.ts

import * as assignGroupId from "src/group/assignGroupId";
...
jest.spyOn(assignGroupId, "default").mockImplementation(() => {});
kamilzielinskidev commented 2 years ago

@EwertonLuan this is the best answer for me, not perfect but the cleanest from all, thanks

R-Iqbal commented 2 years ago

dir/index.ts

export * from "./file"

dir/file.ts

export foo = () => {}

file.test.ts

import * as Index from "./dir";
import * as File from "./dir/file";

it ("does not work", () => {
  jest.spyOn(Index, "foo")
})

it("works", () => {
  jest.spyOn(File, "foo")
})

🤷‍♀️

Have you been able to find a way to create the mock the import * as Index from "./dir"; import?

mywristbands commented 2 years ago

For me, this blog article worked to allow me to mock individual functions of a module for each test. Relevant code below:

import { functionToMock } from "@module/api"; // Step 3.

// Step 1.
jest.mock("@module/api", () => {
    const original = jest.requireActual("@module/api"); // Step 2.
    return {
        ...original,
        functionToMock: jest.fn()
    };
});

// Step 4. Inside of your test suite:
functionToMock.mockImplementation(() => ({ mockedValue: 2 }));
WellingtonDefassio commented 2 years ago

dir/index.ts

export * from "./file"

dir/file.ts

export foo = () => {}

file.test.ts

import * as Index from "./dir";
import * as File from "./dir/file";

it ("does not work", () => {
  jest.spyOn(Index, "foo")
})

it("works", () => {
  jest.spyOn(File, "foo")
})

🤷‍♀️

dir/index.ts

export * from "./file"

dir/file.ts

export foo = () => {}

file.test.ts

import * as Index from "./dir";
import * as File from "./dir/file";

it ("does not work", () => {
  jest.spyOn(Index, "foo")
})

it("works", () => {
  jest.spyOn(File, "foo")
})

🤷‍♀️

that works for me! thx

DanielPe05 commented 2 years ago

Hey y'all!! This is still an issue as of today, I'm stuck with ts-jest (which does not have the problem) because of this. I have a simple setup file for jest doing this before the tests run:

import * as example from './example'

jest.spyOn(example, 'foo').mockResolvedValue()

and the issue comes up. Do we know why this happens with this package and not ts-jest? Trying to understand the issue better to see if I could try to contribute a fix for it since this is keeping us from using it. Thanks for your amazing work on this!

youf-olivier commented 2 years ago

I have a similar issue but a bit different (ununderstandable solution) :

I have an utils file :

const funcB = ()=>{
  ...
}

const funcA = ()=>{
  x = funcB();
  ...
}

And a component :


const Component = (props)=>{
    const handleClick = () => funcA():

   return <Button onClick={handleClick}>click</Button>
}

Before a few days, my spyOn was ok in the test :

import * as Utils from '~/src/common/utils';

...

test(()=>{
   const spyOnA = jest.spyOn(Utils, 'funcA');
...
}

But now I have the same error : "cannot redefine property"

My solution is to specify that :

import * as Utils from '~/src/common/utils';

jest.mock('~/src/common/utils', () => ({
  ...jest.requireActual('~/src/common/utils'),
  funcA: jest.fn(),
}));

...

test(()=>{
   const spyOnA = jest.spyOn(Utils, 'funcA');
...
}

With that, I suppose we say to use the actual implemantation of funcB 🤷🏻

Darkzarich commented 2 years ago

My solution was to use the construction like

jest.mock('@/shared/lib/notify');
import { notify } from '@/shared/lib/notify';

// ...

it('...', () => {
// ... 

    if (jest.isMockFunction(notify.openError)) {
        expect(notify.openError).toHaveBeenCalledWith({
        text: `Error: ${errorText}`,
    });
    }

    jest.restoreAllMocks();
}) 
gersondinis commented 1 year ago

This also works if you need all methods:

import * as fooMethods from "nice-grpc";
const foo = {...fooMethods};

jest.spyOn(foo, "createClient").mockReturnValue({});
jest.spyOn(foo, "createChannel").mockReturnValue({});
Guy7B commented 1 year ago

That was good for us:

///////// above the test content
import { someFunction } from 'some-module';

jest.mock('some-module', () => {
  const originalModule = jest.requireActual('some-module');
  return {
    ...originalModule,
    someFunction: jest.fn((...args) => originalModule.someFunction(...args)),
  };
});
//////////

//////// Inside the test:
expect(someFunction).toHaveBeenCalledWith({ ... });
CalebJamesStevens commented 1 year ago

Hey all, i took a long time to comb through the code and figure out why this is happening. If you are using next/jest then this is happening because next default the transform object in your jest config to be swc. In order to fixed this issue all you need to do is override the transform by adding this to you jest config

const nextJest = require('next/jest');

const createJestConfig = nextJest({
  dir: './',
});

module.exports = createJestConfig({
  // other config settings
  transform: {
    '^.+\\.(js|jsx|ts|tsx|mjs)$': 'babel-jest',
  },
});

As long as the transformer your using doesn't cause exports to have un configurable properties you should be just fine

alexya commented 1 year ago

@CalebJamesStevens but the swc/jest is much faster than babel, right? if we want to use swc instead of babel-jest, how to fix the problem?

baublet commented 1 year ago

+1 here, this can be a point where ESBuild can excel over SWC. The SWC team don't seem to care that this is how Jest and its spies have worked for years, and thousands of tests and lines of test code rely on it. It's a testing environment, so making these configurable to match years of expected behavior seems uncontroversial to me; leave the default as is, but allow us to rewrite these as configurable so our tests still pass.

Their take is some version of "it wasn't correct, so we shouldn't fix it," and instead tell us to rewrite all of our tests. Unfortunately, that's not an option for us.

image
ranran2121 commented 1 year ago

I tryed all the combination proposed above but still not able to pass the test. NB: functions are simplified

services.js export const getOutputs = (chain) => { return chain };

cli.js

import {  getOutputs} from "./services.js";

export function askAndGet(rl_instance, callback) {
  rl_instance.question("Enter the chain: ", async (chain) => {    
    await callback(chain);
    rl_instance.close();
  });
}

export const followupQuestion = (rl, answer) => {
  switch (answer) {
    case "outputs":
      askAndGet(rl, getOutputs);
      break;
        default:
      console.log("invalid input");
  }
};

cli.test.js

import { jest } from "@jest/globals";
import {  followupQuestion } from "./cli.js";
import { getOutputs } from "./services.js";
jest.mock("./services.js", () => {
  const original = jest.requireActual("./services.js"); // Step 2.
  return {
    ...original,
    getOutputs: jest.fn((...args) => original.getOutputs(...args)),
  };
});

const readlineMock = {
  createInterface: jest.fn().mockReturnValue({
    question: jest.fn().mockImplementationOnce((_questionTest, cb) => {
      cb("y");
      readlineMock.createInterface().close();
    }),
    close: jest.fn().mockImplementationOnce(() => undefined),
  }),
};

describe("followupQuestion", () => {
  test("should call getOutputs when the parameter is output", async () => {
    const rl = readlineMock.createInterface();

    followupQuestion(rl, "outputs");

    expect(getOutputs).toHaveBeenCalled();
  });
});

with this test the error I get is Matcher error: received value must be a mock or spy function

Any help?

MisterJimson commented 1 year ago

This fixed a bunch of tests for me https://github.com/aelbore/esbuild-jest/issues/26#issuecomment-1341765767

All I was missing in my transform was mjs, which I don't use directly in my project.

captain-woof commented 1 year ago

I just faced this issue and I managed to workaround it by creating a variable with the methods I want to spy

This doesn't work:

import * as niceGrpc from "nice-grpc";

jest.spyOn(niceGrpc, "createClient").mockReturnValue({});
jest.spyOn(niceGrpc, "createChannel").mockReturnValue({});

This works:

import { createChannel, createClient } from "nice-grpc";
const foo = { createChannel, createClient };

jest.spyOn(foo, "createClient").mockReturnValue({});
jest.spyOn(foo, "createChannel").mockReturnValue({});

You saved my life. Thank you for this! This works!!!

motss commented 1 year ago

I just faced this issue and I managed to workaround it by creating a variable with the methods I want to spy

This doesn't work:

import * as niceGrpc from "nice-grpc";

jest.spyOn(niceGrpc, "createClient").mockReturnValue({});
jest.spyOn(niceGrpc, "createChannel").mockReturnValue({});

This works:

import { createChannel, createClient } from "nice-grpc";
const foo = { createChannel, createClient };

jest.spyOn(foo, "createClient").mockReturnValue({});
jest.spyOn(foo, "createChannel").mockReturnValue({});

You saved my life. Thank you for this! This works!! !

This is wild. I gotta try this later.

ejjsharpe commented 1 year ago

For me I've found that this happens when I'm trying to put a spy on a file that also exports types. I think this is why the above work arounds work for a lot of people as you are not importing the whole file (which also contains types) anymore you are just importing individual functions and then putting them in an object to create your own module to pass to spyOn.

For me the fix was to just import the file directly rather from the index as the index also exported types but the file actually didn't.

Bit of an old thread but hopefully this will help someone.

hyalen commented 11 months ago

This also works if you need all methods:

import * as fooMethods from "nice-grpc";
const foo = {...fooMethods};

jest.spyOn(foo, "createClient").mockReturnValue({});
jest.spyOn(foo, "createChannel").mockReturnValue({});

This solution worked for me, thanks a lot!

joel-daros commented 10 months ago

Is there any solution for when spyOn returns a method and I need to check if the method has been called with proper parameters:

import * as fooMethods from "hooks/useGenerateReport";
const foo = {...fooMethods};

it("testing", () => {
  const mockMutate = jest.fn();

  jest.spyOn(foo, "useGenerateReport")
      .mockImplementation(
        () =>
          ({
            mutate: mockMutate,
            isError: false,
          }) 
      );

  // this line fail, because mockMutate is never called using suggested approaches
  expect(mockMutate).toHaveBeenCalledWith({});
});

Unfortunately I can’t use mock() for this situation, because I need to mock this method for a single test. AFAIK using mock() the module is mocked for the entire test suite, and there is no way to unmock it for specific tests.

baublet commented 7 months ago

Revisiting this in 2024 for those who still face this problem. This doesn't appear to be an issue esbuild-jest can address. The esbuild crew don't seem to be in any rush to add this functionality to support loose CJS compilation. The reasons are two-fold:

  1. Immutable exports are the specification. Loose common JS exports are an artifact of a generation of transpilers and tooling either not conforming to spec or doing their own thing. Fact is, it's more secure to enforce module immutability as broadly as possible. This is a choice Evan and the crew have made. (Same as the SWC team.)
  2. The issue in 2024 is mostly in test environments, where we frequently need to mock, spy, monkey patch, and do other violence to the module system to test our code.

Ways around this are in this thread. The tl;dr: as of January 18, 2024, you have to manually update your tests and/or code to support this.


@joel-daros there are a lot of ways to work around your particular issue. You can always .mockImplementationOnce, or you can use a beforeEach function to reset your mocks between tests:

import * as fooMethods from "hooks/useGenerateReport";
const foo = {...fooMethods};

beforeEach(() => {
  Object.assign(foo, fooMethods);
});
b-rad-is-rad commented 3 months ago

Just wanted to drop our problem and solution for this, encountered just this week in May 2024.

Switching a large production application from CRA / Jest to Vite and Vitest so that we can upgrade to React 18 and utilize a new library we need.

We had several hundred test files where we would do essentially:

const myInstance = new MyClass()
myInstance.someMethod = vi.fn()

This would yield the error TypeError: Cannot assign to read only property 'someMethod' of object '#<MyClass>'

Attempting to use Object.defineProperty() to set writable to true yielded the error TypeError: Cannot redefine property: someMethod

Turns out, defining methods in a class with arrow functions automatically sets writable to false. Defining methods in a class with the older function syntax sets writable to true. This was discovered essentially on accident when we realized some tests involving classes were failing and others were not. We realized the common denominator was arrow function vs. function syntax.

So

export class SomeClass {
    ...
   myMethod = () => {
    // do stuff
  }
}

becomes

export class SomeClass {
    ...
   myMethod() {
    // do stuff
  }
}

After some regex'ing and Vim macros I was able to get several hundred methods converted over to function syntax across about 20 files. Took about an hour and then magically all tests were passing. This was much easier to do than trying to re-write hundreds of test files via a script or something (which probably wouldn't be possible anyhow and ultimately be a manual process that the team has to take on).

I did some diving into why this is the case, but honestly found nothing and it isn't worth diving into ECMAScript 6 to find the answer. My hunch is that is has to do with arrow functions being set as direct properties on the instance vs. a class method on the class prototype, but who knows.

Hope this helps someone somewhere!