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.

Sign up for GitHub

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

Jump to bottom

[New platform] Add the service level mocks for server side #32300

Merged
merged 6 commits into from
Mar 7, 2019
Merged

[New platform] Add the service level mocks for server side #32300

merged 6 commits into from
Mar 7, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Mar 1, 2019 •
edited
Loading

Summary

Issue #31486

  • Implement mechanism that allows us to re-use mocks of core services between tests.
  • Default mocks must satisfy a contract of a service.
  • Default mocks should allow developers to re-define mock values according to contract.
  • Implementation for http, config, logging, elasticsearch services

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Screenshots

when a mock implementation isn't full
2019-03-04_08-40-07

when a mock implementation doesn't satisfy a contract
2019-02-21_08-46-14

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Mar 1, 2019
@mshustov mshustov requested review from epixa and azasypkin March 1, 2019 13:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

mshustov
mshustov commented Mar 1, 2019
View reviewed changes
src/core/server/config/config_service.mock.ts

import { ConfigService } from './config_service';

type MethodKeysOf<T> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a separate place for declaring our custom TS helpers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet. There are notions like "test utilities" elsewhere in the project that we could adopt in some form.

Do you anticipate using this helper in a lot of places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now I inline it in every *.mock.ts file

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's not great. We should come up with something, but it doesn't need to be in this PR.

@elasticmachine

This comment has been minimized.

Sign in to view
@mshustov mshustov added review non-issue Indicates to automation that a pull request should not appear in the release notes labels Mar 1, 2019
@mshustov mshustov requested a review from a team March 4, 2019 07:37
@mshustov mshustov requested a review from epixa March 6, 2019 12:28
@elasticmachine

This comment has been minimized.

Sign in to view
@mshustov

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

epixa
epixa approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

This is only for the core/server though. We should have the same conventions established in core/public as well. Can you update the title of this PR before merging to reflect that this is server-only?

@mshustov mshustov changed the title Add the service level mocks [New platform] Add the service level mocks for server side Mar 7, 2019
@mshustov mshustov merged commit a188f53 into elastic:master Mar 7, 2019
@mshustov mshustov deleted the issue-31486-typed-mocks branch March 7, 2019 14:18
@mshustov mshustov added v7.2.0 and removed review labels Mar 7, 2019
@mshustov
Copy link
Contributor Author

mshustov commented Mar 7, 2019 •
edited
Loading

@epixa should we backport new platform features?

I will send another PR for client side

@mshustov mshustov mentioned this pull request Mar 7, 2019
Merged
mshustov added a commit to mshustov/kibana that referenced this pull request Mar 7, 2019
@mshustov
[New platform] Add the service level mocks for server side (elastic#3…
160408e 
…2300)

* add LoggingServiceMock

* add configServiceMock

* add httpServiceMock and elasticsearchServiceMock

* use elasticsearch.startContract instead of inlined declarations

* add createStartContract factory
mshustov added a commit that referenced this pull request Mar 7, 2019
@mshustov
[New platform] Add the service level mocks for server side (#32300) (#…
24db79f 
…32655)

* add LoggingServiceMock

* add configServiceMock

* add httpServiceMock and elasticsearchServiceMock

* use elasticsearch.startContract instead of inlined declarations

* add createStartContract factory
@epixa
Copy link
Contributor

epixa commented Mar 7, 2019

@restrry Yes, to 7.x at least. Though feel free to backport test changes further to make other backports easier to do with less risk of conflicts.

This was referenced Mar 18, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epixa epixa epixa approved these changes

@azasypkin azasypkin Awaiting requested review from azasypkin

Assignees
No one assigned
Labels
backported chore Feature:New Platform non-issue Indicates to automation that a pull request should not appear in the release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mshustov @elasticmachine @epixa