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.

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

Update wrapper async mode docs #1648

Merged
merged 10 commits into from Aug 19, 2020

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Aug 16, 2020

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)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe: Documentation Update

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:

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:

@AtofStryker 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 AtofStryker force-pushed the update_wrapper_async_mode_docs branch from 80a5df8 to c82d222 Compare August 16, 2020 20:40
@AtofStryker AtofStryker marked this pull request as ready for review August 16, 2020 20:42
@99linesofcode
Copy link
Contributor

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
Copy link
Contributor Author

@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.

Copy link
Contributor

@99linesofcode 99linesofcode left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😄 .


options.at(1).setSelected()
await options.at(1).setSelected()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

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
Copy link
Contributor

99linesofcode commented Aug 17, 2020

@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
Copy link
Contributor Author

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.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

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.


options.at(1).setSelected()
await options.at(1).setSelected()
Copy link
Member

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.

@lmiller1990
Copy link
Member

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

@AtofStryker
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

99linesofcode commented Aug 18, 2020

@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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

lmiller1990 commented Aug 18, 2020

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:

  1. 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.
  2. 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.
  3. 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.

const multiselect = wrapper.find('select')
await multiselect.setValue(['value1', 'value3'])

expect(wrapper.find('select').element.value).toBe(['value1', 'value3'])
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 3801438

@AtofStryker
Copy link
Contributor Author

@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
Copy link
Contributor

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

@AtofStryker AtofStryker force-pushed the update_wrapper_async_mode_docs branch from f460eb3 to 3801438 Compare August 19, 2020 06:00
@lmiller1990 lmiller1990 merged commit f94a62d into vuejs:dev Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants