Plugin mocks by streamich · Pull Request #40123 · elastic/kibana

streamich · 2019-07-02T09:16:30Z

Summary

Adds ability to mock plugins in ui/new_platform.
Adds ability to mock whole ui/new_platform by simply jest.mock('ui/new_platform')

Checklist

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

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
~~Unit or functional tests were updated or added to match the most common scenarios~~
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~This was checked for breaking API changes and was labeled appropriately~~
~~This includes a feature addition or change that requires a release note and was labeled appropriately~~

Dev Docs

`ui/new_platform`

As we move more services to the New Platform we refactor existing code to internally use

import { npSetup, npStart } from 'ui/new_platform';

If your Jest tests break because something not immediate to your plugin was refactored to use ui/new_platform you can use

jest.mock('ui/new_platform');

to stub ui/new_platform module and make your existing Jest work temporarily while you are refactoring them for the New Platform.

elasticmachine · 2019-07-02T09:17:13Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-07-02T09:17:47Z

Pinging @elastic/kibana-platform

rudolf · 2019-07-02T09:33:35Z

Adds ability to mock whole ui/new_platform by simply jest.mock('ui/new_platform')

This was also brought up in the original mocks PR
#39351 (comment)
#39351 (comment)

When your tests don't have to set custom mock values jest.mock('ui/new_platform') is a lot shorter, but you have to do some ugly casting to be able to change the mock return values or inspect if mocked functions were called. So we decided to stick with import { coreStartMock } from '../../../../../../../../core/public/ui_new_platform.test.mocks'.

rudolf · 2019-07-02T09:36:21Z

Happy to re-open the discussion here. The one thing I didn't investigate was why we can't import mocks with absolute file paths like import 'src/core/public/ui_new_platform.test.mocks' as that would make this approach a bit easier to use as you don't have to type such insane relative paths.

streamich · 2019-07-02T09:58:38Z

@skaapgif

This PR still preserves your intended behaviour but with a shorter import statement.

import { coreStartMock } from 'ui/new_platform/mocks/cores.test.mocks';

Or with plugins:

import { mock } from 'ui/new_platform/mocks/cores_and_plugins.test.mocks';

But it also ads ability for devs to mock ui/new_platform with this simple line:

jest.mock('ui/new_platform')

In general though, I'm not sure that importing something from that mock and asserting against it is a good idea. In my view, for unit tests we should not even need this mock, devs in their tests should mock whatever they need to mock for testing. I see this as us adding a "global" mock for easier migration to NP, so that plugins can simply stub (not mock) their static imports with jest.mock('ui/new_platform') temporarily.

ui/new_platform will go away in NP, any assertions implemented against ui/new_platform static import are not useful, as it will not exist in NP. Thus it is better to write tests already now the way they will be in NP. But we have this jest.mock('ui/new_platform') stub just so the existing tests still work.

streamich · 2019-07-02T10:01:18Z

@skaapgif

The one thing I didn't investigate was why we can't import mocks with absolute file paths like import 'src/core/public/ui_new_platform.test.mocks' as that would make this approach a bit easier to use as you don't have to type such insane relative paths.

This PR allows you to import using absolute path.

import 'ui/new_platform/mocks/cores.test.mocks';

or

import { coreStartMock } from 'ui/new_platform/mocks/cores.test.mocks';

I believe to import from core you need to use kibana/..., but not sure about that.

rudolf · 2019-07-02T10:40:29Z

yeah I like the absolute and shorter import from moving src/core/public/ui_new_platform.test.mocks.ts → src/legacy/ui/public/new_platform/mocks/cores.test.mocks.ts +1

Between

jest.mock('ui/new_platform') // (1)

and

import from 'ui/new_platform/mocks/cores.test.mocks'; // (2)

I don't see any advantages to using (1) above (2). So we have two ways to do the same thing without any benefits. Given that (2) is more powerful, I'd prefer if we remove (1) and only keep (2).

streamich · 2019-07-02T10:55:04Z

@skaapgif

I don't see any advantages to using (1) above (2). So we have two ways to do the same thing without any benefits. Given that (2) is more powerful, I'd prefer if we remove (1) and only keep (2).

If we had to pick one, I would actually prefer (1) for the same reason, because (2) is more "powerful". It is more "powerful" because it is easier to do assertions in (2) against ui/new_platform, right?

But I'm not sure there is any benefit of doing assertions agains ui/new_platform, because ui/new_platform will not exist in the New Platform and mocks of ui/new_platform will not exist in the New Platform. Thus all new assertions we create against ui/new_platform will need to be migrated.

