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

feat: Hide props with default values within snapshot #1174

Closed
wants to merge 5 commits into from
Closed

feat: Hide props with default values within snapshot #1174

wants to merge 5 commits into from

Conversation

freakzlike
Copy link
Collaborator

@freakzlike freakzlike commented Dec 23, 2021

This PR will avoid props with their default values within snapshot (Fixes #945).

<template>
  <my-component />
</template>

Will now be stubbed to <my-component-stub /> and no longer to <my-component-stub some-props="default-value" />.

This may lead to invalid snapshots and could be a breaking change?

I currently use JSON.stringify for object comparison but I want to replace it.

https://github.com/freakzlike/vue-test-utils-next/blob/efca188cf62bea1f0e4ed07c9cd4766016c1c71c/src/stubs.ts#L115-L116

// TODO: use some different object comparison
return JSON.stringify(defaultValue) === JSON.stringify(propValue)

Is there already any existing util for this? (I haved found one). If not I will create one.

@lmiller1990
Copy link
Member

Will this only impact shallowMount? Eg, for components using mount, I think all the prop values should be shown?

@freakzlike
Copy link
Collaborator Author

It only impact stubbed components so shallowMount and stubs in mount.
I'm also thinking if the props should be sorted by name. I faced some discrepancies of snapshots a while ago because I just changed the order of how the props are passed to the component.

@freakzlike
Copy link
Collaborator Author

I have added a util function deepCompare and determine the default values of the props during stub setup and not on every render

@lmiller1990
Copy link
Member

I'd like some more feedback on this one, maybe @xanf or @cexbrayat or @afontcu ?

My concern is PR adds a lot of complexity that I foresee as difficult to maintain and also will make it very hard to make changes without breaking snapshots.

I'm thinking that VTU should not even be concerned with snapshots; this is a Jest only feature, which can be heavily customized with their snapshot serializer API. Furthermore, different people have different opinions on snapshots, and I don't think we should be enforcing any opinions on them. Knowing the default prop value is likely important for some people.

Finally, this fix is specific to snapshots with shallowMount, a type of test which is going to be brittle and break at the slightest change to any implementation detail (good tests should be resilient to implementation details changing). How useful and beneficial is providing a highly opinionated snapshot, a feature where the desired behavior is different for each dev and testing framework?

I feel like this is going to cause a lot of overhead in the future and we may unintentionally break snapshots in minor versions. I'd like to get some more feedback before we merge this. Thanks for the work so far!

@freakzlike
Copy link
Collaborator Author

I understand your concerns. Another proposal could be to override the default stub generation by setting a function in the global mounting options. So something like:

const wrapper = shallowMount(MyComponent, {
  global: {
    createStubs: ({ name, component }) => {
      // Implement own logic to create a stub
      return defineComponent({
        name,
        render: () => // Custom render logic
      })
    }
  }
})

This will allow the user the customize the default stub generation when using shallowMount or stubs: { ChildComponent: true }. Maybe some generic function could also be provided by a plugin package

@cexbrayat
Copy link
Member

I also tend to think it adds too much complexity for the benefits...

@afontcu
Copy link
Member

afontcu commented Jan 5, 2022

Hi! First of all, thank you for your work on this, I actually learned a couple of things reading your code 🙌

That being said – I agree on the overall feeling of too much complexity – and also, a bit of opinionated. Assuming that component snapshot testing is useful, why wouldn't I get the default values of props? Or why wouldn't I want to break if prop order changes?

With that, I believe this PR should not be merged, as the behavior of snapshot testing is not something Vue Test Utils should control extensively.

Again, thanks!

@lmiller1990
Copy link
Member

If you want to make a snapshot serializer that implements this same behavior and need VTU to expose things, we could do that.

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.

Stubs rendering default props
4 participants