Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Mocks for CoreStart, CoreSetup and PluginInitializerContext #39351
Changes from 4 commits
968fb71
f189fdc
e9db29f
0d5fadf
b038601
358d928
bcf7630
2be9ca7
4f5331c
2d666fa
a0db51f
45d0887
c52707e
878c46a
f6fd9d1
f4082d8
49aa1b5
96934a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.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!