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

docs: click event without change event does not update checkbox value without attachTo: document.body #1470

Closed
cefn opened this issue May 2, 2022 · 12 comments · Fixed by #1972
Labels
docs good first issue Good for newcomers

Comments

@cefn
Copy link

cefn commented May 2, 2022

Describe the bug

A click() on an input[type="checkbox"] in @vue/test-utils doesn't seem conformant with Chrome. It doesn't cause a change to the vm instance's boolean ref, which is bound by v-model. A watcher of that ref gets no update notification when the checkbox gets a click.

The expected behaviour (that 'click' changes the ref, hence triggers a watcher) CAN be proven interactively in Chrome with this component mounted in a Vite app. It can be proven by either clicking the input or calling .click() on the element from the console. In the Vite-hosted App.vue the selectionHandler watcher of the bound boolean is simply console.log and behaves as expected - reporting the boolean value on every click.

To Reproduce

See the repro at cefn/vite-reactive-repro#3 based off the automated Vue3+Typescript scaffolding created by an out-of-the-box Vite project. You can observe the correct behaviour in Chrome by running pnpm install then pnpm run dev and experiment interactively in the browser. You can observe the broken @vue/test-utils behaviour by running pnpm run test.

image

Expected behavior

I expected the @vue/test-utils test at bad0970 to pass. I expected to be able to call just await containerCheckbox.trigger("click") in order for a v-model bound boolean variable change to propagate to the watcher. I didn't expect to need to explicitly call await containerCheckbox.trigger("change") to double-announce the change.

The missing change event is likely a regression as I discovered this when migrating components and their tests from Vue2, so it could be an obstacle for those upgrading and who assume click will cause change.

We need to port lots of components and having @vue/test-utils align with the behaviour of the previous version will help us with migration.

Related information:

  • @vue/test-utils version: 2.0.0-rc.21
  • Vue version: 3.2.31
  • node version: v16.14.2
  • npm (or yarn) version: pnpm --version is 6.32.10
@cefn cefn added the bug Something isn't working label May 2, 2022
@cexbrayat
Copy link
Member

Hey @cefn

I think this is expected. If I remember correctly, if you want the same behavior as in VTU v1, you need to add attachTo: document.body. Does that fix the issue in your example?

@cexbrayat cexbrayat changed the title Bug: Bug: click event without change event does not update checkbox value May 2, 2022
@cefn
Copy link
Author

cefn commented May 2, 2022

Thanks, @cexbrayat that's exactly what I needed to know. With the extra mount option passed, the test passes and a change event is issued. The commit that fixes it is cefn/vite-reactive-repro@20c1791

@cefn
Copy link
Author

cefn commented May 2, 2022

Does anyone know of any documentation indicating this change? Seems like a surprising and major issue not to describe if it means core eventing fails for all DOM elements.

The issue is not mentioned in the migration guide at the time of writing (linked from https://github.com/vuejs/test-utils/ ). Having read the note in the migration guide I am reopening since probably this should be documented...

image
https://test-utils.vuejs.org/migration/

Also does test state (DOM state) have to be managed more carefully on the new model, to prevent unexpected interactions and dependencies?

The literal document.body reference is a global shared by all tests, potentially breaking a lot of assertions unless you actively remove your elements in a finally when finished with unmount. Currently our tests never explicitly attach and (hence) never unmount.

@cefn cefn reopened this May 2, 2022
@cexbrayat
Copy link
Member

Sure, if you're willing to, a PR would be most welcome to update the migration guide and mention this edge case 👍

To answer your other question, I think the proper way is to use setValue on a checkbox to update its value, without the need for attachTo.

@cexbrayat cexbrayat changed the title Bug: click event without change event does not update checkbox value docs: click event without change event does not update checkbox value without attachTo: document.body May 2, 2022
@cexbrayat cexbrayat added docs good first issue Good for newcomers and removed bug Something isn't working labels May 2, 2022
@cefn
Copy link
Author

cefn commented May 2, 2022

Thanks @cexbrayat I wouldn't go down the setValue route as we aim to align with the philosophy of https://twitter.com/kentcdodds/status/977018512689455106 ( as expanded on at https://testing-library.com/docs/guiding-principles ).

According to that philosophy, (in my view), manipulating the internal state of the checkbox is too close a coupling - it doesn't actually prove that clicking works. What if someone fumbled preventDefault in click handling somewhere? For purity we should probably be issuing an event that clicks on the input in the document having label 'X' as a user would do, and not even making assumptions about which input reference it is.

I'm not well-informed enough to update the documentation without knowing what the previous behaviour was for mount (without attachTo set), and hence what the consequences are, (presumably some class of handlers isn't invoked at all, somewhere in the layers between Vue, Test-Utils, JSDOM). The feature change isn't documented or discussed anywhere I've seen.

I'm assuming it's too late to reconsider the default for attachTo.

@cexbrayat
Copy link
Member

VTU is a low-level library. So it's expected to have to trigger some updates manually.
If you want to write higher-level tests, something like testing-library/vue is maybe what you're looking for.

I don't think we should change the default of attachTo.

But we can add a mention in the migration guide to help users that stumble on the same use-case as yours. Something pretty simple summarizing our conversation should be enough.

Let's add @afontcu and @lmiller1990 who know VTU v1 better than me, and may have a better insight.

@cefn
Copy link
Author

cefn commented May 3, 2022

If you want to write higher-level tests, something like testing-library/vue is maybe what you're looking for

Agreed. However, the work ahead is migration from Vue2 to Vue3 using existing VTU1 tests ported to VTU2 to prove that the Vue3 migration worked. In the suite we have ~700 uses of .trigger( which aim to exercise user-oriented events. I gather these will all fail until we change every mount call to reflect the VTU1=>VTU2 feature change. Ideally our VTU2 tests would pass with the fewest possible changes compared to the VTU1 tests, hence wishing for alignment with previous behaviour but I accept this is not desired by the test-utils team and we will find a way.

@lmiller1990
Copy link
Member

lmiller1990 commented May 3, 2022

I think testing-library/vue defaults to attachTo: document (or something along these lines).

I think the default here is the same as Test Utils v1; we create an element (but do not actually attach it, unless you specifically say so).

V1: https://github.com/vuejs/vue-test-utils/blob/3cd81d0593f56034b96f368d8ab066a855e0b204/packages/test-utils/src/mount.js#L34-L38.

When making VTU v2 we tried to stick as close to the previous behaviour as possible. It should be the same. The reasoning was exactly what you said - making migration from Vue 2 to Vue 3 (and VTU v1 to v2) as seamless as possible.

It seems like attachTo solves your problem; the initial confusion (around needing attachTo in the first place) is not unique to jsdom iirc; if you make a non-attached checkbox in Chrome, you'll have the same problem.

@cefn
Copy link
Author

cefn commented May 3, 2022

Thanks, @lmiller1990 we can go through adding attachTo. The logic of needing attachTo is clear but I can't explain how these worked in VTU1 and how to document the migration as you're saying there's nothing needed.

It sounds like you're saying VTU1 had the same behaviour and that's not what we observe. Rather, we observe hundreds of tests using VTU1 with invocations like .trigger('click') and which never passed attachTo but seemed to work anyway, hence the original issue.

I am particularly confused because the use of click without dom attachment is right there in the reference documentation https://v1.test-utils.vuejs.org/api/wrapper/trigger.html

There is a comment there about attachTo being required only for the 'focus' event, which suggests the reverse - that mostly mount and trigger() works without it.

@afontcu
Copy link
Member

afontcu commented May 3, 2022

hey!

I think testing-library/vue defaults to attachTo: document (or something along these lines).

Yep, it attaches to document.body if no baseElement is specified: code, docs.

Beyond that, issues with inputs and click events have always been present. In fact, that's why Vue Testing Library provides fireEvent.update, to overcome such limitation (source, check out the comments). I'm not sure this is 100% related to the issue described by the OP, but maybe it's worth looking into it.

we aim to align with the philosophy of https://twitter.com/kentcdodds/status/977018512689455106 ( as expanded on at https://testing-library.com/docs/guiding-principles )

if that's the case (and I strongly agree with those statements!) then you should definitely give Vue Testing Library a go, which acts as a wrapper of VTU and provides methods to test at user level

@lmiller1990
Copy link
Member

Thanks, @lmiller1990 we can go through adding attachTo. The logic of needing attachTo is clear but I can't explain how these worked in VTU1 and how to document the migration as you're saying there's nothing needed.

It sounds like you're saying VTU1 had the same behaviour and that's not what we observe. Rather, we observe hundreds of tests using VTU1 with invocations like .trigger('click') and which never passed attachTo but seemed to work anyway, hence the original issue.

I am particularly confused because the use of click without dom attachment is right there in the reference documentation https://v1.test-utils.vuejs.org/api/wrapper/trigger.html

There is a comment there about attachTo being required only for the 'focus' event, which suggests the reverse - that mostly mount and trigger() works without it.

I should clarify - you don't need attachTo for all trigger events, but specifically for changing a checkbox. It's a weird DOM thing that has nothing to do specifically with test utils - you can't change a checkbox value that is not in the DOM.

@lmiller1990
Copy link
Member

So for the action to close this, I'd say a small note on this page: https://test-utils.vuejs.org/guide/essentials/event-handling.html would suffice!

Amour1688 pushed a commit to vuejs/babel-plugin-jsx that referenced this issue Oct 15, 2022
* chore(deps): update all non-major dependencies

* chore: remove the lockfile and reinstall to update trnasitive deps

The outdated `@types/babel__traverse` package is causing type errors

* fix: fix htmlTags type error

* fix: pin @types/node to 18.8.0 to work around vuejs/language-tools#1985

* fix: pin @vue/test-utils temporarily to avoid snapshot differences

* test: update snapshot

As far as I see, all the snapshot differences are merely the newlines
after `import` statements

* test: add `attachTo: document.body` to make click event take effect

See vuejs/test-utils#1470 (comment)

* fix: fix mjs processing for webpack 4

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Haoqun Jiang <[email protected]>
cexbrayat added a commit to cexbrayat/vue-test-utils-next that referenced this issue Feb 10, 2023
Fixes vuejs#1470

Some events, like clicking on a checkbox to change its `v-model`, will only work if the test uses `attachTo: document.body`.
Otherwise, the `change` event will not be triggered, and the `v-model` value does not change.
cexbrayat added a commit that referenced this issue Feb 10, 2023
Fixes #1470

Some events, like clicking on a checkbox to change its `v-model`, will only work if the test uses `attachTo: document.body`.
Otherwise, the `change` event will not be triggered, and the `v-model` value does not change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants