-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Mocks for CoreStart, CoreSetup and PluginInitializerContext #39351
Conversation
Pinging @elastic/kibana-platform |
Closes #38679 |
💔 Build Failed |
e970b27
to
a9e37e6
Compare
💔 Build Failed |
💔 Build Failed |
@stacey-gammon I tested the mocks by using them in the embeddable tests, please review. |
498ca3d
to
0d5fadf
Compare
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions but generally, LGTM. did not pull down and test locally, code review only.
src/core/server/mocks.ts
Outdated
prod: false, | ||
}, | ||
}, | ||
config: pluginInitializerContextConfigMock(config)() as jest.Mocked< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, since the only place this is called is right here and the return value is used right away, why not just make it
export function createPluginInitializerContextConfigMock<T>(config: T) {
const mock: jest.Mocked<PluginInitializerContext<T>['config']> = {
create: jest.fn().mockReturnValue(of(config)),
createIfExists: jest.fn().mockReturnValue(of(config)),
};
return mock;
}
But I notice you export that file so maybe you assume it will likely be used elsewhere, and in those cases, passed in a value
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I based this on the tests in https://github.com/toddself/kibana/blob/proxy/x-pack/plugins/proxy/server/cluster_doc.test.ts#L23-L59 (not yet merged into master), which used default and then specific overrides for each of the tests.
Though looking at this again, it feels like this is easy enough for the specific test to implement themselves if they need it. It makes the mock a little weird to consume and developers will probably need to look at the source code to understand the signature, at which point having an additional Object.assign({}, mydefaults, value)
is probably much easier to follow for anyone reading the code.
@@ -17,33 +17,15 @@ | |||
* under the License. | |||
*/ | |||
|
|||
import { fatalErrorsServiceMock, notificationServiceMock } from '../../../../core/public/mocks'; | |||
import { coreMock } from '../../../../core/public/mocks'; |
There was a problem hiding this comment.
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 having this file also be in NP so we don't even have to duplicate this everywhere? Then in your actual test files, you'd just import something like
import { coreMock } from '../../../../core/public/np_core.test.mocks';
?
... though I suppose in this case I wouldn't want to mock the overlay functions for everyone... But it does seem like in most cases, just importing the single file is enough for folks who don't have to mock anything particular. 🤔 Just a thought!
...s/embeddable_api/public/panel/panel_header/panel_actions/add_panel/add_panel_flyout.test.tsx
Outdated
Show resolved
Hide resolved
💔 Build Failed |
💚 Build Succeeded |
FYI Just tried this out in one of my PRs, and it worked great. |
💔 Build Failed |
retest |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
retest |
💔 Build Failed |
retest |
💔 Build Failed |
💚 Build Succeeded |
…39351) * Mocks for CoreStart, CoreSetup and PluginInitializerContext * Public CoreStart, CoreSetup mocks * Update api signature/docs * Convert embaddable_api tests to new core mocks * CR Feedback * Introduce ui_new_platform.test.mocks and refactor embedabble tests * Hack to get TS warnings for Core mocks * Core mocks types cleanup & hack to get TS warnings for Server Core mocks * Use __mocks__ new_platform * Remove accidently commited auto-mock * Introduce MockedKeys type for Core mocks * Better typing/docs for UiSettings * Revert "Use __mocks__ new_platform" This reverts commit 2d666fa. * Add missing mock to test * Cleanup UiSettings types
…39894) * Mocks for CoreStart, CoreSetup and PluginInitializerContext * Public CoreStart, CoreSetup mocks * Update api signature/docs * Convert embaddable_api tests to new core mocks * CR Feedback * Introduce ui_new_platform.test.mocks and refactor embedabble tests * Hack to get TS warnings for Core mocks * Core mocks types cleanup & hack to get TS warnings for Server Core mocks * Use __mocks__ new_platform * Remove accidently commited auto-mock * Introduce MockedKeys type for Core mocks * Better typing/docs for UiSettings * Revert "Use __mocks__ new_platform" This reverts commit 2d666fa. * Add missing mock to test * Cleanup UiSettings types
Summary
Mocks for CoreStart, CoreSetup and PluginInitializerContext to make it easier to write plugin tests that depend on Core.