ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.93k stars 13.51k forks source link

bug: vue jest utils, shallowMount does not work when property values are symbols #24181

Closed boboldehampsink closed 2 years ago

boboldehampsink commented 2 years ago

Prerequisites

Ionic Framework Version

Current Behavior

After bumping Ionic Vue from v5 to v6 RC, all my tests that are shallowMounted fail with the following error:

Screen Shot 2021-11-08 at 09 41 06

Expected Behavior

This didn't happen with v5, and I expect it to work in v6 as well

Steps to Reproduce

Run an Ionic 6 app test with shallowMount

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.17.1

Utility:

cordova-res : not installed globally native-run : not installed globally

System:

NodeJS : v17.0.1 npm : 8.1.0 OS : macOS Monterey

Additional Information

Also see https://github.com/vuejs/vue-test-utils/issues/1833

ionitron-bot[bot] commented 2 years ago

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

liamdebeasi commented 2 years ago

Hey there,

Could you send over a sample app so I can verify if this is the same issue as https://github.com/vuejs/vue-test-utils/issues/1833?

boboldehampsink commented 2 years ago

@liamdebeasi I have created a sample app that reproduces this when running yarn run test:unit. I found that this happens when there is an ion-input present. Here you go: https://github.com/boboldehampsink/test

liamdebeasi commented 2 years ago

Apologies for the delay. Can you double check your repo? I get the following error when I try to run npm run test:unit:

   TypeError: importer_1.importer.babelJest(...).createTransformer is not a function

      at ConfigSet.get (node_modules/ts-jest/dist/config/config-set.js:395:86)
      at ConfigSet.babelJestTransformer (node_modules/ts-jest/dist/util/memoize.js:43:24)
      at TsJestTransformer.process (node_modules/ts-jest/dist/ts-jest-transformer.js:86:57)
boboldehampsink commented 2 years ago

hi @liamdebeasi, just clean checked out my test repo again, ran npm install, then npm run test:unit on Node 17, and I don't see the error you see but the error I found before.

liamdebeasi commented 2 years ago

I did a fresh install on the repo and ran npm run test:unit with Node 17, but I still get the same error as above. I do get a few incompatibility warnings:

ts-jest[versions] (WARN) Version 4.4.4 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=2.7.0 <4.0.0). Please do not report issues in ts-jest if you are using unsupported versions.
ts-jest[versions] (WARN) Version 27.3.1 of babel-jest installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=24.0.0 <25.0.0). Please do not report issues in ts-jest if you are using unsupported versions.

Do you have some steps to reproduce the issue on my end? I can try creating a sample app of my own.

boboldehampsink commented 2 years ago

hi @liamdebeasi its specifically this commit: https://github.com/boboldehampsink/test/commit/9240dcd0f420dd11420ae9281db6a5121eb897f4

Try a shallowMount with an ion-input in place

liamdebeasi commented 2 years ago

Thanks, I can reproduce this behavior. The issue does indeed seem to be related to https://github.com/vuejs/vue-test-utils/issues/1833.

The reason this is occurring is in Vue 3.1, all property keys are added to the properties object, even if certain properties are not being used. This resulted in properties on some of our components getting set to undefined even when the user was not actually using them. The recommended workaround is to set the empty values to be a Symbol instead of undefined (since undefined is a valid value for certain properties).

As a workaround, using mount instead of shallowMount should fix the issue. I can follow up on that Jest Utils threads to provide more context. Thanks!

boboldehampsink commented 2 years ago

Hi @liamdebeasi, great. Unfortunately, this occurs for us when updating from Ionic 5 to Ionic 6. It works fine on Ionic 5 with Vue 3.1. That's why I'm reporting it here.

Also, rewriting our entire testsuite from shallowMount to mount is a lot of work and not a drop-in replacement.

And at last, the issue https://github.com/vuejs/vue-test-utils/issues/1833 is about vue-test-utils 1x, while I'm experiencing this in 2x. I told them about this issue as well: https://github.com/vuejs/vue-test-utils-next/issues/985#issuecomment-944057738. Maybe a new issue should be openend?

liamdebeasi commented 2 years ago

I hear you, rewriting from shallowMount to mount is definitely not ideal. Unfortunately, we cannot remove our Symbol usage as it would cause other breaks in behavior when using Vue 3.1.

I'll open an issue in the vue-test-utils-next repo.

boboldehampsink commented 2 years ago

Thanks Liam :-) appreciated. Hope this gets fixed before Ionic 6 reaches stable!

liamdebeasi commented 2 years ago

Filed an issue: https://github.com/vuejs/vue-test-utils-next/issues/1076

lmiller1990 commented 2 years ago

I don't think this is as simple as a bug in Test Utils; I'm unable to reproduce with a minimal reproduction in the code base: https://github.com/vuejs/vue-test-utils-next/compare/issue-1076?expand=1

I am seeing the issue in the provided reproduction, though, There's a lot more dependencies there, it's not super clear exactly what the problem is. The actual stack trace appears to be from somewhere in jsdom - has anyone tried running the reproduction in a browser? Seems like this is specific to jsdom and shallowMount together (weird).

See my comment: https://github.com/vuejs/vue-test-utils-next/issues/1076#issuecomment-971507588

lmiller1990 commented 2 years ago

Debugging... the problem goes away if we don't pass https://github.com/vuejs/vue-test-utils-next/blob/0a8e02c34a9e600278ad88293b09939366069978/src/stubs.ts#L254

lmiller1990 commented 2 years ago

Edit - I reproduced it. I'll look into this.

Basically the problem is we do something like this (only for stubs):

<some-stub sym="Symbol()" />

Which is not valid in jsdom, you cannot serialize a symbol (according to their implementation). I don't really see any good solution here... the problem is basically that the attribute serializer in jsdom chooses to error here. Any ideas?

We are not the first ones to run into this: https://github.com/jsdom/webidl-conversions/issues/14. We might need to add some kind of escape hatch or hack into Test Utils for this, I'm not entirely sure how to proceed.

What we could do is have a "dumb stubs" config option, so instead of attempting to spread props over a stub, like I described above, you'd just get no props. It would solve this problem, but you wouldn't get props on your stubs (which I think is fine. Something like:

import { config } from '@vue/test-utils'
// global
config.global.noPropsOnStubs = true

// per test
shallowMount(Comp, {
  global: {
    noPropsOnStubs: true
  }
})

Still not great, but there's basically no chance jsdom will change this, and I don't see another good option really. Please give me your input (either here or in the Test Utils issue). I'll rely on everyone to help move this fix forward.

boboldehampsink commented 2 years ago

I wouldn't mind if a stub wouldn't have props

lmiller1990 commented 2 years ago

Maybe we should try using https://www.npmjs.com/package/happy-dom and see what happens? Be interesting to how they handle the Symbol attribute case.

I'm not 100% sold on the above proposal, just throwing ideas out there... this does feel like quite a big feature to consider for this one weird combination/edge case. Happy to get more feedback/opinions.

boboldehampsink commented 2 years ago

Sounds good but isn't a new dom parser a far bigger feature than simply introducing a new optional config option

boboldehampsink commented 2 years ago

Perhaps vue test utils can convert a symbol to a string before it goes to jsdom

liamdebeasi commented 2 years ago

I am not very familiar with the inner workings of Vue Test Utils, but is it possible to stub Symbol or replace it with a stubbed value? This would let the tests pass without having to introduce a new feature. Somewhat similar to the idea proposed in https://github.com/ionic-team/ionic-framework/issues/24181#issuecomment-971606690.

lmiller1990 commented 2 years ago

I don't think we can stub out a JS primitive constructor. It would be like replacing String or Array.

I see this doesn't work in a real browser either - if you open your console and do document.body.setAttribute('sym', Symbol()) you get a similar error. I think for this reason we probably do want to remove any symbols that would be passed as attributes to stubs... I'm not sure how difficult this is to do, I can give it a try.

If anyone else would like to work on this in the meantime, I think you need to write some code here. There's a minimal reproduction here.

What you'd need to do is something like this on this line

const render = (ctx: ComponentPublicInstance) => {
  // take $ctx.props and filter out any keys that have a symbol
  // consider you can also have a `default` value which is a function that returns a symbol
  // const props = {} 
  // for (const [k, v] of Object.entries(ctx.$props)) {
  //   # if symbol, continue
  //   # else props[key] = v
  // }
  return h(tag, ctx.$props, renderStubDefaultSlot ? ctx.$slots : undefined)
}

Happy to work with someone if anyone would like to give this a try - I'm trying to grow the contributors to Test Utils to make it better for everyone :)

lmiller1990 commented 2 years ago

Edit, I just implemented it, much faster, please give me some 👀 https://github.com/vuejs/vue-test-utils-next/pull/1086

Be really good if someone could try this against their repos with problems and see if it works. Easiest way to test would be clone and checkout my branch, yarn build, and copy paste the dist directory in your your node_modules/@vue/test-utils/dist.

liamdebeasi commented 2 years ago

Thanks! I tested this on the repo found on https://github.com/vuejs/vue-test-utils-next/issues/1076 as well as the test app we use to verify Ionic Vue changes at https://github.com/ionic-team/ionic-framework/tree/main/packages/vue/test-app. Both test suites passed using the changes in https://github.com/vuejs/vue-test-utils-next/pull/1086.

lmiller1990 commented 2 years ago

Cool, I'll wait until the weekend to see if any of the other contributors can review/give some feedback, if not I'll merge/release.

We could port this back to VTU v1 if needed but not sure how valuable this would be.

boboldehampsink commented 2 years ago

@lmiller1990 great! By the way, there is also an open issue for this in VTU v1 that would be fixed by backporting this: https://github.com/vuejs/vue-test-utils/issues/1833

lmiller1990 commented 2 years ago

We actually found a way to fix this in Vue core if anyone is curious: https://github.com/vuejs/vue-next/pull/4964

boboldehampsink commented 2 years ago

I think this can be closed in favor of https://github.com/vuejs/vue-test-utils/issues/1833

lmiller1990 commented 2 years ago

Fixed for VTU v2, we will release this week.

I don't know about VTU v1. I posted in the above issue: https://github.com/vuejs/vue-test-utils/issues/1833

ionitron-bot[bot] commented 2 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.