Mocks at the service level

[mshustov]

Motivation

With introducing more and more core services we see the problem that writing unit test requires manual mocking of plenty of dependent services.
Ideally, we want that all mocks were kept up to date automatically and any interface change was reflected across codebase immediately.

Proposal:

• Each service defines its set of mocks and allocates them inside its folder.

└─ server
   └─ http
       ├─ http_service.mock.ts
       ├─ http_service.tests.ts
       └─ http_service.ts

• A mock factory exposes public methods only.

const createHttpServiceMock = (): jest.Mocked<PublicMethodsOf<HttpService>> => ({
  start: jest.fn(),
  stop: jest.fn(),
  registerRouter: jest.fn(),
});

export const httpServiceMock = { create: createHttpServiceMock };

• When writing test and want to mock our service we import predefined mock and use it instead of writing it manually

-const mockHttpService = { start: jest.fn(), stop: jest.fn(), registerRouter: jest.fn() };
+ import { httpServiceMock } from './http/http_service.mock';
+ const mockHttpService = httpServiceMock.create();
jest.mock('./http/http_service', () => ({
  HttpService: jest.fn(() => mockHttpService),
}));
...
expect(mockHttpService.start).not.toHaveBeenCalled();

Caveats

• [Update] Fixed in Update jest to v24 #31825, Update Typescript to the latest version #32063 It's not possible to have proper type checking for jest.MockInstance methods with current @types/jest version. Proper support was added in @types/jest v24:
  
  

• [Updated] For now we can use node scripts/type_check.js. The current jest setup doesn't report type-checking errors. Requires additional work: Type error reporting kulshekhar/ts-jest#79, How to fail all tests or exit jest when there are TSC errors? kulshekhar/ts-jest#250

• there is no such thing as a public interface of a service. thus we have to suppress types when passing in another service
  https://github.com/elastic/kibana/blob/master/src/core/server/legacy_compat/legacy_service.test.ts#L64-L69
  https://github.com/elastic/kibana/blob/master/src/core/server/index.test.ts#L66

Action points

• decide if we want to control mocking process manually (as done here https://github.com/restrry/kibana/tree/issues-31486-mock-service-level) or automatically (as here https://github.com/elastic/kibana/blob/master/src/core/server/plugins/plugins_service.test.ts#L65)
• decide if we use factory or use object literal as a mock declaration(can it lead to sharing state between different tests?)
• decide if services should declare their dependencies on interface only. Moved to a separate issue Mock ES6 Classes in New Platform #33396
• decide if we should provide additional helpers out-of-the-box, like reset mocks, get call stat and others from https://github.com/elastic/kibana/blob/master/src/core/server/logging/__mocks__/index.ts#L22-L58
• implement mocks for http, plugin, elasticsearch, legacy, logging and config services
• update @types/jest version to have type-checking for jest methods (mockImplementation, mockReturnValue etc.) Update jest to v24 #31825
• update typescript to the latest version (to be compatible with updated @types/jest version) Update Typescript to the latest version #32063
• make tests fail if there are type errors in tests node scripts/type_check.js is enough for now

Other alternatives to consider

• https://github.com/kulshekhar/ts-jest/blob/master/docs/user/test-helpers.md#mocked

[elasticmachine]

Pinging @elastic/kibana-platform

[epixa]

I think this is a good start. I don't really have opinions about all of the action points and such, so I'm only highlighting those that I do have thoughts about.

Each service defines its set of mocks and allocates them inside appropriate __mocks__ folder.

We currently put jest tests as siblings to the service, does it make sense to do the same for mocks rather than a separate directory? Something like:

└─ server
     └─ http
         ├─ http_service.ts
         ├─ http_service.mock.ts
         └─ http_service.test.ts

decide if we okay with current minimal functionality. examples for http, config and logging services

I do think we should provide sensible return values out of the box though. Tests can change them if necessary, but if I'm invoking a function in my service that happens to check for mockService.config().someDefaultConfig, I wouldn't want that to fail my test until I figured out how to import and instantiate Config and then seed it with some defaults that make sense for the mocked service.

That said, the minimal functionality you propose here is a big step up from what exists today. I'd be happy to start with that if the return value thing proves to be impractical.

decide if we use factory or use object literal as a mock declaration(can it lead to sharing state between different tests?)

I'm a big fan of factories for test setup. Without them, I've in seen in the Kibana project developers accidentally leak memory and accidentally introduce state into their tests.

[mshustov]

We currently put jest tests as siblings to the service, does it make sense to do the same for mocks rather than a separate directory

I see the benefit of it, but also I'm afraid of bloated folders. I'm fine with either option, as I don't have strong arguments here, except visual discoverability (which is quite a subjective thing).

I'd be happy to start with that if the return value thing proves to be impractical.

I believe we should provide mocks with meaningful defaults. Probably I should start with @types/jest version update. Otherwise, we cannot have type-checking for them.

[azasypkin]

Each service defines its set of mocks and allocates them inside appropriate __mocks__ folder.

Does it play well with the Jest manual mocks feature? If I'm not mistaken, if someone just calls jest.mock(...path...to...service) our mocks will be automatically loaded by the Jest, but signature of that mock module will be different from what real module has (e.g. we expose createHttpServiceMock instead of HttpService) that may potentially lead to some cryptic errors...

I do think we should provide sensible return values out of the box though.

Agree, having mocks for services is good, but having mocks for the start contracts is really critical since this is what is being passed around to other core services, legacy and plugins.

[mshustov]

Does it play well with the Jest manual mocks feature

You answered this :)

start contracts are really critical since this is what is being passed around to other core services, legacy, and plugins.

Agree. There are not so many examples in the repo, so I'd suggest to implement it as a next step when we merge #28344

updated description.
next steps:

• define mock factories for http, config and logging services. mocks should define service interface (implementing start contracts is the next step)
• update tests accordingly
• update jest and @type/jest versions. I played around with updating and realized that all tests pass, but some additional work is required to satisfy new typings for old tests. My question - should I finish dependency update as a part of the issue? Is there any policy regarding this process? @epixa @azasypkin

[epixa]

My question - should I finish dependency update as a part of the issue? Is there any policy regarding this process?

I think it's fine to include the upgrade under the umbrella of this issue, but I do recommend doing it as a standalone pull request. It's totally fine (and encouraged) to submit multiple smaller/focused pull requests for a single issue rather than trying to bundle everything together.