-
Notifications
You must be signed in to change notification settings - Fork 663
Allow mocks to be set in config object #531
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
Conversation
packages/shared/merge-options.js
Outdated
| } | ||
| } | ||
|
|
||
| function getMocks (optionMocks, config) { |
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.
This is nearly the same as the getStubs function, can you split it into a generic getOptions function?
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.
Sure. Maybe we can use it to provide an interface to all options.
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.
Ok! done.
methodsalso works in the same way, now that we have a genericgetOptions. Just need to add it tomergeOptions. Should we add that in this commit? It's only a few lines.computedmight need a bit more work, since it needs agetterandsetterto be set up. It can't just be added in the same way asmethodsandmocks.- I did not try
propsanddatayet.
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.
Great work! Yes, please add methods :)
| ``` | ||
|
|
||
| ### `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.
I think that it is necessary to add links for this section to following pages.
https://github.com/vuejs/vue-test-utils/blob/dev/docs/en/api/README.md
https://github.com/vuejs/vue-test-utils/blob/dev/docs/en/README.md
https://github.com/vuejs/vue-test-utils/blob/dev/docs/en/SUMMARY.md
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.
Yep, for sure. Will do. Thanks.
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.
Edit: actually, I don't think we need to. This doesn't add a new API, just build on the existing config page.
|
I added The ones I can think of use cases for:
May or may not make sense:
Can't think of a use case for:
What does everyone think? I'm pretty happy wit the current state, it seems there was a shared interest for |
|
I think methods, mocks, and stubs is good for now 👍 |
eddyerburgh
left a comment
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.
Can you add some tests in tests/specs/mounting-options/methods.spec.js and mocks.spec.js to make sure the options take priority over config version, similar to this test: https://github.com/vuejs/vue-test-utils/blob/dev/test/specs/mounting-options/stubs.spec.js#L226
eddyerburgh
left a comment
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.
Great work. I'm going to open a PR to fix config for server-test-utils
First try at #325 .
I am sure this is not the best way to write the test, though.
Initially I was using
$storeinstead of$t, which caused a test related to installing vuex oncreateLocalVueto fail. I wonder why?