feat: expose everything on wrapper.vm to help testing `script setup` #931

cexbrayat · 2021-09-09T16:17:28Z

This commit changes wrapper.vm to actually point to vm.$.proxy.
This shouldn't change the behaviour of existing tests, but allows components written with script setup to be tested as well,
without the need to expose everything just for testing purposes.

For example a component like:

<script setup lang="ts">
import { ref } from 'vue'

const count = ref(0)
</script>

can now be tested with expect(wrapper.vm.count).toBe(0), whereas you previously had to add defineExpose({ count }) for this to work.

The downside is that you now can't test that something is not exposed by a component, but I don't think this is a problem.

This also removes the previous hacks for script setup, as it looks like they are no longer necessary.

cexbrayat · 2021-09-09T16:22:04Z

Note that I tested this patch on a project that I fully migrated to script setup, and this made the tests go from half failing to 100% success. I'm praying that there are no side effect to exposing vm.$.proxy but that may be worth a try.

This commit changes `wrapper.vm` to actually point to `vm.$.proxy`. This shouldn't change the behaviour of existing tests, but allows components written with `script setup` to be tested as well, without the need to expose everything just for testing purposes. For example a component like: ```vue <script setup lang="ts"> import { ref } from 'vue' const count = ref(0) </script> ``` can now be tested like `expect(wrapper.vm.count).toBe(0)`, whereas you previously had to add `defineExpose({ count })` for this to work. The downside is that you now can't test that something is _not_ exposed by a component, but I don't think this is a problem. This also removes the previous hacks for script setup, as it looks like they are no longer necessary.

cexbrayat · 2021-09-09T16:56:57Z

tsconfig.volar.json

@@ -3,5 +3,6 @@
  "compilerOptions": {
    "lib": ["DOM", "ES2020"],
    "skipLibCheck": true
-  }
+  },
+  "exclude": ["tests/expose.spec.ts"]


volar can't figure out that we magically expose more things on wrapper.vm. That might be an issue long term as users may run into it as well.

Seems like this isn't really something than can be fixed - it makes little sense for Volar to have something special for test utils.

Yeah. I think there was an issue even with regular defineExpose (which should work), but we'll see if that's really an issue and what we can do about it.

cexbrayat · 2021-09-10T08:47:58Z

src/vueWrapper.ts

+    // a test will still be able to do something like
+    // `expect(wrapper.vm.count).toBe(1)`
+    // (note that vm can be null for functional components, hence the condition)
+    this.componentVM = vm ? (vm.$.proxy as T) : (vm as T)


we could also consider alternatives like:

exposing the proxy on a different field, like wrapper.proxy, allowing to do expect(wrapper.proxy.count).toBe(2), without changing wrapper.vm

add a mounting option to expose everything:

const wrapper = mount(Hello, { expose: true }); expect(wrapper.vm.count).toBe(2); // vm would be proxy here

only do this for script setup components, or for components that do not expose anything

expect(wrapper.proxy.count).toBe(2) doesn't seem ideal - unless you know Vue and script setup very well, it'd be hard to understand (and even document). I think tests should be written the same way, regardless of whether you are using script setup, etc.

Yeah I feel the same. I think testing different types of components should not change the way tests are written.

I totally support @cexbrayat point - but being a devil's advocate - component implementation specific already leaks into VTU: for example you can't use data() option for class components, you won't be able to use components param with functional components, etc. I'm a bit concerned that we are doing exceptions here, although script setup is designed to be "blackboxed"

lmiller1990 · 2021-09-20T00:48:06Z

Removing the hack is a great idea. I am not convinced exposing things that are intentionally unexposed is a good idea - it's like when you make a private field public just to test it.

I don't think want an extra field specific to script setup - ideally our entire suite would run the same for both script setup and regular composition components. One should not need to consider such implementation details when testing.

Either way, I'm guessing this won't cause any problems, so shall we merge it up?

afontcu

The downside is that you now can't test that something is not exposed by a component, but I don't think this is a problem.

Not sure if a bug… or a feature 😉

cexbrayat · 2021-09-20T08:18:16Z

