Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

thenResolve() with a mocked object not working #191

Open
jlkeesey opened this issue Jun 20, 2020 · 24 comments
Open

thenResolve() with a mocked object not working #191

jlkeesey opened this issue Jun 20, 2020 · 24 comments

Comments

@jlkeesey
Copy link
Contributor

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
Copy link
Contributor Author

PR is #192

@jlkeesey
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

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

@NagRock
Copy link
Owner

NagRock commented Jul 3, 2020

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
Copy link

ianw11 commented Aug 23, 2020

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
Copy link

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

@jamesharv
Copy link

jamesharv commented Oct 15, 2020

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
Copy link

algono commented Oct 27, 2020

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

@nickzelei
Copy link

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
Copy link

Would love to see this addressed soon.

@HyperCharlie
Copy link

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
Copy link

MasDenBy commented Feb 3, 2021

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

@ThangHuuVu
Copy link

ThangHuuVu commented Mar 23, 2021

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
Copy link

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

@dcofer-sterling
Copy link

+1 for @jamesharv solution. Thank you.

@mikehowson
Copy link

Any plans fix this?

@MalouNetlight
Copy link

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

@dariosn85
Copy link

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
Copy link

nielssc commented Mar 25, 2022

I just came across this problem. Any news?

@RomanReznichenko
Copy link

RomanReznichenko commented Mar 28, 2022

I just came across this problem. Any news?

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

@GuiSim
Copy link

GuiSim commented Aug 19, 2022

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

@alejandrotineo
Copy link

alejandrotineo commented Sep 27, 2022

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
Copy link

bhleb commented Jan 19, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests