Update wrapper async mode docs #1648

AtofStryker · 2020-08-16T19:36:55Z

Since sync mode has been removed, many of the wrapper methods now return nextTick, which is async and is awaitable. These methods include setValue, setSelected, setChecked, setData, and setProps. Though these methods are async, the documentation has them referenced/used in a sync fashion. I am not sure if this is intentional or just missed with the sync removal, but the use of these methods seems a bit unclear. Hopefully updating the documentation will allow for clearer use. If there are issues with the examples I have updated (esp. likely on the offshoot examples), please let me know!

What kind of change does this PR introduce? (check at least one)

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

It's submitted to the dev branch.
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
All tests are passing: https://github.com/vuejs/vue-test-utils/blob/dev/.github/CONTRIBUTING.md#development-setup
New/updated tests are included

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

99linesofcode · 2020-08-17T21:00:32Z

Hey @AtofStryker,

This indeed seemed like an oversight as I learned when writing up issue #1626. I had intended to make these changes myself tomorrow so I will instead do a quick review to see whether you missed anything. Thanks 🎩

AtofStryker · 2020-08-17T21:10:32Z

@99linesofcode Sorry for the oversight on my end! It was something that has been bothering me for a while and didn't realize we had an open issue for it. Definitely would appreciate a quick review and would love to collaborate on this if there is anything you would like to change or add.

99linesofcode

What are you thoughts on some of the test cases not containing an assertion? I noticed they are left out sometimes and I personally think that they could be made more complete. Especially now that you have wrapped them in test cases.

See my initial comment.

99linesofcode · 2020-08-17T21:50:25Z

docs/api/wrapper/setChecked.md

