vuejs / test-utils

Vue Test Utils for Vue 3
https://test-utils.vuejs.org
MIT License
1.04k stars 244 forks source link

Feature request: Clear events returned by "VueWrapper.emitted" method (similar to Jest mockClear) #1363

Closed iliubinskii closed 7 months ago

iliubinskii commented 2 years ago

Is your feature request related to a problem? Please describe.

Consider the following code:

test("Description", () => {
  const wrapper = vueTestUtils.mount(...); 
  ...
  expect(wrapper.emitted()).toStrictEqual([event1]);
  ...
  expect(wrapper.emitted()).toStrictEqual([event1, event2]);
  ...
  expect(wrapper.emitted()).toStrictEqual([event1, event2, event3]);
}

I would like to be able to write it this way:

test("Description", () => {
  const wrapper = vueTestUtils.mount(...); 
  ...
  expect(wrapper.emitted()).toStrictEqual([event1]);
  wrapper.clearEmitted();
  ...
  expect(wrapper.emitted()).toStrictEqual([event2]);
  wrapper.clearEmitted();
  ...
  expect(wrapper.emitted()).toStrictEqual([event3]);
}

Describe the solution you'd like

Add "clearEmitted" methods to "VueWrapper" interface.

cexbrayat commented 2 years ago

Could be interesting. @lmiller1990 WDYT?

lmiller1990 commented 2 years ago

This comes up every so often and the answer is always the same; why is this better than just writing another test with a new mount to get a fresh VueWrapper?

Historical thread (I'm sure there are others): https://github.com/vuejs/vue-test-utils/issues/1252

The maintainers opinion was not the most popular at the time (see the thread). What does Angular do for this @cexbrayat? Is there some kind of "reset" or is the standard just create another test/mount a new component? This is what React does, iirc.

This could certainly be implemented using our plugins API? I feel like this feature is only useful for a subset of people, who could easily be served by a copy + paste plugin (which could also be distributed via npm).

Always good to consider new features. I think we should have a hard stop on absolutely no API changes until 2.0 is out, though (two remaining failing tests: https://github.com/vuejs/test-utils/pull/1293), after which we can look into more enhancements. I like to keep the API surface pretty small; even as it stands, the code base is pretty complex and doesn't have a ton of active contributors - more code always leads to move maintenance overhead.

What do you guys think?

If there's a really compelling case for resetting emitted events, please point it out - happy to have a discussion about this feature. From what I can see it's just a convenience thing, and since it can be done quite easily in a plugin (I think?) that might be the best alternative.

iliubinskii commented 2 years ago

why is this better than just writing another test with a new mount to get a fresh VueWrapper?

  1. The same question can be asked about Jest's "mockClear" function. Why is "mockClear" better than just writing another test? Obviously, Jest developers decided that in some cases "mockClear" is better, because they added it.
  2. "clearEmitted" is better if test has significant overhead. Imagine that you need to define props (e.g. Quasar table component has 63 props) + define slots (e.g. Quasar table component has 19 slots) + define subcomponent (e.g. to receive injections of the tested component) + reach certain state of the component (e.g. show dialog). All of this can require overhead of 10+ lines. Duplicating such overhead is less convenient than using "clearEmitted".

This could certainly be implemented using our plugins API

Thx for the hint. I'll examine it.

lmiller1990 commented 2 years ago

The same question can be asked about Jest's "mockClear" function. Why is "mockClear" better than just writing another test? Obviously, Jest developers decided that in some cases "mockClear" is better, because they added it.

My understanding is that if you

const mocked = jest.mock('someFn')

In your test, there is literally no way to "unmock" or "reset" it. Whatever changes is persistent for all tests. This isn't the case with a Vue component, since you can just remount it or write a new test. They are not globally registered like Jest mock. So that's why mockClear would be essential in Jest - there's no other alternative.

"clearEmitted" is better if test has significant overhead. Imagine that you need to define props ...

What do your tests looks like? Wouldn't it just be

function mountComponent () {
  return mount(QuasarComponent, { /* 63 props */ }
}

it('case A', () => {
  const wrapper = mountComponent()
  wrapper.find('#a').trigger('click')
  expect(wrapper.emitted()[aa']).toEqual([ /* something *] ])
})

it('case B', () => {
  const wrapper = mountComponent()
  wrapper.find('#b').trigger('click')
  expect(wrapper.emitted()['b']).toEqual([ /* something *] ])
})

A tiny bit more code than you'd have with resetEmits. If you really want one test, wouldn't you just make the two assertions in the same test?

Sorry for so many questions - just wanting to see if there's really a use case here I'm missing.

lmiller1990 commented 2 years ago

Edit: I tried to implement this as a plugin but it's not currently possible - events is a private, readonly value.

Happy to leave this as a feature suggestion but unless there's a case where there's a test you cannot write with VTU, I don't think we should add this, at least certainly not before 2.0.0 - the current goal is to reach a stable release before adding new features.

iliubinskii commented 2 years ago

For anyone who has the same problem below is my temporary solution:

interface WrapperExtension {
  readonly clearEmitted: () => void;
}

type ExtendedWrapper<T extends VueWrapper> = T & WrapperExtension;

function extendWrapper<T extends VueWrapper>(wrapper: T): ExtendedWrapper<T> {
  Reflect.set(wrapper, "clearEmitted", (): void => {
    for (const events of Object.values(wrapper.emitted())) events.length = 0;
  });

  return wrapper as ExtendedWrapper<T>;
}

// Use it this way:

const wrapper = extendWrapper(mount(Component));

wrapper.clearEmitted();
lmiller1990 commented 2 years ago

Oh neat - maybe we can implement this as an option plugin. I forgot about Reflect. I will give this another try.

lmiller1990 commented 2 years ago

Was there any reason you can't use my original suggestion of a factory function to make creating a new wrapper more concise? I'd like to either decide 1) we need this feature (not a fan) or 2) close if we don't need it.

I've been burned by adding a feature that's only useful for a small number of use cases, which is why my default is "push back until we have a very compelling use case".

iliubinskii commented 2 years ago

Ok, here is the problem again:

test("Sample test", () => {
  const wrapper = vueTestUtils.mount(Table, {
    global: testUtils.globalMountOptions(),
    props:
      columns: [
        {
          align: "left",
          field(row) {
            return cast.string(reflect.get(row, "name"));
          },
          label: "Sample label",
          name: "column",
          sortable: true
        }
      ],
      pagination: {
        descending: false,
        page: 5,
        rowsPerPage: 0,
        sortBy: "column"
      },
      rowKey: "id",
      rows: [
        { id: "key1", name: "Sample row 1" },
        { id: "key2", name: "Sample row 2" },
        { id: "key3", name: "Sample row 3" },
        ...
      ]
  });

  wrapper.find(".first").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 0,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);

  wrapper.find(".prev").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 0,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 4,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);

  wrapper.find(".next").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 0,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 4,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 6,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);

  wrapper.find(".last").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 0,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 4,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 6,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 10,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);

  wrapper.find(".page-7").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 0,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 4,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 6,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 10,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ],
    [
      {
        descending: false,
        page: 7,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);
});

My solution:

test("Sample test", () => {
  const wrapper = vueTestUtils.mount(Table, {
    global: testUtils.globalMountOptions(),
    props:
      columns: [
        {
          align: "left",
          field(row) {
            return cast.string(reflect.get(row, "name"));
          },
          label: "Sample label",
          name: "column",
          sortable: true
        }
      ],
      pagination: {
        descending: false,
        page: 5,
        rowsPerPage: 0,
        sortBy: "column"
      },
      rowKey: "id",
      rows: [
        { id: "key1", name: "Sample row 1" },
        { id: "key2", name: "Sample row 2" },
        { id: "key3", name: "Sample row 3" },
        ...
      ]
  });

  wrapper.find(".first").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 0,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);
  wrapper.clearEmitted();

  wrapper.find(".prev").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 4,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);
  wrapper.clearEmitted();

  wrapper.find(".next").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 6,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);
  wrapper.clearEmitted();

  wrapper.find(".last").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 10,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);
  wrapper.clearEmitted();

  wrapper.find(".page-7").trigger("click");
  expect(wrapper.emitted("update:pagination")).toStrictEqual([
    [
      {
        descending: false,
        page: 7,
        rowsPerPage: 0,
        sortBy: "column"
      }
    ]
  ]);
});

Your solution with factory:

test("Sample test", () => {
  // eslint-disable-next-line @typescript-eslint/explicit-function-return-type
  function createWrapper() {
    return vueTestUtils.mount(Table, {
      global: testUtils.globalMountOptions(),
      props:
        columns: [
          {
            align: "left",
            field(row) {
              return cast.string(reflect.get(row, "name"));
            },
            label: "Sample label",
            name: "column",
            sortable: true
          }
        ],
        pagination: {
          descending: false,
          page: 5,
          rowsPerPage: 0,
          sortBy: "column"
        },
        rowKey: "id",
        rows: [
          { id: "key1", name: "Sample row 1" },
          { id: "key2", name: "Sample row 2" },
          { id: "key3", name: "Sample row 3" },
          ...
        ]
    });
  }

  {
    const wrapper = createWrapper();

    wrapper.find(".first").trigger("click");
    expect(wrapper.emitted("update:pagination")).toStrictEqual([
      [
        {
          descending: false,
          page: 0,
          rowsPerPage: 0,
          sortBy: "column"
        }
      ]
    ]);
  }

  {
    const wrapper = createWrapper();

    wrapper.find(".prev").trigger("click");
    expect(wrapper.emitted("update:pagination")).toStrictEqual([
      [
        {
          descending: false,
          page: 4,
          rowsPerPage: 0,
          sortBy: "column"
        }
      ]
    ]);
  }

  {
    const wrapper = createWrapper();

    wrapper.find(".next").trigger("click");
    expect(wrapper.emitted("update:pagination")).toStrictEqual([
      [
        {
          descending: false,
          page: 6,
          rowsPerPage: 0,
          sortBy: "column"
        }
      ]
    ]);
  }

  {
    const wrapper = createWrapper();

    wrapper.find(".last").trigger("click");
    expect(wrapper.emitted("update:pagination")).toStrictEqual([
      [
        {
          descending: false,
          page: 10,
          rowsPerPage: 0,
          sortBy: "column"
        }
      ]
    ]);
  }

  {
    const wrapper = createWrapper();

    wrapper.find(".page-7").trigger("click");
    expect(wrapper.emitted("update:pagination")).toStrictEqual([
      [
        {
          descending: false,
          page: 7,
          rowsPerPage: 0,
          sortBy: "column"
        }
      ]
    ]);
  }
});

Why I prefer "clearEmitted" method:

  1. Factory solution results in creating more component instances. Components may be quite heavy. My tests already take 15 seconds with parallelization and 40 seconds without parallelization. If I create new component instance for each small action I guess it will be several minutes.
  2. Factory solution triggers "@typescript-eslint/explicit-function-return-type" error because it is impossible to easily define return type of the factory function (in fact wrapper's type is supposed to be defined automatically). So, I'll need to polute code with a lot of "// eslint-disable-next-line" exclusions.
  3. Jest has native methods like "mockClear", "mockReset", "mockRestore". Why don't they force me to create factories?
  4. I guess that discussions about adding or not adding "clearEmitted" method already took hours, while simply adding it requires literally 3 lines of code:
    function clearEmitted(wrapper: VueWrapper): void {
    for (const events of Object.values(wrapper.emitted())) events.length = 0;
    }

I still prefer to have official solution because the above function is a hack that may stop working in the future.

lmiller1990 commented 2 years ago

Why not just

wrapper.find(".last").trigger("click");
expect(wrapper.emitted("update:pagination")[0]).toStrictEqual(/*...*/)

wrapper.find(".last").trigger("click");
expect(wrapper.emitted("update:pagination")[1]).toStrictEqual(/*...*/)

wrapper.find(".last").trigger("click");
expect(wrapper.emitted("update:pagination")[2]).toStrictEqual(/*...*/)

It's the same amount of lines of code. What do you think? Did you consider just indexing the emitted event array, or this won't work for your use case?

Again - just pushing back to make sure we aren't adding a specific method for a niche use case. It looks like you can write the same test in as concise a method by just indexing the emitted events. I'm not pushing back because I don't like the idea of new features for a more ergonomic testing experience, I'm pushing back because it looks like you can accomplish what you want with the existing API.

iliubinskii commented 2 years ago

Why not just

  1. IMHO, when I have several blocks inside one test I prefer to keep them independent of each other whenever possible, which means that interchanging these blocks should not break test. In your example, if you interchange blocks they will break because of indexes ([0], [1], [2]).

  2. Jest's jest-extended package has "nthCalledWith" matcher that allows you to do this:

    
    const callback = jest.fn();

callback("abc1"); expect(callback).nthCalledWith(0, "abc1");

callback("abc2"); expect(callback).nthCalledWith(1, "abc2");

callback("abc3"); expect(callback).nthCalledWith(2, "abc3");

So, according to your logic, they should remove "mockClear". However, they let me do this:
```ts
const callback = jest.fn();

callback("abc1");
expect(callback).toHaveBeenCalledWith("abc1");
callback.clearMock();

callback("abc2");
expect(callback).toHaveBeenCalledWith("abc2");
callback.clearMock();

callback("abc3");
expect(callback).toHaveBeenCalledWith("abc3");
callback.clearMock();

The last sample has even one extra line per block (3 lines vs 2), but these blocks are completely independent and interchangable which is IMHO an advantage.

Mabe it is possible to attach some kind of vote to this thread.

lmiller1990 commented 2 years ago

I think you've made a reasonable case for the feature. I don't agree with it, but I don't think it's unreasonable, so happy to solicit a little more feedback from the wider community. Generally people will :+1: something they like, maybe we can wait a little and see if anyone else feels strongly about this feature. If so, we can look to implement it.

A good first start to move this forward would be to actually create a PR adding this feature - it'll give more visibility, so people can vote on it, and we'll see if there's any unexpected complexity. What do you think? Would you be interested in this?

Once we've seen the technical complexity, I can share this on Twitter (PR or issue) and get some more :eyes: on it.

JessicaSachs commented 2 years ago

I tried to implement this as a plugin but it's not currently possible - events is a private, readonly value.

This is the biggest gripe I have against resetting emitted -- events are immutable within the Vue instance. Resetting them is not production-like, and Vue is currently implemented with the understanding that events are a readonly collection.

BUT what I am curious about is an improved API for emitted that essentially allows you to pass in spies (from Vitest, Jest, Cypress) and "auto-spy" on stuff, and reset your spies at will. I think that would:

  1. give you fine-grained control
  2. be framework agnostic
  3. let test frameworks use spies, which lets some of the fancier matchers work against emitted events
lmiller1990 commented 2 years ago

I'd like to see some way to inject jest.spy or sinon.spy into emitted - then instead of re-implementing jest.spy (or jest.fn) we could leverage the runner's implementation. This would let the OP accomplish what he'd like to do in a more native fashion.

freakzlike commented 2 years ago

I think it is already easy possible to use a spy function with emitted, when it's passed as props and on-prefix

const TestComponent = defineComponent({
  emits: ['click'],
  template: '<button @click="click">Click</button>',
  setup(props, { emit }) {
    return {
      click: () => emit('click', 'some-value')
    }
  }
})

it('should check emit with spy function', async () => {
  const onClick = vi.fn()
  const wrapper = mount(TestComponent, {
    props: {
      onClick
    }
  })

  await wrapper.find('button').trigger('click')
  expect(onClick).toHaveBeenCalledTimes(1)
  expect(onClick).toHaveBeenCalledWith('some-value')

  onClick.mockClear()
  expect(onClick).not.toHaveBeenCalled()

  await wrapper.find('button').trigger('click')
  expect(onClick).toHaveBeenCalledTimes(1)
  expect(onClick).toHaveBeenCalledWith('some-value')
})
cexbrayat commented 7 months ago

I recently used the same trick that @freakzlike commented above, and I think it does a good enough job instead of complicating the VTU APIs for this use case 👍