That's why I don't see a reason to create any assertions agains ui/new_platform at all. It should be just a stub—only to make existing tests still function, while we are in the migration process.

The only plugin that was doing assertions against ui/new_platform was Embeddables API. I am currently moving Embeddables API to the New Platform and I had to remove those assertions in my branch, because there is no ui/new_platform in the New Platform.

elasticmachine · 2019-07-02T16:31:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rudolf · 2019-07-03T10:39:28Z

But I'm not sure there is any benefit of doing assertions agains ui/new_platform, because ui/new_platform will not exist in the New Platform and mocks of ui/new_platform will not exist in the New Platform. Thus all new assertions we create against ui/new_platform will need to be migrated.

I think this is a good point. After discussing this with @streamich we could not think of a good reason someone would have to make assertions against the ui/new_platform module from existing legacy code.

Our assumption is:

Existing code doesn't need to make assertions against the ui/new_platform module
New code should be written as a New Platform shim or at the very least receive it's dependencies as parameters. This allows passing a Core mock in your tests without requiring you to mock the ui/new_platform module.

If this assumption is true, I agree that jest.mock('ui/new_platform') is a better way to enforce good architecture for new code and the syntax is a bit more explicit that you're mocking a module.

Since it's easier to start with being too restrictive and relaxing it later than the other way around going for option (1) seems like the best choice until our assumption is proven wrong.

@elastic/kibana-platform Thoughts?

lukeelmers

Our assumption is:

Existing code doesn't need to make assertions against the ui/new_platform module

New code should be written as a New Platform shim or at the very least receive it's dependencies as parameters. This allows passing a Core mock in your tests without requiring you to mock the ui/new_platform module.

I was previously advocating for the other more "powerful" approach, but I think reading this thread has changed my mind :)

I like the idea of sticking with an approach that enforces good architectural practices for NP code.

Overall this LGTM! I added a few notes, but no major concerns on my end that should block merging.

lukeelmers · 2019-07-03T21:42:03Z

src/plugins/data/public/mocks.ts

+import { Plugin } from '.';
+
+export type Setup = jest.Mocked<ReturnType<Plugin['setup']>>;
+export type Start = jest.Mocked<ReturnType<Plugin['start']>>;


We should already be exporting these ReturnTypes as something like DataSetup from the index.ts file. Probably fine to leave as-is for now to prevent confusion with the legacy/shim data plugin, just wanted to note that here.

lukeelmers · 2019-07-03T21:48:32Z

src/plugins/__mocks__/mocks.ts

+// eslint-disable-next-line @kbn/eslint/no-restricted-paths
+import { dataPluginMock } from '../data/public/mocks';
+
+export const pluginsMock = {


Do we want/need this global mock for plugins in the new platform? vs importing into helpers.ts whatever plugin setup contracts we need directly from the plugins themselves?

I'm trying to think of what the use case would be here in the NP. Based on the discussion on this PR, I was under the impression we wouldn't do a "global" solution like this in NP (just for ui/new_platform), and instead people would import mocks for core and whichever services their plugin depends on.

So if that assumption is correct, it implies this file would be going away eventually, in which case I'm sure it's needed in the first place.

I was thinking it could simplify testing if there was one object with all plugin mocks, similar to how there is coreMock with mocks of all Core services.

...instead people would import mocks for core and whichever services their plugin depends on.

Yes, you still need to explicitly import it in the New Platform. The idea is—if your plugin depends on 5 other plugins it is more convenient to import an object of all plugin mocks from one place then have 5 different import statements and construct that object yourself. If you want to be really precise in your tests, you can destructure that object and pick only the needed plugin mocks.

I think what @lukeelmers is suggesting is not to change any functionality but just to move the contents of this file into src/legacy/ui/public/new_platform/__mocks__/helpers.ts. I think that's a good idea 👍 Like you said, it makes sense to get all the plugin mocks when you're consuming NP from legacy ui/new_platform but if all the mocking logic lives in the same file, then we can just delete that file once the migration is complete.

elasticmachine · 2019-07-03T23:12:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rudolf · 2019-07-04T08:08:43Z

src/plugins/__mocks__/mocks.ts

+// eslint-disable-next-line @kbn/eslint/no-restricted-paths
+import { dataPluginMock } from '../data/public/mocks';
+
+export const pluginsMock = {


I think what @lukeelmers is suggesting is not to change any functionality but just to move the contents of this file into src/legacy/ui/public/new_platform/__mocks__/helpers.ts. I think that's a good idea 👍 Like you said, it makes sense to get all the plugin mocks when you're consuming NP from legacy ui/new_platform but if all the mocking logic lives in the same file, then we can just delete that file once the migration is complete.

rudolf

I'd lean towards removing the src/plugins/__mocks__/mocks.ts file but happy either way.

elasticmachine · 2019-07-04T11:34:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-04T13:02:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Plugin mocks (#40123) * test: 💍 add ability to mock plugins in ui/new_platform * chore: 🤖 delete olds NP backdoor mock * test: 💍 normalize plugin contract naming in line with Core * test: 💍 use mock from ui/new_platform instead of core * test: 💍 remove ui/new_platform mocking from Core * chore: 🤖 leave only one mocking strategy for ui/new_platform * style: 💄 format according to Prettier * chore: 🤖 remove /src/plugins/__mocks__ folder * fix: 🐛 move mock to correct folder * test: 💍 fix test mock

test: 💍 add ability to mock plugins in ui/new_platform

827a5bb

streamich requested a review from rudolf July 2, 2019 09:16

streamich requested a review from a team as a code owner July 2, 2019 09:16

streamich added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes review Team:AppArch labels Jul 2, 2019

streamich added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// and removed Feature:New Platform labels Jul 2, 2019

streamich changed the title ~~test: 💍 add ability to mock plugins in ui/new_platform~~ Plugin mocks Jul 2, 2019

streamich mentioned this pull request Jul 2, 2019

「AppArch」Interpreter 👉 New Platform #39329

Merged

chore: 🤖 delete olds NP backdoor mock

ebfeea4

test: 💍 normalize plugin contract naming in line with Core

7e1fde1

streamich added 2 commits July 2, 2019 11:44

test: 💍 use mock from ui/new_platform instead of core

535d47f

test: 💍 remove ui/new_platform mocking from Core

517ecf8

streamich requested a review from a team as a code owner July 2, 2019 09:48

streamich requested review from lukeelmers and ppisljar July 2, 2019 10:11

Merge branch 'master' of github.com:elastic/kibana into plugin-mocks

4e4c7a1

elastic deleted a comment from elasticmachine Jul 2, 2019

streamich added 4 commits July 3, 2019 21:19

chore: 🤖 catch up with master

d34ff89

chore: 🤖 leave only one mocking strategy for ui/new_platform

fcf5f0b

style: 💄 format according to Prettier

70f525a

Merge branch 'master' of github.com:elastic/kibana into plugin-mocks

774470d

elastic deleted a comment from elasticmachine Jul 3, 2019

Merge branch 'master' of github.com:elastic/kibana into plugin-mocks

561cadf

lukeelmers approved these changes Jul 3, 2019

View reviewed changes

rudolf requested changes Jul 4, 2019

View reviewed changes

rudolf approved these changes Jul 4, 2019

View reviewed changes

streamich added 2 commits July 4, 2019 12:23

Merge remote-tracking branch 'upstream/master' into plugin-mocks

99341e8

chore: 🤖 remove /src/plugins/__mocks__ folder

370271f

streamich merged commit 7ad2c94 into elastic:master Jul 4, 2019

streamich added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 4, 2019

streamich mentioned this pull request Jul 4, 2019

[7.x] Plugin mocks (#40123) #40362

Merged

Conversation

streamich commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev Docs

ui/new_platform

Uh oh!

elasticmachine commented Jul 2, 2019

Uh oh!

elasticmachine commented Jul 2, 2019

Uh oh!

rudolf commented Jul 2, 2019

Uh oh!

rudolf commented Jul 2, 2019

Uh oh!

streamich commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

streamich commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rudolf commented Jul 2, 2019

Uh oh!

streamich commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 2, 2019

💚 Build Succeeded

Uh oh!

rudolf commented Jul 3, 2019

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

lukeelmers Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeelmers Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

streamich Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

rudolf Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 3, 2019

💔 Build Failed

Uh oh!

rudolf Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 4, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 4, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

streamich commented Jul 2, 2019 •

edited

Loading

`ui/new_platform`

streamich commented Jul 2, 2019 •

edited

Loading

streamich commented Jul 2, 2019 •

edited

Loading

streamich commented Jul 2, 2019 •

edited

Loading

lukeelmers Jul 3, 2019 •

edited

Loading