Default props are set as attributes when mounting a stubbed component

[horyd]

Version

1.0.0-beta.31

Reproduction link

https://codesandbox.io/s/eloquent-mirzakhani-1im92?fontsize=14&hidenavigation=1&theme=dark

Steps to reproduce

See CodeSandbox. The HelloWorld component sets a default prop on id which the stub is reflecting (you can see the printed snapshot in the test even though it errors because it can't save it)

What is expected?

I think I would expect not too see the default prop "id" in the snapshot as it is part of the stubbed components logic - logic you would expect to not be executed because of the stubbing action

What is actually happening?

The default prop is set and visible in the snapshot

---

I'm generally interested on the design thoughts on this one and whether it constitutes a legitimate bug. For me I am seeing "id" props being set by vue-material MdInput components in a shallow mount. These ids have random characters in them which is causing my snapshots to be different each time. So in a shallow mount I wouldn't expect this interference from the component in the wrapper.

[thejcannon]

I agree with you @horyd . If I upgraded my version of a dependency, which added some attributes, I wouldn't expect my tests to fail as I haven't changed how I interact with the components in that dependency.

Not to mention, some components have a lot of attributes. So seeing them all is quite verbose.

[lmiller1990]

I'm reworking the stubbing logic for the next version of VTU - which also is aiming to support Vue 3. I am not sure how many of these fixes will be back ported to Vue 2's support, though. Not sure we need more breaking changes.

Anyway, For context of other readers, the output is

<helloworld-stub
    id="12345"
    msg="Hello Vue in CodeSandbox!"
  />

And the usage is <HelloWorld msg="Hello Vue in CodeSandbox!" />. The argument is that id should not appear in the snapshot, since that prop is not passed - it's rendering the default value. msg, on the other hand, should be renderer in the snapshot, because the user explicitly passes that prop.

I'm not convinced we should show props passed by the user, but not default props - I think stubs and shallowMount should stub everything, so the snapshot would show just be helloworld-stub. I personally don't use snapshots, though, so I don't have a strong opinion. I think it would be difficult for the snapshot serializer to figure out which props are and are not default, though. What do you guys think?

Interesting in @dobromir-hristov @JessicaSachs and @afontcu 's opinions, since this is something I think we need to define before making any more changes. The docs should also reflect this, with some examples.

[afontcu]

I think stubs and shallowMount should stub everything, so the snapshot would show just be helloworld-stub

I'm in favor of this 👍 – I would not show any prop/attribute from the stubbed out component, as the stub should be a simple placeholder to speed up the test or avoid a more complex test setup.

[lmiller1990]

Is this a problem of VTU actually? Doing wrapper.html() never shows props. Perhaps this is related to serializing the snapshots?

[horyd]

Is this a problem of VTU actually? Doing wrapper.html() never shows props. Perhaps this is related to serializing the snapshots?

The following assertion does pass though expect(wrapper.find("helloworld-stub").attributes("id")).toBe("12345");

[lmiller1990]

Hm. Do you think it should pass through @horyd ? I'm in favor of making stubs truly "stubs" and literally just a shell that has no props, attrs etc. Now is a good chance to make these changes since Vue 3 is in alpha and we can potential add something to the codebase to make VTU better.

[horyd]

This adjustment to getCoreProperties logic on the props key strips out the default logic successfully (likely others too, but it did succeed then in producing a snapshot without the id in my example)

props: Object.keys(componentOptions.props || {}).reduce((p, k) => {
      p[k] = { type: componentOptions.props[k].type }
      return p;
    }, {})

Overall though I think that I would prefer it for VTU to retain the attributes and handlers passed to stubs. The reason behind this is that, often enough, there can be thin component abstractions over basic HTML nodes. Take for instance an input element and the md-input vue-material input field. There is a significant overlap in their attribute/props APIs. It's reasonable that I may have render logic in a form to add the disabled prop to a certain md-input - but if props are scraped from stubs then I won't be able to test that that input is "disabled" in a certain test scenario. The same could apply in a scenario of adding a className synonymous with an error to a component and not being able to see that either. And I wouldn't want to mount in either of these situations here because I'm not interested in testing the md-input (or other components) internals.

Another neat thing with the current stubbing logic is that you can do this on something like the vue-material button that implements it's own click handler (and therefore cannot be accessed with trigger)
wrapper.find("MdButton").vm.$emit("click")
Broadly the same applies for all event handler logic where you want to test not the instance method itself but the handling of the emission of an event from a child component.

Wrapping up, imo, the act of passing certain props from the component being tested to its child components can often encapsulate logic that is not unreasonable to desire testing coverage on.

[horyd]

Ah, but I do think that the default props logic should be stripped out though, theres no reason for that to ever be asserted on from a parent component and as shown it can muck up the snapshot rendering

[lmiller1990]

Great ideas and discussion all.

I have some thoughts about

but if props are scraped from stubs then I won't be able to test that that input is "disabled" in a certain test scenario

I'm not sure this is really a good practice or something VTU should encourage. If you want to test a form is disabled, to me, it would make the most sense to click the submit button, and verify that the form submission logic did not trigger, no?

For forms in general, it makes way more sense to me to just use mount and test it like a user would - entering values and clicking buttons. Is there a reason you prefer to use shallow mounting?

(and therefore cannot be accessed with trigger) wrapper.find("MdButton").vm.$emit("click")

I think we need to rethink this, too, I think. This particular example feels pretty close to testing a implementation detail, instead of the public API. I don't think stubs should even have a VM - our stubs are a bit too smart for my liking. I don't have a great solution for this, at the moment, though.

Ah, but I do think that the default props logic should be stripped out though, theres no reason for that to ever be asserted on from a parent component and as shown it can muck up the snapshot rendering

I don't have an informed opinion on snapshots - I don't use them. Either way, the generated DOM come from Vue, not VTU - all VTU does is call outerHTML (or something similar) to get the current DOM. I think how the DOM is serialized and what props appear is outside the scope of VTU.

No right answers, just some food for thought. We need to make some big changes to support Vue 3, and in doing so, have a chance to rethink the API and improve the DX.

[horyd]

For forms in general, it makes way more sense to me to just use mount and test it like a user would

Maybe, but there are so many cases where the public API of the child components is more than enough to indicate user actions, without the overhead of mounting (and the difficulty of finding the element you're looking for in mounted wrappers and other potential side effects of mount).

Is there a reason you prefer to use shallow mounting?

Because I'm not interested in any interference by child components and their internal logic when I'm focussing on testing its parent component.

This particular example feels pretty close to testing a implementation detail, instead of the public API

If you consider components as first-class citizens then they're really no different from any given DOM element which you can trigger different actions on. Even in cases where the component is not a thin abstraction (such as MdButton over button), most tests can be understood as interactions between the APIs of each of these components, and are in fact best understood this way imo. I'm not sure I see how there's a difference between the following:
wrapper.find('button).trigger('click') and wrapper.find('md-button-stub).vm.$emit('click'). With the first you understand an inextricable link between the click event and the button instance, and the following example is the same, the click event and the MdButton instance. The logic is, in both cases, perfectly understandable (as child component public APIs should be)

I think how the DOM is serialized and what props appear is outside the scope of VTU.

Unless I'm mistaken, the distinction between default and regular props are lost at the point where the DOM is serialised. It's within the domain of VTU to choose how and what to stub right?

I'm coming into Vue from React, where Enzyme is a reasonably mature testing utility.
You can see a CodeSandbox of how it operates with the prop function invocation here and read the docs here for that example.

You can see in that example also how the components defaultProps is respected, however that can work slightly differently in Vue, which allows more dynamic values because the default key can be supplied a function that resolves for the default on every instantiation. (see updated CodeSandbox here for that). Unfortunately that's exactly what vue-material is doing 😂

[lmiller1990]

What kind of interactivity does Enzyme give you for stubs?

Regarding the snapshot serialization, maybe you are right - I need to review how stubs work again. I think they will change a bit for the next iteration of this library for Vue 3, since they are pretty buggy, so now is a good chance to decide what they should and shouldn't do.

I think we should make the more test friendly, so instead of find('md-button-stub').vm.$emit something like find('md-button-stub').trigger might be nicer - if components are first class citizens, we should try to use the same methods we use on actual DOM elements.

[protiumx]

Any update on this?

[lmiller1990]

No update on this, most of my time is on Vue 3 integration and vue-jest. I am also not sure how big a problem this really is - the point of snapshots really is to tell you if anything changed, so this isn't really blocking anyone as far as I can tell. Thinking about this more now, it seems like having the default prop isn't really a bad thing - if you changed that unintentionally, the snapshot would inform you (good thing, right?)

Do you feel strongly about this @protium-dev? What do you think? Is there a specific point in the discussion about you would like to see some action on (a few things were discussed).

[markrian]

I think the most painful aspect of this is when you add an optional prop to a component. That should be a backwards-compatible change (assuming its default value leads to no change in behaviour), but shallowMounted snapshots on components which consume the changed component would then fail.

Worse, a fully mounted version of the same test would pass. This is the opposite of what I would expect, since one of the main reasons to use shallowMount over mount is to ignore the implementation details of child components. shallowMount actually exposes the implementation detail of optional props and their default values.

If a newly-added optional prop or an existing optional prop's value is important in a given test on another component, that's probably more of an integration test, and shallowMount probably shouldn't be used anyway.

Since others have argued in favour of the current behaviour, WDYT of making the default serialisation/stubbing logic of shallowMount configurable? There is the stubs argument of course, but I mean a way to configure the behaviour globally.

[lmiller1990]

I have not looked into snapshot serializing much, but I wonder if it would make more sense for this to be customizable via an option in the snapshot serializer, as opposed to in VTU.

[ebisbe]

#1564 (comment)