+test('setChecked demo', async () => {
+  const wrapper = mount(Foo)
+  const radioInput = wrapper.find('input[type="radio"]')
+  await radioInput.setChecked()


expect(wrapper.find('input[type="radio"]').element.checked).toBeTruthy()

On that note, is there something wrong with using a let here, allowing Jest / VueTestUtils to update the radio button and allowing us to replace the duplicate wrapper.find('input[type="radio"]') call with radioInput instead?

expect(wrapper.find('input[type="radio"]').element.checked).toBeTruthy()

On that note, is there something wrong with using a let here, allowing Jest / VueTestUtils to update the radio button and allowing us to replace the duplicate wrapper.find('input[type="radio"]') call with radioInput instead?

I don't think there is an issue with using let over const here. I think it depends on the strategy we want to go about from a documentation standpoint, which I'll describe a bit more here. If we ultimately wind up adding the assertions, we can use let, but for example use, duplication isn't necessarily a bad thing 😄 .

99linesofcode · 2020-08-17T21:52:52Z

docs/api/wrapper/setSelected.md


-options.at(1).setSelected()
+  await options.at(1).setSelected()


expect(wrapper.find('option:checked').element.value).toBe('bar')

I like this one better, too.

99linesofcode · 2020-08-17T22:08:52Z

@AtofStryker not at all. I had intended to start making small contributions here and there as I'm fairly new to the Vue ecosystem. Glad to see you beat me to the punch 😉

I'll double check everything tomorrow morning and will make any changes necessary (if any). Good night 👋

AtofStryker · 2020-08-17T23:49:07Z

What are you thoughts on some of the test cases not containing an assertion? I noticed they are left out sometimes and I personally think that they could be made more complete. Especially now that you have wrapped them in test cases.

@99linesofcode It definitely does appear a bit inconsistent at times, but we might want to be careful about adding them. I think the general approach to documentation is to be as descriptive as possible on how to use the API over potentially being prescriptive. We definitely could add some assertions to the testing documentation, but we also don't want to be misleading as to suggesting that these assertions are they way individuals should test their vue components. In other words, it might be best to not include the assertions where they are not needed. But I will defer to @lmiller1990 on this one.

Also, welcome to the community and the Vue ecosystem! We are glad to have you.

lmiller1990

One small comment!

@afontcu may have some input. I would REALLY like to thin the guides down to only include actual info on how to use VTU, much less guides on things like tooling.

Ideally, it'd be great to reorganize things more like the VTU next docs: https://vuejs.github.io/vue-test-utils-next-docs/guide/introduction.html if anyone is motivated. We may be able to share some of the docs from there in here, too, since the ideas and API are very similar.

lmiller1990 · 2020-08-18T01:39:49Z

docs/api/wrapper/setSelected.md


-options.at(1).setSelected()
+  await options.at(1).setSelected()


I like this one better, too.

lmiller1990 · 2020-08-18T01:42:50Z

@AtofStryker I personally like "complete" tests too, include the setup, test and assertion.

AtofStryker · 2020-08-18T01:53:09Z

@lmiller1990 Sounds good! It might be a good idea, at a minimum, to include the assertions that @99linesofcode is recommending in this PR. Then in a separate effort, attempt to reorganize the docs? Likely an audit is in place to keep things consistent.

lmiller1990 · 2020-08-18T04:11:53Z

Sounds good... I would not recommend any large reorganizing until we have a good outline. @afontcu should be involved, but I would say this is lower priority - I think we should spend time on the V2 docs first, then re-use the structure and content where possible in these docs, too.

The docs for this lib were written a long time ago by many different people without really any review process, so there is not very much consistency or direction. One thing I like with the V2 docs is there is a clear structure: starting with the topic, simple example, complex example, then a conclusion. There is more focus on how to use the lib, and less on the intricacies of JS, test runners and tooling.

AtofStryker · 2020-08-18T04:49:57Z

@lmiller1990 most definitely. In the meantime, we can find some low hanging fruit to iron out some inconsistencies that could be problematic to those using the 1.x API. While this was on my mind, I figured it was a good idea to make the wrapper-array consistent with the wrapper API as well as the doc changes proposed here. So I opened #1650 to give the wrapper-array API some love (Not sure how often the wrapper-array API is used). We probably want to do the same there as we do here with the documentation (from the assertions convo, not the restructure 😃 ). @99linesofcode did you want to take the reins on adding the assertions to the docs?

99linesofcode · 2020-08-18T09:22:35Z

@lmiller1990, while finding my way around testing vue components, I also stumbled upon your vue testing handbook. That resource proved very viable but I noticed some of the examples are also starting to deviate from the 1.x API.

Do you intend to keep that separate as it is a bit opinionated as to how to handle testing or would it perhaps be an idea to combine it with the v2 documentation?

I think we should spend time on the V2 docs first, then re-use the structure and content where possible in these docs, too.

Were there any discussions on this that you could point me to so I can get a better sense of the direction you want this to head in?

99linesofcode · 2020-08-18T09:32:38Z

@AtofStryker definitely. I will focus on reviewing your changes and adding the missing assertions first. If there is anything I could do to help align the examples with v2 of the docs, I will gladly do so. If there is already a PR to track those changes, I will see if I can find it.

lmiller1990 · 2020-08-18T11:34:11Z

@99linesofcode I will rewrite that book and have a v2. I will leave the current edition as v1. I should update that for the current v1 API, actually. I will do this right now.

lmiller1990 · 2020-08-18T11:35:15Z

Should we just merge this as-is then? any other changes needed? Clearly this PR is already making improvements, or do you plan to do more?

Up to you!

99linesofcode · 2020-08-18T12:07:00Z

@lmiller1990

I've added the requested assertions by opening a PR to @AtofStryker's fork. Not sure if thats the way to go?

That said, adding those extra assertions feels a bit redundant considering we are going to backport the v2 docs. Is there a PR that contains an outline for the preferred structure for the new documentation? Can I just look at the docs that are already there and start a new PR to discuss the overhaul of v1?

lmiller1990 · 2020-08-18T12:23:51Z

I agree it might be a bit redundant if we want to overhaul everything.

Maybe we should try and finish writing the docs for V2 and just backport all of that! I have been doing most of it, any submissions of unwritten content would be great - writing good docs in incredibly tiring.

This is where the new docs were planned: vuejs/test-utils-docs#19 and the live version is here: https://vuejs.github.io/vue-test-utils-next-docs/guide/installation.html#installation

I would very much like to have as much consistency between V1 and V2 as possible. Ideally we use almost the same docs and API for both.

If you guys want to help out with docs and want direction, let's plan this a bit. What do you think of this:

finish writing the current V2 docs. There is issues for each unwritten article here https://github.com/vuejs/vue-test-utils-next-docs/issues and some others. I think there is more value in preparing for V2 than improving these docs at this point. For this, I'd recommend posting in the relevant issue in the repo, pinging someone if you want guidance, and writing. It takes around 3-4 hours to write an article for docs in my experience.
when we get V2 docs done, we go through each and write the examples with V1 (they should all be guaranteed to work with V2 already). See what does and doesn't port backwards, and modify those articles to work with V1.
Replacing existing docs with new ones

What do you think? @AtofStryker @99linesofcode ? Having some more people on board to help would be great, I'm doing vue-jest + v1 + v2 + docs and it's a lot to handle! Lucky it's OSS so anyone can help with anything.

99linesofcode · 2020-08-18T13:53:25Z

docs/api/wrapper/setValue.md

+  const multiselect = wrapper.find('select')
+  await multiselect.setValue(['value1', 'value3'])
+
+  expect(wrapper.find('select').element.value).toBe(['value1', 'value3'])


Actually, is this the way the value is returned in case of a multiselect?

Good question! I'll take a look and see what that line looks like.

expect(wrapper.find('select').element.value).toBe('value1')? Looks like only the first value is returned? That's kind of lame 😆

Well, that goes to show how often I use multiple selects 😆

Haha, I've been trying to get this to work in v1.0.3 but only just realized that change was only just released as v1.0.4 😴

Looking at the PR, the test performs the assertions as can be seen here: https://github.com/vuejs/vue-test-utils/pull/1554/files#diff-21606123e32f708f8a6962091664fb13R76 but that seems off to me as well. What is select.element.selectedOptions? I don't see that being used anywhere else?

Alternatively, you can also write assertions against the prop bound to the v-model directive.

Good find! selectedOptions should work really well for an example. I can add that example to the commit you added to the fork to prevent a rebase nightmare 😅

added in 3801438

AtofStryker · 2020-08-18T13:58:01Z

@lmiller1990 I would definitely love to help! I think it sounds like a solid plan to replace the existing docs. We just need to be aware of the caveats with 1.x when backporting, but to your point, the examples should be guaranteed to work.

99linesofcode · 2020-08-18T18:23:50Z

@lmiller1990 Sounds like a plan. Lets continue this conversation in vuejs/test-utils-docs#19

AtofStryker force-pushed the update_wrapper_async_mode_docs branch 2 times, most recently from 49b4cb9 to 80a5df8 Compare August 16, 2020 20:36

AtofStryker added 5 commits August 16, 2020 16:40

docs(setvalue): update setValue documentation to reflect async mode

fcd6f6d

docs(setselected): update setSelected docs to reflect async mode

e7e410d

docs(setprops): update setProps docs to reflect async mode

159971d

docs: update setData docs to reflect async mode

69a8cd8

docs(setchecked): update setChecked docs to reflect async mode

c82d222

AtofStryker force-pushed the update_wrapper_async_mode_docs branch from 80a5df8 to c82d222 Compare August 16, 2020 20:40

AtofStryker marked this pull request as ready for review August 16, 2020 20:42

99linesofcode reviewed Aug 17, 2020

View reviewed changes

lmiller1990 approved these changes Aug 18, 2020

View reviewed changes

docs/api/wrapper/setSelected.md

options.at(1).setSelected()

await options.at(1).setSelected()

Copy link

Member

lmiller1990 Aug 18, 2020

Choose a reason for hiding this comment

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

I like this one better, too.

99linesofcode added 4 commits August 18, 2020 09:40

docs(setchecked): add assertions

65f813e

docs(setdata): add whitespace to AAA

d3b2eed

docs(setprops): add whitespace to AAA

c90aed5

docs(setselected): add assertion

702851e

99linesofcode reviewed Aug 18, 2020

View reviewed changes

99linesofcode approved these changes Aug 18, 2020

View reviewed changes

docs(setvalue): add assertions

3801438

AtofStryker force-pushed the update_wrapper_async_mode_docs branch from f460eb3 to 3801438 Compare August 19, 2020 06:00

lmiller1990 merged commit f94a62d into vuejs:dev Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wrapper async mode docs #1648

Update wrapper async mode docs #1648

AtofStryker commented Aug 16, 2020 •

edited

Loading

99linesofcode commented Aug 17, 2020

AtofStryker commented Aug 17, 2020

99linesofcode left a comment

99linesofcode Aug 17, 2020

AtofStryker Aug 17, 2020

99linesofcode Aug 17, 2020

lmiller1990 Aug 18, 2020

99linesofcode commented Aug 17, 2020 •

edited

Loading

AtofStryker commented Aug 17, 2020

lmiller1990 left a comment

lmiller1990 Aug 18, 2020

lmiller1990 commented Aug 18, 2020

AtofStryker commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020

AtofStryker commented Aug 18, 2020

99linesofcode commented Aug 18, 2020 •

edited

Loading

99linesofcode commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020

99linesofcode commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020 •

edited

Loading

99linesofcode Aug 18, 2020

AtofStryker Aug 18, 2020

AtofStryker Aug 18, 2020

99linesofcode Aug 19, 2020

AtofStryker Aug 19, 2020

AtofStryker Aug 19, 2020

AtofStryker commented Aug 18, 2020

99linesofcode commented Aug 18, 2020


		options.at(1).setSelected()
		await options.at(1).setSelected()

Update wrapper async mode docs #1648

Update wrapper async mode docs #1648

Conversation

AtofStryker commented Aug 16, 2020 • edited Loading

99linesofcode commented Aug 17, 2020

AtofStryker commented Aug 17, 2020

99linesofcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

99linesofcode commented Aug 17, 2020 • edited Loading

AtofStryker commented Aug 17, 2020

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 commented Aug 18, 2020

AtofStryker commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020

AtofStryker commented Aug 18, 2020

99linesofcode commented Aug 18, 2020 • edited Loading

99linesofcode commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020

99linesofcode commented Aug 18, 2020

lmiller1990 commented Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Aug 18, 2020

99linesofcode commented Aug 18, 2020

AtofStryker commented Aug 16, 2020 •

edited

Loading

99linesofcode commented Aug 17, 2020 •

edited

Loading

99linesofcode commented Aug 18, 2020 •

edited

Loading

lmiller1990 commented Aug 18, 2020 •

edited

Loading