ts-mockito icon indicating copy to clipboard operation
ts-mockito copied to clipboard

thenResolve() with a mocked object not working

Open jlkeesey opened this issue 4 years ago • 24 comments

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 avatar Jun 20 '20 01:06 jlkeesey

PR is #192

jlkeesey avatar Jun 20 '20 01:06 jlkeesey

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 avatar Jun 24 '20 18:06 jlkeesey

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.

jlkeesey avatar Jun 24 '20 22:06 jlkeesey

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

joelmeaders avatar Jul 01 '20 16:07 joelmeaders

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.

NagRock avatar Jul 03 '20 08:07 NagRock

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

ianw11 avatar Aug 23 '20 01:08 ianw11

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

MOHACGCG avatar Sep 21 '20 19:09 MOHACGCG

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

jamesharv avatar Oct 15 '20 00:10 jamesharv

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

algono avatar Oct 27 '20 22:10 algono

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.

nickzelei avatar Nov 25 '20 20:11 nickzelei

Would love to see this addressed soon.

shaungrady avatar Jan 21 '21 17:01 shaungrady

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<IFoo>()))

HyperCharlie avatar Jan 21 '21 19:01 HyperCharlie

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

MasDenBy avatar Feb 03 '21 12:02 MasDenBy

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

ThangHuuVu avatar Mar 23 '21 07:03 ThangHuuVu

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

hallsamuel90 avatar Jun 02 '21 02:06 hallsamuel90

+1 for @jamesharv solution. Thank you.

dcofer-sterling avatar Jul 21 '21 00:07 dcofer-sterling

Any plans fix this?

mikehowson avatar Sep 02 '21 15:09 mikehowson

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

MalouNetlight avatar Jan 24 '22 16:01 MalouNetlight

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.

dariosn85 avatar Jan 28 '22 10:01 dariosn85

I just came across this problem. Any news?

nielssc avatar Mar 25 '22 20:03 nielssc

I just came across this problem. Any news?

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

RomanReznichenko avatar Mar 28 '22 14:03 RomanReznichenko

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

GuiSim avatar Aug 19 '22 19:08 GuiSim

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?

alejandrotineo avatar Sep 27 '22 14:09 alejandrotineo

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

bhleb avatar Jan 19 '24 02:01 bhleb