Skip to content

wrapper.setSelected() triggers a change event even if the option is already selected #1339

Closed
@vvanpo

Description

@vvanpo
Contributor

Version

1.0.0-beta.29

Reproduction link

https://codepen.io/vvanpo/pen/MWWroOK

Steps to reproduce

  1. Mount a component with a <select> element.
  2. Call setSelect() multiple times on one of the select element's options.
  3. Observe the <select> element wrapper's emitted('change').length is equal to the number of times setSelect() was called.

What is expected?

A <select> element emits a change event only when the element value has actually changed.

What is actually happening?

setSelected() triggers a change event every time it is called, regardless of whether the underlying value was changed.

Activity

vvanpo

vvanpo commented on Nov 2, 2019

@vvanpo
ContributorAuthor

I hesitated to open this as a bug, as the documentation is explicit in saying that trigger('change') is called every time: https://github.com/vuejs/vue-test-utils/blob/v1.0.0-beta.29/docs/api/wrapper/setSelected.md

However, I believe setSelected() is generally used to mimic user interaction, not programmatic interaction. So it follows that setSelected() should behave in the same way as a user interacting with a <select> element's <option>s, which I believe will only ever fire a change event if the selection has been modified.

dobromir-hristov

dobromir-hristov commented on Dec 15, 2019

@dobromir-hristov
Contributor

You are correct, it only emits change event if select another option. It shouldnt be hard to see if an option is selected.

codebryo

codebryo commented on Dec 17, 2019

@codebryo
Contributor

@vvanpo as you said and the docs explicitly state it triggers the change event every time I don't think this is a bug. I don't think the test-utils should keep track of data and know if something has changed between two calls. If setSelected() gets called multiple times, it's fair that it does it job multiple times.

dobromir-hristov

dobromir-hristov commented on Dec 17, 2019

@dobromir-hristov
Contributor

@codebryo I am a bit split here. I agree with you on that one, yet I saw that selecting the same option does not trigger an event... Do we want to emulate a user action or explicitly do what the action states.. hmmmm @afontcu @JessicaSachs Opinions here?

afontcu

afontcu commented on Dec 17, 2019

@afontcu
Member

If I'm not mistaken, setChecked is not sending the event if the element is already checked

if (this.element.checked === checked) {
return

...even though docs say the exact same thing regarding it being an alias of setting the right value and emitting the event.

I'd say all setXXXX methods to behave the exact same way. We could make them emulate the user or not, but in a consistent way. For the record, I'd like them to stay closer to the users.

vvanpo

vvanpo commented on Dec 19, 2019

@vvanpo
ContributorAuthor

@afontcu good find, it seems clear to me that emulating user actions is the most intuitive and useful behaviour, here.

Interestingly, that same is not true when calling setChecked() on an <input type="radio" />. It used to be, but was removed in ef66c26#diff-f8b21801dcf5dee76bc9f5f22e227ea8L516, which looks to me like a regression.

vvanpo

vvanpo commented on Dec 19, 2019

@vvanpo
ContributorAuthor

If there are no objections, I'll open a PR for this.

JessicaSachs

JessicaSachs commented on Dec 19, 2019

@JessicaSachs
Collaborator

Yes please, @vvanpo

added a commit that references this issue on Jan 3, 2020

Merge pull request #1380 from vvanpo/set-selected

916b07f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @codebryo@JessicaSachs@vvanpo@afontcu@dobromir-hristov

      Issue actions

        wrapper.setSelected() triggers a change event even if the option is already selected · Issue #1339 · vuejs/vue-test-utils