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.

Sign up for GitHub

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

Jump to bottom

Allow mocks to be set in config object #531

Merged
merged 9 commits into from Apr 18, 2018
Merged

Allow mocks to be set in config object #531

merged 9 commits into from Apr 18, 2018

Conversation

lmiller1990
Copy link
Member

@lmiller1990 lmiller1990 commented Apr 15, 2018 •
edited

First try at #325 .

I am sure this is not the best way to write the test, though.

Initially I was using $store instead of $t, which caused a test related to installing vuex on createLocalVue to fail. I wonder why?

eddyerburgh
eddyerburgh reviewed Apr 15, 2018
View reviewed changes
packages/shared/merge-options.js Outdated
@@ -16,12 +16,29 @@ function getStubs (optionStubs, config) {
}
}

function getMocks (optionMocks, config) {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok! done.

  • methods also works in the same way, now that we have a generic getOptions. Just need to add it to mergeOptions. Should we add that in this commit? It's only a few lines.
  • computed might need a bit more work, since it needs a getter and setter to be set up. It can't just be added in the same way as methods and mocks.
  • I did not try props and data yet.

Copy link
Member

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 :)

38elements
38elements reviewed Apr 16, 2018
View reviewed changes
docs/en/api/config.md
@@ -24,3 +24,22 @@ import VueTestUtils from '@vue/test-utils'

VueTestUtils.config.stubs['my-component'] = '<div />'
```

### `mocks`

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member Author

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.

@lmiller1990
Copy link
Member Author

lmiller1990 commented Apr 16, 2018 •
edited

I added methods as well. I initially thought we should allow for all options, but some don't really make that much sense (maybe)?

The ones I can think of use cases for:

  • methods
  • mocks
  • stubs

May or may not make sense:

  • data (can accomplish the same thing with config.mocks, even if it's not exactly the same)
  • propsData (would you want a default prop..?)

Can't think of a use case for:

  • computed
  • slots
  • localVue

What does everyone think? I'm pretty happy wit the current state, it seems there was a shared interest for mocks mainly, but I'm sure other people have different use cases that might call for the others (not sure what, though).

@eddyerburgh
Copy link
Member

I think methods, mocks, and stubs is good for now 👍

eddyerburgh
eddyerburgh requested changes Apr 16, 2018
View reviewed changes
Copy link
Member

@eddyerburgh eddyerburgh left a 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

@lmiller1990
Copy link
Member Author

lmiller1990 commented Apr 18, 2018 •
edited

Sure.

Done!

eddyerburgh
eddyerburgh approved these changes Apr 18, 2018
View reviewed changes
Copy link
Member

@eddyerburgh eddyerburgh left a 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

@eddyerburgh eddyerburgh merged commit 9960f7c into vuejs:dev Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@38elements 38elements 38elements left review comments

@eddyerburgh eddyerburgh eddyerburgh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lmiller1990 @eddyerburgh @38elements