NagRock / ts-mockito

Mocking library for TypeScript
MIT License
977 stars 93 forks source link

thenResolve() with a mocked object not working #191

Open jlkeesey opened 4 years ago

jlkeesey commented 4 years ago

This was a "fun" one (if three days of pulling my hair out is fun, but I can't get a haircut so I guess that's OK :-). I have an interface that has a method that returns a promise of another interface which is also mocked. When using thenResolve() (or thenReturn() with Promise.resolve()) any await on the promise fails to resolve. It works if any non-mocked object or primitive type is used.

So given (greatly simplfied):

interface Main {
  getChild(): Promise<Child>;
}

interface Child {
  name: string;
}

and then using this test (Mocha and Chai):

describe("mocking", function() {
  it("Should work", async function() {
        const mockedChild = mock<Child>();
        when(mockedChild.name).thenReturn("Sylvia");
        const child = instance(mockedChild);

        const mockedMain = mock<Main>();
        when(mockedMain.getPromise()).thenResolve(child);
        const main = instance(mockedMain);

        const result = await main.getPromise();  // <-- Here be dragons, this times outs
        console.log(`@@@@ val = '${result}'`);
        expect(result.name).to.equal("Sylvia");
  });
});

The test times out at the indicated line. I set the timeout to 10 seconds and it still times out.

Normally at this point I'd paste in more code, like the package.json etc. Or I'd create a separate, simplified project (which I did) and link it here (I can if you really want it) but I was so aggravated that I obviously didn't understand what I was doing that I decided to figure out what I was doing wrong. It turns out, I wasn't doing anything wrong, it was a slight by-product of the design of ts-mockito. I won't call it a bug as everything is working exactly as designed. It's more of an oversight of the subtleties of JavaScript and Promises.

BTW, one of the reasons I decided to figure it out on my own was because of another "bug" I'm going to report after this one pertaining to toString(). Another "fun" issue that made this one much harder for me.

So, what went "wrong." If you read the Promise.resolve() description (I'm using the Mozilla one here because it's easier to understand but I did read the JavaScript Spec and it says the same thing just using waaaaay more words) it says in part:

The Promise.resolve() method returns a Promise object that is resolved with a given value. If the value is a promise, that promise is returned; if the value is a thenable (i.e. has a "then" method), the returned promise will "follow" that thenable, adopting its eventual state; otherwise the returned promise will be fulfilled with the value. This function flattens nested layers of promise-like objects (e.g. a promise that resolves to a promise that resolves to something) into a single layer.

Promise.resolve() documentation

The bold is the interesting part. It says that it takes the resolve value, sees if it has a .then() method and if it does then uses the .then() method to resolve the value of the promise. Of course, because this is a mock of everything all of the objects that ts-mockito creates has a .then() method. Ruh-roh!

OK, now that is the problem. For bonus points, I have a solution. I think it's right, and it does pass all the tests. It is in the attached PR. I have hand-patched my copy of ts-mockito and it works for me.

The basic solution is to have a list of what I'm calling defaultedPropertyNames which are properties that, if they are not explicitly set to return a value, will return undefined in the Proxy.get method. This is similar to the excludedPropertyNames in the .get() method that contains the hasOwnProperty value so we don't override the one value we must have from JavaScript. The only difference is that my check comes after to check for a defined value for the property so if a when() is defined, that will be used.

BTW, in the PR there is another name in the defaultedPropertyNames list, Symbol(Symbol.toPrimitive) and that is to solve the next bug I'm opening, the fix was that same so I made them at the same time.

jlkeesey commented 4 years ago

PR is #192

jlkeesey commented 4 years ago

OK, I had to go away for the weekend, but now I'm back and my fix does not seem to be working. I will look into it and get back.

jlkeesey commented 4 years ago

OK, I figured out what went wrong and I had placed a test in the wrong place, so here we are again. The new PR is #194.

joelmeaders commented 4 years ago

I just ran into this issue today! If this PR is good I would love to see it merged.

NagRock commented 4 years ago

Hey @jlkeesey Thanks for finding this and for the PR! Looks nice. I will play with it a bit and merge if not find issues.

ianw11 commented 4 years ago

I just ran into this issue today, wondering if there's a likelihood that this and #194 will get merged into a release soon?

MOHACGCG commented 4 years ago

ran to this issue as well. thanks for the PR @jlkeesey! looking forward to the merge by @NagRock :)

jamesharv commented 4 years ago

Looking forward to this fix getting merged.

In the interim I am working around this problem by using a custom wrapper for the instance() function (resolvableInstance()), which wraps the ts-mockito Proxy in a new Proxy that returns undefined for the Promise interface methods that are causing the problem.

See my implementation below:

