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

Adding support for mocking interfaces #76

Open
wants to merge 6 commits into
base: interfaces-mocking
Choose a base branch
from

Conversation

johanblumenberg
Copy link
Contributor

@johanblumenberg johanblumenberg commented Jan 5, 2018

This commit adds support for mocking interfaces, like this:

interface Foo {
  bar(value: string): string;
}

let mockedFoo: Foo = imock();
let foo: Foo = instance(mockedFoo);

when(mockedFoo.bar('a')).thenReturn('b');

@codecov-io
Copy link

codecov-io commented Jan 5, 2018

Codecov Report

Merging #76 into master will decrease coverage by 1.34%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage    94.9%   93.56%   -1.35%     
==========================================
  Files          34       34              
  Lines         609      653      +44     
  Branches       70       81      +11     
==========================================
+ Hits          578      611      +33     
- Misses         22       28       +6     
- Partials        9       14       +5
Impacted Files Coverage Δ
src/Spy.ts 80.55% <100%> (ø) ⬆️
src/utils/MethodCallToStringConverter.ts 100% <100%> (ø) ⬆️
src/MethodStubVerificator.ts 100% <100%> (ø) ⬆️
src/MethodToStub.ts 100% <100%> (ø) ⬆️
src/ts-mockito.ts 92.75% <75%> (-3.97%) ⬇️
src/Mock.ts 91.55% <81.39%> (-4.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1c7f69...a1baf00. Read the comment docs.

@NagRock
Copy link
Owner

NagRock commented Jan 10, 2018

😮 @johanblumenberg this looks great! Interfaces mocking feature has been long awaited. I only got to think about imock function name and add readme.
Thanks for contributing!

const mockedValue = new Mocker(Empty, true).getMock();

if (typeof Proxy === "undefined") {
return mockedValue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should throw Error here to get know user that interface mocking is not available without Proxy support. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right

@johanblumenberg
Copy link
Contributor Author

@NagRock I added an exception if calling imock() with no Proxy support

@Markus-Ende
Copy link
Contributor

This only supports stubbing methods, am I right? I also tried something like this some time ago but failed to differentiate between properties and method calls.

@johanblumenberg
Copy link
Contributor Author

johanblumenberg commented Jan 15, 2018

@Markus-Ende Yes, only stubbing methods. That's why I made a separate function, imock(), because it works slightly different.

But it's still quite useful to be able to automatically mock interfaces, even though you cannot mock properties on interfaces.

@ilbonte
Copy link

ilbonte commented Feb 19, 2018

Hi, what is blocking this PR? Is there anything I can help with?

@jjordan-dff
Copy link
Contributor

jjordan-dff commented Feb 23, 2018

Looks useful. But I think there should be a type parameter for imock(). so the return types can be defined (just like when using mock()).

let mock = imock<Foo>(); // mock: Foo

let foo = instance(mock); // foo: Foo

EDIT: Sorry, I didn't see there already is a type parameter.

@johanblumenberg
Copy link
Contributor Author

@NagRock Is there anything blocking this PR?

@perransw
Copy link

any update on this PR? will it be merged soon?

@kwolfy
Copy link
Contributor

kwolfy commented Apr 11, 2018

+1, when will it merged ?

@drenner-netspend
Copy link

+2 on merging

@NagRock any ideas when this will be merged and available?

@perransw
Copy link

@johanblumenberg maybe add usage examples to the readme as well?

@johanblumenberg
Copy link
Contributor Author

johanblumenberg commented May 22, 2018

Fixed a problem when retuning mock instances in thenResolve():

The problem is that the Promise.resolve() call will check if the given value is a promise, by checking if it has a function property named then. If so, it believes that the value is a promise, and will wait for it to resolve before returning.

A mock object will intersect all unknown properties and return a stub function, so it will look like a promise to the promise library.

(test "resolves with given mock value" in stubbing.method.spec.ts)

@johanblumenberg
Copy link
Contributor Author

Added support for mocking both properties or methods on interfaces.

This will make all properties on the object to be stubbed as properties:

let mock: Foo = imock(MockPropertyPolicy.StubAsProperty);
when(mock.foo).thenReturn('xyz');

This will make all properties on the object to be stubbed as methods:

let mock: Foo = imock(MockPropertyPolicy.StubAsMethod);
when(mock.foo()).thenReturn('xyz');

For classes, the same behavior is kept. It will look at the class to decide which properties are methods and which are properties. The new policy applies to unknown properties and all properties on interfaces.

@johanblumenberg
Copy link
Contributor Author

Added support for mocking interfaces containing both properties and methods.
The default policy in the previous commit decides what happens with unknown properties.
The type of a property can be set by setting up an expectation on the property.

let mock: Foo = imock();

when(mock.method()).thenReturn(1); // Here it is decided that 'method' is a method
when(mock.property).thenReturn(2); // Here it is decided that 'property' is a property

This must be done before the mock instance is used. If not, the default property policy is used to decide if the property is a property or a method.

@jensbodal
Copy link

Is there an alternative way to mock an interface like this at this point? (without casting to any)

@johanblumenberg
Copy link
Contributor Author

ping

@kouros51
Copy link

kouros51 commented Dec 27, 2018

Is there any updates on this, this is a blocker for us to use ts-mockito, specially because we have to implement interface to mock them. Can you please merge this soon, I've waiting for the this feature for so long, thank you for this awesome ts mocking lib.

@johanblumenberg
Copy link
Contributor Author

johanblumenberg commented Dec 28, 2018

@kouros51, @DonJayamanne, @cjanietz, @drenner-netspend, @kwolfy, @perransw, @Markus-Ende

I published a forked package, that contains the possibility to mock interfaces: https://www.npmjs.com/package/@johanblumenberg/ts-mockito

It has support for mocking methods and properties on interfaces.

@johanblumenberg
Copy link
Contributor Author

Pushed another commit with updated README, describing imock()

@kouros51
Copy link

@johanblumenberg thanks for that. This was very helpful.

@aserra54
Copy link

aserra54 commented Jan 6, 2019

@johanblumenberg

I tried your build, and I was having issues with mocking properties defined in interfaces. For example:

interface Foo {
  someProperty: string;
}

describe('Tests', () => {
  it('Should work', () => {
    const fooMock: Foo = imock();
    when(fooMock.someProperty).thenReturn('hello');
    const foo = instance(foo);
    expect(foo.name).to.equal('hello');
  });
});

I rewrote this test from memory, I don't have the machine that ran this test, but you get the general gist of it. The explicit error was something like "could not reference 'add' property of undefined". I can't remember if the error occurs from the when clause or the expect clause.

Has anyone else seen this?

@aserra54
Copy link

aserra54 commented Jan 6, 2019

Hm... never mind. I tried to reproduce it locally with a much smaller sample, and it worked fine. The below code passes just fine:

import { imock, instance, when } from '@johanblumenberg/ts-mockito';
import { expect } from 'chai';

interface Foo {
  someProperty: string;
}

describe('Test', () => {
  it('Should work', () => {
    const fooMock: Foo = imock();
    when(fooMock.someProperty).thenReturn('hello!');
    const foo = instance(fooMock);
    expect(foo.someProperty).to.equal('hello!');
  });
});

@aserra54
Copy link

@NagRock Is this repository still maintained? This review was opened over a year ago, and despite being incredibly useful, hasn't been merged into master.

Essentially, I work on a project that heavily requires mocks, and we opted to go with ts-mockito over typemoq. Is it worth it to continue to wait for ts-mockito to merge this feature (among others in), or should we start looking at migrating to a new framework?

@johanblumenberg
Copy link
Contributor Author

@aserra54 I published my fork as a package on npm: https://www.npmjs.com/package/@johanblumenberg/ts-mockito

Feel free to use it, or to suggest other PR's that I should add to it.

@NagRock
Copy link
Owner

NagRock commented Mar 18, 2019

Unfortunately this PR doesn't support getters / setters mocking. Thats the reason why this is not merged.

So this will break some functionalities or we will have to remove getters / setters feature. This will also break existing projects that uses this functionality.

@NagRock NagRock closed this Mar 18, 2019
@NagRock
Copy link
Owner

NagRock commented Mar 18, 2019

@NagRock Is this repository still maintained? This review was opened over a year ago, and despite being incredibly useful, hasn't been merged into master.

There is no good solution for interface mocking (or maybe there is but I dont have it). One will break current API, and this one will break getters / setters functionality.

@DonJayamanne
Copy link

DonJayamanne commented Mar 18, 2019

Thanks for closing this PR, at least we have a resolution.

@johanblumenberg
Copy link
Contributor Author

and this one will break getters / setters functionality.

@NagRock could you give an example where this PR is breaking getters / setters?
All unit tests are passing, so this would be untested functionality?

@DonJayamanne
Copy link

Agreed, would be good to provide technical insight for someone wanting to doing submit another PR to address the concerns, now it in the future....
Would be good to have tests for the areas that can potentially break with the current solution.

@NagRock
Copy link
Owner

NagRock commented Mar 19, 2019

@johanblumenberg Sorry I will check it once again and add specs for getters and setters into your PR. Looks like it can be my mistake.

@DonJayamanne
Copy link

Should we reopen this PR?

@NagRock NagRock changed the base branch from master to interfaces-mocking March 20, 2019 19:47
@NagRock NagRock reopened this Mar 20, 2019
@NagRock
Copy link
Owner

NagRock commented Mar 29, 2019

@johanblumenberg can you explain what is MockPropertyPolicy?

@johanblumenberg
Copy link
Contributor Author

The purpose of MockPropertyPolicy is this:

When creating a mock object from an interface, a Proxy object is set up to return mocked properties from the object.
Since interfaces is a compile time construct of typescript only, this proxy doesn't know anything about the properties that it is supposed to mock.

So when the proxy should return a property, it has to know if it is supposed to return a property or a function mock.

One way to tell the proxy object what to do is to set up an expectation for that call. For example:

when(mock.foo).thenReturn(5); // Now foo is a property
when(mock.foo()).thenReturn(5); // Now foo is a method

The code will be checked in compile time, so only the correct signature is possible. One of the above will give a compile error.
The proxy is smart enough to see that the property was invoked as a property or as a method, and then knows how the mock should behave.

The problem is when there are properties that don't have any expectations set. Now the proxy doesn't know how to handle that. Typically it would either handle it as a property, returning null as the default value, or it would handle it as a method, returning a function that returns null.

The MockPropertyPolicy tells the mock how to handle unknown properties. It can either handle them as properties, as methods, or it can be set to throw an exception whenever an unknown property is accessed.

The first two are really only a workaround because all information about interfaces is lost in runtime. If there is any other way to fix it, that would be better.
The third option, to throw, is useful if you really want to control that your code is only accessing the functions or properties that you have set up expectations from, but nothing else.

when(mockedFoo.bar).thenReturn('five');
```

For interface mocks, you can set the defauklt behviour for mocked properties that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For interface mocks, you can set the defauklt behviour for mocked properties that
For interface mocks, you can set the default behavior for mocked properties that

@DonJayamanne
Copy link

@NagRock

Thanks for maintaining this awesome library.
I know managing such open source libraries can take a lot of time. So thanks.
Out of curiosity what holding this PR.
would be great to provide some feedback to the author and to the OSS community about the status of this PR.
As you can understand a number of people are using this library and need this feature, of there's a PR that adds this feature we should try to get it in, or at the least provide some more information on the status of the PR, or just reject it all together. Right now we're all in a state of limbo.

@aserra54
Copy link

I integrated Johan Blumenberg's release into our test setup, and while it generally works great, we've had issues where instance returns a null when given a mock object returned by imock.

Unfortunately, the issue reproduces consistently if I run our entire test suite, but if I take the same interface (which is actually a standalone interface) and put it in a small test project to try and reproduce the issue, it works fine.

I think the philosophy behind the idea of imock is solid, and I see no reason not to integrate it into ts-mockito, but I think there's also a gap in testing ts-mockito as a whole, and possibly this is why there's some hesitation to integrate it.

@DonJayamanne
Copy link

@aserra54 Would be good to provide some steps to repro the issue.

@NagRock
Copy link
Owner

NagRock commented Jul 20, 2019

New version with support for interfaces released: https://github.com/NagRock/ts-mockito/releases/tag/v2.4.0

Great thanks @johanblumenberg for this PR that pushed me forward with final implementation. And sorry that this took so long time.

If something works wrong, please open an issue.

@rennomarcus
Copy link

Should this be closed now, @NagRock ?

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

Successfully merging this pull request may close these issues.

None yet