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

Generic type constraint for ngMocks.stub tedious to write #166

Closed
coyoteecd opened this issue Jul 15, 2020 · 15 comments · Fixed by #189
Closed

Generic type constraint for ngMocks.stub tedious to write #166

coyoteecd opened this issue Jul 15, 2020 · 15 comments · Fixed by #189

Comments

@coyoteecd
Copy link

coyoteecd commented Jul 15, 2020

If I understand it correctly, to stub a service MyService and provide some stubbed implementations, I should be using something like this:

{ 
  provide: MyService, 
  useValue: ngMocks.stub(MyService, { serviceMethod: ... })
}

Problem is, the type definition of stub is too relaxed:

stub<I extends object, O extends object>(instance: I, overrides: O): I & O;

This means overrides parameter has no relation with type I. Consequently, if I rename serviceMethod later, the test still compiles, which is something I do not like. Most common case is to stub a service and override some of its methods; actually I find it pretty hard to find a use case when I would override the service implementation with different methods.

Proposal:
A) constrain "stub" to Partial<I>. This breaks backwards compatibility, so I guess it's not desirable.
B) add an overload that is more constrained:

stubExact<I extends object>(instance: I, overrides: Partial<I>): I

Thoughts? I can send a pull request with the change, should be a small one.

Edit:
Right now I'm specifying the generic types explicitly, which is pretty verbose when the service name is a bit longer:

{ 
  provide: MyService, 
  useValue: ngMocks.stub<MyService, Partial<MyService>(MyService, { serviceMethod: ... })
}
@satanTime
Copy link
Member

satanTime commented Jul 15, 2020

Hi, it provides instances.

In your case it should be like that:

{ 
  provide: MyService, 
  useValue: ngMocks.stub(MockService(MyService), { serviceMethod: ... })
}

Partial<I> won't work because you can stub anything as you want, change args, result etc. I'll check the case with I & O and an unknown declaration.

@coyoteecd
Copy link
Author

Yeah you're right, I will edit the comment. Read along, the point is about typings

@satanTime
Copy link
Member

satanTime commented Jul 15, 2020

Might you share the test you have for better understanding of MyService structure and its role in the test?

@coyoteecd
Copy link
Author

coyoteecd commented Jul 15, 2020

I have a component test and I'm stubbing the service.
Some of the services' methods are returning Observables, for which MockService defaults to undefined.
Now, the component under test is trying to subscribe to those observables and it breaks because of the undefined, so I am overriding them to return a default value. It works fine, except for the lack of type safety. Something like this:

      providers: [
        { provide: MyService,
          useValue: ngMocks.stub<MyService, Partial<MyService>>(
            MockService(MyService), {
              onSuccessSet$: of(''),
              onWarningSet$: of(''),
              onErrorSet$: of('')
          })
        }
      ]

Basically what I need is a variant of stub that doesn't let me override methods which are not defined in MyService, without the hassle of repeating the generic type constraints every time.

@satanTime
Copy link
Member

Thanks, got it, I'll take a look on this / next weekend to find a proper solution with the idea to ensure it works correctly with spies and mocks from jasmine and jest.

@coyoteecd
Copy link
Author

Maybe the difference is that I am setting up spies separately for each test and use stub only to provide a default implementation.

@satanTime
Copy link
Member

satanTime commented Jul 15, 2020

If you set them separately I think you can leave MockService(MyService) only.

.stub derived from an ability to manage methods and properties that are hard to access to, for example private or even undeclared.

ngMocks.stub(obj, 'test', 'get') and then to use spyOnProperty(obj, 'test', 'get').and.returnValue(123).
or ngMocks.stub(obj, {newMethod: () => true}) for custom logic.

@coyoteecd
Copy link
Author

I am setting spies separately, but not for all the methods in all the tests. To be precise, I mock the service and provide empty responses for all the observables, so that whatever a component is doing, won't crash because of .subscribe to undefined. Then:

  • test 1 spies on method1, maybe provide some relevant test data
  • test 2 spies on method2
  • test 3 might spy on both
    etc. (to keep tests small so their it-description is relevant)

As for your examples of using stub(), I think they're probably a different use-case. I am stubbing a service that's a dependency of the component under test. Naturally, the component uses only the public surface of the service, therefore why would I stub a method that the component doesn't know about?

@satanTime
Copy link
Member

satanTime commented Jul 15, 2020

Are they methods or properties? if they are methods it should be like onSuccessSet$: () => of(''),.

The problem with stub of methods if that not always Spy or Mock fits exactly signature of the method, and to facilitate it .stub doesn't limit types.

@coyoteecd
Copy link
Author

coyoteecd commented Jul 15, 2020

Are they methods or properties? if they are methods it should be like onSuccessSet$: () => of(''),.

In this particular example they're just fields. But in other cases they can be methods or properties.

The problem with stub of methods if that not always Spy or Mock fits exactly signature of the method, and to facilitate it .stub doesn't limit types.

Thus option B, provide a variant that is more constrained...
As mentioned above, I can get around it by specifying the type arguments, but it's just noisy and verbose. And I'd rather not fork a copy just for a slight inconvenience.

@satanTime
Copy link
Member

satanTime commented Sep 26, 2020

Hi again,

might you check if this behavior fits your expectations, you can use ngMocks.stub?
ng-mocks.zip

@coyoteecd
Copy link
Author

Sorry for the late reply. I checked your modification (now included in master) and it's not doing what I needed. Type definition currently is:

stub<I extends object, O extends Partial<I>>(instance: I, overrides: O): I & O;

This constrains overrides a little, but not to the extent that specifying the generic type arguments does. Case in point:

providers: [
        { provide: Router, useValue: ngMocks.stub(MockService(Router), {
            events: routerStubEvent.asObservable(),
            createUrlTree() { return new UrlTree(); },
            serializeUrl1() { return 'fake'; },
          })
        }
]

compiles just fine, even though serializeUrl1 is not defined in Router, whereas:

providers: [
        { provide: Router, useValue: ngMocks.stub<Router, Partial<Router>>(MockService(Router), {
            events: routerStubEvent.asObservable(),
            createUrlTree() { return new UrlTree(); },
            serializeUrl1() { return 'fake'; },
          })
        }
]

gives a compile-time error. In real life, this happens when Router is a service in your code and somebody renames one of its methods, I want the compiler to complain that I'm mocking a method that's no longer present.

I'm not sure how you can do this without breaking backwards compatibility, therefore I suggested a stubExact method that does this.

@satanTime
Copy link
Member

Hi again,

that's fine, might you try changes in #203?

@coyoteecd
Copy link
Author

Yup, those changes are doing exactly what I need.

@satanTime
Copy link
Member

great, will be released on Sunday.

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 a pull request may close this issue.

2 participants