As I mentioned on Discord, I think providing a way for users to easily modify the state of a component, or spy on it, is nice (that's why wrapper.vm exists).

But I get your point on not exposing things, so I don't mind if we don't merge this.

On the other hand, the PR should not have any impact, and we don't even need to document it: it works as expected. I think it's more surprising that expect(wrapper.vm.count).toBe(0) fails when switching to script setup if we don't do anything.

ghost · 2021-09-20T14:29:13Z

it's like when you make a private field public just to test it.

In the interest of writing tests faster, I would definitely leverage this if J-Unit could somehow access private variables at runtime only in the test environment. JS also doesn't have public/private variables to my knowledge, so that sounds like it might be a straw-man argument. In the rfc, it sounded like the reason <script setup> contents aren't exposed was because of parent/child interaction with refs being abused. Exposing them on the vm only in the test environment would make testing easier without undermining the intentions laid out in the <script setup> rfc (as long as it doesn't undermine that.. maybe it does?). I would definitely see that as a feature.

lmiller1990 · 2021-09-21T01:16:30Z

I think it's more surprising that expect(wrapper.vm.count).toBe(0) fails when switching to script setup if we don't do anything.

This is actually a good point. When moving to script setup, people will want to rely on their tests to help.

I think we should probably merge this; the public API isn't actually changing (right, just to confirm; this is my understanding). It just makes things work with <script setup> that didn't previously.

cexbrayat · 2021-09-21T05:39:27Z

@lmiller1990 I confirm the API does not change. If we spot regressions, I think we can reconsider, and expose it behind a mount option, or something else 👍

xanf · 2021-09-21T11:41:45Z

I have mixed feelings about it. While I still feel accessing wrapper.vm as strong antipattern (huge red flag for me on review) I was super happy that <script setup> is "private by default" and always considered this as a feature - we're receiving the wrapper.vm identical to behavior of ref in real code. This confuses me especially when we're combining things with expose to control what could/should be seen from outside

ghost · 2021-09-21T12:21:31Z

This confuses me especially when we're combining things with expose to control what could/should be seen from outside

Define "outside." Sure, parent components shouldn't have access to wrapper.vm but unit tests drive units of code and have had access to the vm for a long time. What's the danger there?

xanf · 2021-09-21T12:40:17Z

@micDropper I believe there is a common ground (no matter of mount/shallowMount preference) that testing a component should rely on observable behavior (effectively making a component as a blackbox) - this is crucial to make sure that unit tests do not depend on implementation details of the component (including, but not limited to names of fields in data, names of specific methods, etc.). For example, that's why I'm very positive about dropping setMethods / methods in VTU v2 - it makes harder to write bad tests (and yet possible if you need an escape hatch). I see ability to access wrapper.vm in the same way - while it might be required (to call $emit for example), having less opportunities to do so makes (IMO) better tests

ghost · 2021-09-21T14:52:45Z

@xanf There's a difference between encouraging and enforcing. Unit tests do depend on implementation details and there's no way to completely decouple them. Angular enforces everything, Vue's philosophy is that it is approachable. Some may want to test functionality through the template (VTU's api isn't the most friendly to this method of testing as it stands either, @testing-library/vue has a much friendlier api for testing through template interaction), others may want to black-box down to the method/computed level and test units of code there. It makes VTU unapproachable to unexpectedly require people to expose vm data manually just to adopt a nice sugar syntax.

xanf · 2021-09-21T15:19:43Z

Vue's philosophy is that it is approachable.

Sorry, maybe I'm missing something - where it is stated?

others may want to black-box down to the method/computed level and test units of code there.

First of all - by any means I'm not a member of Vue/VTU team, but I see VTU mission to prevent things which are considered "not best practices". I'm taking general direction of VTU as stated on this page https://next.vue-test-utils.vuejs.org/guide/essentials/easy-to-test.html which provides "general guidance" and explicitly states that we should not test implementation details

It makes VTU unapproachable to unexpectedly require people to expose vm data manually just to adopt a nice sugar syntax.

I struggle to draw a line between "encouraging" and "enforcing". For me "enforcing" is a scenario when no escape hatches are available and this is not the case - you still can do wrapper.vm.$.proxy if you want to access internals of the component.

My main concern about this change - I really would like VTU to be consistent in following goals. If setMethods was removed because "it encourages bad practices" and accessing things on wrapper.vm are considered bad practice - I think the decision should be similar

Having ability to do things in multiple ways is a real pain - we constantly feel it in GitLab where VTU v1 "everything is possible" approach resulted in so many approaches in codebase resulting migration even between minor versions of VTU v1 pain and a challenge, so I really liked the direction of VTU team of taking "opinionated" approach, while maintaining (or at least not preventing) escape hatches for everything

ghost · 2021-09-21T18:17:35Z

where it is stated?

"Approachable Versatile Performant"

^^Stated on the vue.js home page. https://vuejs.org/

"We don't want to scare people away and we want to make sure Vue stays approachable. " -Evan

^^Also stated by Evan You in a podcast with egghead.io

you still can do wrapper.vm.$.proxy

That's comforting to hear, but there again as originally pointed out by @cexbrayat, if it's still available, why break people's tests when they migrate to <script setup> when it could instead be available at wrapper.vm? That's a headache too.

we should not test implementation details

I do agree that tests should be tested through the template as much as possible. Many developers disagree though, and they often make good points too. You can encourage these developers to do it the right way without making their life harder.

Having ability to do things in multiple ways is a real pain - we constantly feel it in GitLab

If GitLab is struggling with consistency, I would say start unit testing your unit tests for behaviors you don't like (like usage of setMethods perhaps), improve testing guidance in your MR templates, emphasize code review, and consider using @testing-library/vue to help your developers test through the template easier.

xanf · 2021-09-21T18:32:40Z

That's comforting to hear, but there again as originally pointed out by @cexbrayat, if it's still available, why break people's tests when they migrate to <script setup> when it could instead be available at wrapper.vm? That's a headache too.

Sorry, I still believe that release of VTU 2.0 (without beta) is the rare point where we could evaluate "breaking peoples test" and it was done earlier. We can use same argumentation:

setMethods removal - if you can patch wrapper.vm.someMethod() - why break people's test when they migrate to VTU2?
is removal - if someone can do expect(wrapper.element.tagName).toBe(....) - why break people's test when they migrate to VTU2?
renderStubDefaultSltos ... why change this behavior?

I can continue, but I believe you've got an idea. Changes are very painful, and maintaining compatibility gives us very rare opportunities "to do things in the correct way (c)"

My position is straightforward - both consistency and encouraging people to write code in the way it is expected to be written are important

If GitLab is struggling with consistency, I would say start unit testing your unit tests for behaviors you don't like (like usage of setMethods perhaps), improve testing guidance in your MR templates, emphasize code review, and consider using @testing-library/vue to help your developers test through the template easier.

Thank you for your suggestions and everything you've mentioned is deployed (except @testing-library/vue). But for any tool we have it is very important to force people "to fall into the pit of success" - any guidelines/linters/etc. should be a second line defense

Again - I'm not against this MR at all, but it would be great to hear from the team about their vision and ensure that this vision stays consistent, or we will have even more such conversations otherwise.

afontcu · 2021-09-22T11:50:24Z

it would be great to hear from the team about their vision and ensure that this vision stays consistent, or we will have even more such conversations otherwise

I think you have explained it better than I would! 😄 When it comes to opinions on how to write tests, I think it is not black or white, but rather a spectrum. Vue Testing Library is highly opinionated. Vue Test Utils, on the other hand, is not. That doesn't mean, though, that the library should expose everything, allow everything, and accept everything. I think it is perfectly fine to favor one over others.

If releasing VTU 2 gives us the opportunity to address some past decisions (through breaking changes) and help define a testing style that most of us are comfortable with, why would anyone miss the opportunity?

I do believe this is what makes VTU (and Vue) "approachable". Soft learning curve, sensible defaults, coherent and helpful docs, and escape hatches for advances usages. Allowing everything isn't approachable ("Having ability to do things in multiple ways is a real pain").

cexbrayat · 2021-09-22T17:37:58Z

I think we all agree that tests should not tinker too much with component internals.

But in real life, it's sometimes very handy (especially to mock/spy on a function).

I'm afraid that we'll have an endless stream of issues from developers trying to test their script setup components and failing to access wrapper.vm (we already had some). I migrated a few average-sized projects and ran into this myself.
We can of course drop this and document it as an edge case, but I think we would not be helping the developers adopting script setup.

This PR does not change anything to existing projects, and it does not tell developers to use wrapper.vm more: it'll just work as expected 🙂

lmiller1990 · 2021-09-24T01:46:48Z

While I may not agree with the idea of testing internals (eg, using vm), I personally do think we should merge this. As it stands, not exposing vm may block many people with large suites trying to upgrade to script setup. "Just rewrite your tests" is a bit hostile, even if it's with positive intentions.

If we do not merge this, a user who'd like to test internals will just do expose anyway. Although the idea of not exposing vm is to encourage writing a different style of test, I don't think users will think "I should find another way to test this". More likely, they will just find a way to write their test and move on to the next task, like using expose, or some other hack.

If this was a new library with a clear, opinionated vision, I'd definitely not want this kind of feature. That said, I feel like at this point, VTU is old enough and widespread enough that our goal should be sustainability and long term longevity - eg, making sure things continue to work in the same manner they have done so in the past. In this case, that would mean making vm accessible in the same way when using script setup.

What does everyone else think? Happy to agree that testing against vm is not ideal. My main concern is that upgrading to script setup will break your existing tests, which is not a good look.

cexbrayat · 2021-09-24T04:38:58Z

@lmiller1990 👍Can you cut a release with this?

lmiller1990 · 2021-09-24T05:18:57Z

Yes https://github.com/vuejs/vue-test-utils-next/releases/tag/v2.0.0-rc.15

lmiller1990 · 2021-09-24T05:20:44Z

To follow up on this I think a vision or mission statement around what VTU is supposed to be is a great idea. It would definitely help guide development and decisions.

tangjinzhou · 2021-09-24T15:19:28Z

after udpate, we get wrapper.vm is empty object.

cexbrayat · 2021-09-24T15:50:11Z

TS can't see the properties yet, see #972

But you can just cast as explained in the issue and it should work

cexbrayat mentioned this pull request Sep 9, 2021

Setup and defineProps #862

Closed

cexbrayat force-pushed the fix/script-setup branch from 6334414 to 77cbbf6 Compare September 9, 2021 16:20

cexbrayat force-pushed the fix/script-setup branch from 77cbbf6 to 25dfe7f Compare September 9, 2021 16:51

cexbrayat commented Sep 9, 2021

View reviewed changes

cexbrayat commented Sep 10, 2021

View reviewed changes

afontcu approved these changes Sep 20, 2021

View reviewed changes

cexbrayat merged commit e80b7b2 into vuejs:master Sep 24, 2021

cexbrayat deleted the fix/script-setup branch September 24, 2021 04:43

cexbrayat mentioned this pull request Sep 24, 2021

Better types for wrapper.vm when the component is closed #972

Open

stevemandl mentioned this pull request Dec 19, 2023

VueWrapper typing problem vuejs/language-tools#3808

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose everything on wrapper.vm to help testing `script setup` #931

feat: expose everything on wrapper.vm to help testing `script setup` #931

cexbrayat commented Sep 9, 2021

cexbrayat commented Sep 9, 2021

cexbrayat Sep 9, 2021

lmiller1990 Sep 20, 2021

cexbrayat Sep 20, 2021

cexbrayat Sep 10, 2021

lmiller1990 Sep 20, 2021

cexbrayat Sep 20, 2021

xanf Sep 21, 2021

lmiller1990 commented Sep 20, 2021 •

edited

Loading

afontcu left a comment

cexbrayat commented Sep 20, 2021

ghost commented Sep 20, 2021

lmiller1990 commented Sep 21, 2021

cexbrayat commented Sep 21, 2021

xanf commented Sep 21, 2021

ghost commented Sep 21, 2021

xanf commented Sep 21, 2021

ghost commented Sep 21, 2021

xanf commented Sep 21, 2021

ghost commented Sep 21, 2021

xanf commented Sep 21, 2021

afontcu commented Sep 22, 2021

cexbrayat commented Sep 22, 2021

lmiller1990 commented Sep 24, 2021 •

edited

Loading

cexbrayat commented Sep 24, 2021

lmiller1990 commented Sep 24, 2021

lmiller1990 commented Sep 24, 2021

tangjinzhou commented Sep 24, 2021

cexbrayat commented Sep 24, 2021

feat: expose everything on wrapper.vm to help testing script setup #931

feat: expose everything on wrapper.vm to help testing script setup #931

Conversation

cexbrayat commented Sep 9, 2021

cexbrayat commented Sep 9, 2021

cexbrayat Sep 9, 2021

Choose a reason for hiding this comment

lmiller1990 Sep 20, 2021

Choose a reason for hiding this comment

cexbrayat Sep 20, 2021

Choose a reason for hiding this comment

cexbrayat Sep 10, 2021

Choose a reason for hiding this comment

lmiller1990 Sep 20, 2021

Choose a reason for hiding this comment

cexbrayat Sep 20, 2021

Choose a reason for hiding this comment

xanf Sep 21, 2021

Choose a reason for hiding this comment

lmiller1990 commented Sep 20, 2021 • edited Loading

afontcu left a comment

Choose a reason for hiding this comment

cexbrayat commented Sep 20, 2021

ghost commented Sep 20, 2021

lmiller1990 commented Sep 21, 2021

cexbrayat commented Sep 21, 2021

xanf commented Sep 21, 2021

ghost commented Sep 21, 2021

xanf commented Sep 21, 2021

ghost commented Sep 21, 2021

xanf commented Sep 21, 2021

ghost commented Sep 21, 2021

xanf commented Sep 21, 2021

afontcu commented Sep 22, 2021

cexbrayat commented Sep 22, 2021

lmiller1990 commented Sep 24, 2021 • edited Loading

cexbrayat commented Sep 24, 2021

lmiller1990 commented Sep 24, 2021

lmiller1990 commented Sep 24, 2021

tangjinzhou commented Sep 24, 2021

cexbrayat commented Sep 24, 2021

feat: expose everything on wrapper.vm to help testing `script setup` #931

feat: expose everything on wrapper.vm to help testing `script setup` #931

lmiller1990 commented Sep 20, 2021 •

edited

Loading

lmiller1990 commented Sep 24, 2021 •

edited

Loading