import { instance } from "ts-mockito";

export const resolvableInstance = <T extends {}>(mock: T) => new Proxy<T>(instance(mock), {
  get(target, name: PropertyKey) {
    if (["Symbol(Symbol.toPrimitive)", "then", "catch"].includes(name.toString())) {
      return undefined;
    }

    return (target as any)[name];
  },
});

Usage example:

import { resolvableInstance } from "./resolvableInstance";
import { expect } from "chai";
import { mock } from "ts-mockito";

class Service {
  public execute(): boolean {
    return false;
  }
}

class ServiceFactory {
  public async getService(): Promise<Service> {
    return new Service();
  }
}

describe("My test", () => {
  const serviceFactory = mock<ServiceFactory>();
  const service = mock<Service>();

  it("Can resolve a promise with a mock", () => {
    const serviceFactoryInstance = instance(serviceFactory);
    const serviceInstance = resolvableInstance(service);

    when(serviceFactory.getService()).thenResolve(serviceInstance);
    when(service.execute()).thenReturn(true)

    // This line hangs if serviceInstance was created with instance() rather than resolvableInstance().    
    const resolvedService = serviceFactoryInstance.getService();
    expect(resolvedService.execute()).to.eq(true);
  });
});
algono commented 4 years ago

@jamesharv Your workaround works like a charm, thanks! Hoping for a more permanent solution soon, this issue is quite troublesome.

nickzelei commented 4 years ago

Thank you @jamesharv - this fixed my issue. Was starting at my code for a long while trying to figure out why my test wouldn't resolve. I've used ts-mockito for years now and was unsure what I was doing wrong. Will keep an eye out on this issue as it seems like a pretty big regression.

shaungrady commented 3 years ago

Would love to see this addressed soon.

HyperCharlie commented 3 years ago

Just ran into this issue. I tried @jamesharv's solution with resolvableInstance, but it doesn't seem to work; I still get "undefined" as the result from .thenResolve(resolvableInstance(mock()))

MasDenBy commented 3 years ago

The solution from @jamesharv works for me. Thank you!!!

ThangHuuVu commented 3 years ago

Thank you @jamesharv, your solution works great for me. The only thing I would change is to use Reflect to return the original behavior instead of return (target as any)[name]

export const resolvableInstance = <T extends object>(mock: T): T =>
    new Proxy<T>(instance(mock), {
        get(target, prop, receiver) {
            if (["Symbol(Symbol.toPrimitive)", "then", "catch"].includes(prop.toString())) {
                return undefined;
            }

            return Reflect.get(target, prop, receiver);
        }
    });
hallsamuel90 commented 3 years ago

Just hit this today as well, anyone still working on this?

dcofer-sterling commented 3 years ago

+1 for @jamesharv solution. Thank you.

mikehowson commented 3 years ago

Any plans fix this?

MalouNetlight commented 2 years ago

Just stumble accross this issue, any plans to fix that?

dariosn85 commented 2 years ago

Alternative workaround to what @jamesharv described here is to monkey patch MockableFunctionsFinder. The advantage of this approach is that you keep your test code unchanged (no need for additional method calls like resolvableInstance() or similar).

What you need to do is to simply call following code before running all tests:

        try {
            const MockableFunctionsFinder = require('ts-mockito/lib/utils/MockableFunctionsFinder').MockableFunctionsFinder;
            if (!MockableFunctionsFinder.prototype.isMockableOriginal) {
                MockableFunctionsFinder.prototype.isMockableOriginal = MockableFunctionsFinder.prototype.isMockable;

                MockableFunctionsFinder.prototype.isMockable = function (name: string): boolean {
                    if (['catch', 'then'].indexOf(name) >= 0) {
                        return false;
                    }

                    return this.isMockableOriginal(name);
                };
            }
        } catch (error) {
            console.warn('Failed to patch ts-mockito MockableFunctionsFinder', error);
        }

For example, we are using Jest, so we put this patch simply into jest.setup.ts file.

Original post about that and additional information is here.

nielssc commented 2 years ago

I just came across this problem. Any news?

RomanReznichenko commented 2 years ago

I just came across this problem. Any news?

The same thing. @NagRock do you plan to fix it?

GuiSim commented 2 years ago

@NagRock if this project is dead, could you give someone else the keys? I'd love to get this fix.

alejandrotineo commented 2 years ago

Thank you @jamesharv for the solution. I did notice that when using the resolvable instance method, you loose the ability to verify execution / capture params on the mock.

For example:

verify(mock.doSomething(anything()).once();

or

const [params] = capture(mock.doSomething).last()

Has anyone stumbled upon this?

bhleb commented 10 months ago

@alejandrotineo Yes, I faced the same issue after adopting @jamesharv solution