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

Add client side service mocks #32999

Merged
merged 9 commits into from
Mar 18, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Mar 12, 2019

Summary

Issue #31486

Checklist

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

For maintainers

return startContract;
};

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.

as this type definition is duplicated across all *.mock.ts files I'd suggest to finally decide where to can place such helpers. We can do that in following PRs tho

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm definitely pro adding a file of "meta" types that are types used to create other types. This shouldn't have any types that are Kibana or even Node/Browser specific, but just generic types like this.

Copy link
Contributor Author

@mshustov mshustov Mar 14, 2019

Choose a reason for hiding this comment

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

I don't want to introduce new entities in the codebase. how about to just place such helpers in typings/index.d.ts ? @joshdover @epixa

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like an appropriate place to me.

return this;
});
const MockBasePathService = basePathServiceMock.create();
const BasePathServiceConstructor = jest.fn().mockImplementation(() => MockBasePathService);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine not to have *Constructor as a part of *.mock.ts. It's not widely used and it's easy to declare right at the place of use.

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.2.0 non-issue Indicates to automation that a pull request should not appear in the release notes review and removed Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc chore v7.2.0 non-issue Indicates to automation that a pull request should not appear in the release notes labels Mar 12, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

1 similar comment
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov marked this pull request as ready for review March 12, 2019 14:57
@mshustov mshustov requested a review from a team as a code owner March 12, 2019 14:57
@mshustov mshustov marked this pull request as ready for review March 12, 2019 14:57
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Mocks themselves look good to me and I like the clarity of the pattern you've introduced.

The enforcement of accurate mocks via the type system is great. Thanks for spending the time updating Jest to make that possible.

@@ -17,6 +17,7 @@
* under the License.
*/

import { basePathServiceMock } from '../../../../../core/public/base_path/base_path.mock';
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about exporting mocks at the root level of core/public? I imagine many plugins are going to be using these mocks and it'd best if they don't have to dig into the filesystem of core. I think adding a core/public/mocks.ts file that exports only the mocks that are needed (core start contracts) would be ideal.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov requested a review from joshdover March 18, 2019 09:46

const createStartContractMock = () => {
const startContract: jest.Mocked<NotificationsStart> = {
// we have to suppress type errors until decide how to mock es6 class
Copy link
Contributor Author

Choose a reason for hiding this comment

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

created an issue #33396

@mshustov mshustov merged commit 03afe53 into elastic:master Mar 18, 2019
@mshustov mshustov deleted the issue-31486-client-mocks branch March 18, 2019 15:11
mshustov added a commit to mshustov/kibana that referenced this pull request Mar 18, 2019
* Add client side service mocks

* remove ab obsolete test snapshot

* put back accidentally removed type definition

* define MethodKeysOf, PublicMethodsOf on project level

* export src/core/public service mocks

* export src/core/server service mocks
@mshustov mshustov added v7.2.0 and removed review labels Mar 18, 2019
mshustov added a commit that referenced this pull request Mar 18, 2019
* Add client side service mocks

* remove ab obsolete test snapshot

* put back accidentally removed type definition

* define MethodKeysOf, PublicMethodsOf on project level

* export src/core/public service mocks

* export src/core/server service mocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported chore Feature:New Platform release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants