remove setComputed

[eddyerburgh]

The setComputed method changes the value of computed properties permanently.

That means the instance is in a state that is impossible to reach in production. This could lead to bugs that slip through tests because a component behaves differently in tests versus production.

What do people think?

[lmiller1990]

Is using setComputed different to using the computed options object passed to shallow?

One of my main use cases I feel is might be related is when I have computed functions like this:

all () {
  return this.$store.state.members.all
},

allIds () {
  return this.$store.state.members.allIds
},

then I need to do

import { createLocalVue, shallow } from 'vue-test-utils'
import Vuex from 'vuex'

const localVue = createLocalVue()
localVue.use(Vuex)
let store

beforeEach(() => {
  store = new Vuex.Store({
    state: {
      members: {
        all: {}, allIds: []
      }
    }
  }
})

// write some test to see if a component is rendered by looping `allIds`, 
// and another to see what happens if there if `allIds` is empty...

Which is a huge amount of boilerplate. So I normally do:

const wrapper = shallow(Foo, {
  computed: {
     all: {},
     allIds: []
})

which saves making the Vuex store. I believe if you are just doing data tests, which involved seeing how the UI looks based on some state, you should not use an actual store, but simply computed and setComputed, since a store is an implementation detail, which is not something we should be testing.

Do you have an alternative in mind that would allow for a computed value to be changed? Or is setComputed a bad pattern in general? Does setData stuff the same problem?

[Alex-Sokolov]

I think better persist setComputed

For example, if we have 3 computed variables: a, b, c
a depend on some properties
b depend on some other properties
c depend on a and b.

When we write tests for c with setComputed we can just "switch" results of a and b and assert that result c is correct. If we didn't have setComputed we need to change (maybe) a huge amount of properties, which change a and b and later check c

[Austio]

Was some good discussion in this thread a while back. #55 (comment)

[LinusBorg]

@Alex-Sokolov I don't think you should test a computed prop like that in a component unit test, it's essentially testing implementation details instead of testing the result of the component.

If on a rare ofccation a computed prop is so complicated that they need to be tested individually, do that outside of a component unit test and test the function on its own.

As I agree with the risks @eddyerburgh mentions (I brought them up, in fact), I vote to remove it.

[lmiller1990]

If removed, will we still be able to set computed by passing it to the constructor options? eg:

shallow(Foo, {
  computed: {
    someVal: () => true
  }
})

?

[eddyerburgh]

Yes @lmiller1990

[lmiller1990]

Ok then I am happy to see setComputed leave.

[jonnyparris]

@LinusBorg 'Complicated' computed variables in my case are fairly simple methods but they can rely on a fairly long chain of dependencies on other computed variables. Testing them separately would be overkill.

It's not clear how the original concern of setComputed providing a permanent mock is not addressed by clear docs as opposed to removing it completely. It seems like there's only downsides to this.

[yx296]

Don't remove setComputed! Sad!

Suppose I have the following component:

import { mapState } from 'vuex';
import externalDependency from '...';

export default {
  name: 'Foo',
  computed: {
    ...mapState(['bar'])
  },
  watch: {
    bar () {
     externalDependency.doThing(this.bar);
    }
  }
}

When testing, I want to ensure that externalDependency.doThing() is called with bar (which comes from the vuex state) like so:

it('should call externalDependency.doThing with bar', () => {
  const wrapper = mount(Foo);
  const spy = jest.spyOn(externalDependency, 'doThing');

  wrapper.setComputed({bar: 'baz'});

  expect(spy).toHaveBeenCalledWith('baz');
});

[eddyerburgh]

Mocking computed is dangerous, because you can put your component into a state that it can't be in during production. The concern with setComputed is that it encourages users to mock the computed value.

You could achieve the same result by overwriting the computed option when you mount the component:

const wrapper = mount(Foo, {
  computed: {
    bar() {
      return 'baz'
    }
  }
})

[jonnyparris]

Isn't setComputed just a helper for overwriting the computed option more tersely?

On Thu, 15 Mar 2018, 08:25 Edd Yerburgh, ***@***.***> wrote: Mocking computed is dangerous, because you can put your component into a state that it can't be in during production. The concern with setComputed is that it encourages users to mock the computed value. You could achieve something similar by overwriting the computed option. const wrapper = mount(Foo, { computed: { bar() { return 'baz' } } }) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#331 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGoAC5pDMDi0F2R3woz6ef_VKviREO1ks5teiWIgaJpZM4RUVmq> .

-- sent from phone (possible typos)