Skip to content

Clear/Reset emitted events #1252

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

Closed
jpenna opened this issue Jun 3, 2019 · 19 comments
Closed

Clear/Reset emitted events #1252

jpenna opened this issue Jun 3, 2019 · 19 comments

Comments

@jpenna
Copy link

jpenna commented Jun 3, 2019

What problem does this feature solve?

When building a component and checking for the events emitted by this component we use component.emitted('eventX')[0] and do the tests. On the next test, we build the component again and use the same assertion.

When the component is more complex, it would be better to just reset the emitted event instead of rebuilding the component. This is common behavior on testing libraries for function (sinon.js, Jest, for example).

What does the proposed API look like?

component.resetEmitted() would be enough.

I can try doing it if this is a desired feature.

@marcosnav
Copy link

I'd like this feature to be implemented, meanwhile I'm doing the following to reset/clear the emitted events:

component._emitted[event] = []

@lmiller1990
Copy link
Member

lmiller1990 commented Jan 3, 2020

Hi! Sorry it took so long for someone to follow up on this.

I see why this is a valid concern, but couldn't this be as easily solved with a factory function? Eg:

const createWrapper = () => {
  return mount(MyComplexComponent, {
    // ... a ton of options
  })
}

Then you test becomes:

test('It works', () => {
  const wrapper = createWrapper()
  // that's it!
})

You can also make createWrapper take arguments if you want to set data, props etc.
Likely you want to reset the entire component, not just the the emitted events? It seems more ideal to have an entirely fresh copy of the component for each test. There is a good article about it in the Vue.js cookbook: https://vuejs.org/v2/cookbook/unit-testing-vue-components.html#Real-World-Example

What do you think?

@jpenna
Copy link
Author

jpenna commented Jan 3, 2020

Hey @lmiller1990 ! This is an option, and that's how I was doing it, but recreating the component every time is a little over kill if we just want to test a behavior triggered from the same action again and again, for example.

In the case I'm talking about, I'd like to reset just the emitted events, like jest.clearAllMocks() or sinon's spy.resetHistory().

Sorry I don't have a real use case to include here right now.

@lmiller1990
Copy link
Member

lmiller1990 commented Jan 4, 2020

Thanks for the reply.

If there is a real performance bottleneck with recreating the component, I think that is something we should fix.

At this point, I don't think this is feature something we will be including in Vue Test Utils. If this was to become a feature one day, I'd be in support of a resetComponent method, that would reset everything to the base state. Since that can be accomplished with a factory function, though, this isn't something we will be including.

Thanks for your understanding!

@jpenna
Copy link
Author

jpenna commented Jan 4, 2020

I believe this is something that should be implemented, not only it would be useful, it would also follow the same functionality of other stub/spy test frameworks (which include this for a reason).

We shouldn't be modifying component._emitted[event] = [] directly to achieve this reset functionality...

@dobromir-hristov
Copy link
Contributor

I am also against this. You should not have to reset the emitted counter. If you do, then this is another test.
End of story.

@TheDutchCoder
Copy link

TheDutchCoder commented Jan 13, 2020

Consider the following test:

  it('can be disabled', () => {
    const wrapper = mount(ActionButton)
    const button = wrapper.find('button')
    button.trigger('click')

    // Expect the button to be active and respond to clicks.
    expect(wrapper.vm.disabled).toBe(false)
    expect(wrapper.emitted('click')).toBeTruthy()

    // Disable the button and expect it not to respond to clicks.
    wrapper.setProps({ disabled: true })
    button.trigger('click')

    expect(wrapper.vm.disabled).toBe(true)
    expect(wrapper.emitted('click')).not.toBeTruthy()
  })

It's a valid use-case to make sure an event gets emitted (or not emitted) based on a prop change.

This currently doesn't work because the emitters somehow are stored.

Changing it to expect(wrapper.emitted('click').length).toBe(1) works but is (in my opinion) very counter intuitive and now depends on the author tracking when and where and how many events were emitted.

@lmiller1990
Copy link
Member

lmiller1990 commented Jan 13, 2020

Tracking how many times emit was called should not be too difficult in a single unit test. Or are you using the same wrapper instance for many tests?

Alternatively, you could have two tests.

it('does emit when enabled', () => {
  const wrapper = mount(ActionButton, { 
    propsData: { 
      disabled: false 
    }
  })
  const button = wrapper.find('button').trigger('click')

  expect(wrapper.emitted('click')).toBeTruthy()
})

it('does not emit when disabled', () => {
  const wrapper = mount(ActionButton, { 
    propsData: { 
      disabled: true 
    }
  })
  const button = wrapper.find('button').trigger('click')

  expect(wrapper.emitted('click')).toBeFalsy()
})

Now you get more granular failures, since each test only deals with one situation.

I personally feel messing with the internals of a component during a test isn't not ideal, since that is a behavior Vue does not support (so you are testing against a non production behavior).

@gregveres
Copy link

I am really surprised to see the resistance to this. I agree with the comment above that this is very much like resetting the calls on a spy. I think one of the best use cases that can be put forth for this when you consider something like this for your tests:

describe('test my component'), () => {
  let wrapper;
  beforeAll(() => {
    wrapper = mount(...);
  });
  beforeEach(() => {
    wrapper.resetEmitted();
  });
});

When I have 10,000+ unit tests for my app, it matters if the test run can be shaved down in time by not recreating the mounted component for each and every test. This is a pattern I have used over and over again when using Spys. Emit is no different than a Spy. We want to knot that it got called, and each test run should have a fresh copy of the, without having to do too much work. (mounting the component for each and every test seems like too much work, even if it is fairly fast.)

@dobromir-hristov
Copy link
Contributor

So how do you reset your component between tests? How do you ensure each prop, data is untouched? Wont that be more work than just re-mounting it? Just thinking out loud here.

@gregveres
Copy link

There is often a whole series of tests that can be run on an object without it corrupting the object and needing to be reset. And sure, for those tests that need a fresh copy of the component, then create a new one.

Here is an example, let's assume that there are a number of getter values that are used to control how the object looks and those react and change based on the value of the store and how that store might change.

I can change the store over and over again and test those values are correct based on the current value of the store without having to remount the component. And in some of those, the component might be emitting events based on how the store changes and I want to test that, so all I have to do is reset the emit counter, change the store appropriately and verify the emit happens.

@lmiller1990
Copy link
Member

I feel like this could be a slippery slope - won't we then need to condition resetData, resetComputed, resetProps as well?

A previous comment said you can just do component._emitted[event] = []. Or say, component._emitted = [] for the full reset. The fact that this feature is already available in a one liner seems like it should be enough - are you just asking for a prettier syntax around component._emitted[event] = []?

If there is a performance issue, related to remounting components, we should definitely look into that, and find out where the bottleneck is.

@gregveres
Copy link

@lmiller1990 That work around didn't work for me. I don't know why, but it didn't work. I would have to dig into it again to see why it didn't.

I disagree that this has to lead to a slippery slop of resetting the other things. In fact, the other things are already spelled out in different guides as far as I remember.

I think the best way to think of this as being analogous to Spies in jest/jasmine. Why did Jest/Jasmine provide a reset() function on a spy? Why didn't they just say "just recreate the spy. We don't want to provide a way to reset the call counts"? I don't have insight into what they were thinking at the time, but it is exactly the same case as emit. This is something that the component can do over time and you could choose to reset it in the beforeEach().

@lmiller1990
Copy link
Member

Persisted state between unit tests is never a good idea - you should always remount your component for every test.

I'm happy to help write some code to reset the emitted events that people can add into the projects (like a plugin). I don't think this belongs in core - the API is already too large and complicated as is. We are already trying to cut back on features for the Vue 3 compat, adding new APIs at this point isn't something I think is ideal.

I think we should consider exposing an API for plugins, that way people can have whatever they want. If you want to try and implement this feature for your project, the code for emitted is here and we log events here. It should be as simple as resetting that array/object.

@gregveres
Copy link

@lmiller1990
So in your unit tests you never use BeforeAll / BeforeEach? That's what these are for, to do test suite setup.

@lmiller1990
Copy link
Member

lmiller1990 commented Feb 2, 2020

beforeAll and beforeEach have their place - we use them for global stubs and configure in this library, for example. I personally don't use them for component related setup, because I don't think it's good practice. You are, of course, welcome to use them how you see fit.

I'm sorry if this is disappointing to you, but I don't see this feature as common enough to justify increasing the API surface, considering how much work making this all work with Vue 3 is going to be. I'd be happy to review code and assist if someone reading this comment feels strongly enough this feature. The code could be used like a plugin with existing projects.

@ImTheC
Copy link

ImTheC commented Sep 24, 2021

A previous comment said you can just do component._emitted[event] = []. Or say, component._emitted = [] for the full reset.

That work around didn't work for me. I don't know why, but it didn't work. I would have to dig into it again to see why it didn't.

I also had trouble getting that workaround to work. After that line runs, emitted() no longer seems to function.

My team has ended up checking for emitted values by using .pop().pop(). This effectively clears the emitted values.

For example:

    const id = wrapper.emitted().delete.pop().pop()
    expect(id).toBe('A')

However, it will leave an empty array ([]) for that key. So if I test for wrapper.emitted().delete in a later test and am expecting it not to have been called then I have to do something like: expect(wrapper.emitted().delete.pop()).toBeUndefined().

@lmiller1990
Copy link
Member

What exactly is your use case? You can't create a fresh component?

@ImTheC
Copy link

ImTheC commented Sep 25, 2021

Personally, I'm a big proponent of creating a fresh component each time, at least when tests need to manipulate data within the component (e.g. setting props).

I submitted the above comment more so to help other people who find themselves in a similar situation where they are unable to (or unwilling to) create a fresh component.

However, saving time is usually the use case I'm given. Creating a fresh component for each test extends the amount of time it takes to complete the test suite. When running suites with a few tests, not such a big deal, but when running suites with a lot of tests the extra time adds up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants