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

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

Closed
4 of 6 tasks
boboldehampsink opened this issue Nov 8, 2021 · 29 comments
Closed
4 of 6 tasks
Labels
bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) type: bug a confirmed bug report

Comments

@boboldehampsink
Copy link

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

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 vuejs/vue-test-utils#1833

@ionitron-bot ionitron-bot bot added the triage label Nov 8, 2021
@liamdebeasi liamdebeasi added the ionitron: needs reproduction a code reproduction is needed from the issue author label Nov 8, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Nov 8, 2021

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.

@ionitron-bot ionitron-bot bot removed the triage label Nov 8, 2021
@liamdebeasi
Copy link
Contributor

Hey there,

Could you send over a sample app so I can verify if this is the same issue as vuejs/vue-test-utils#1833?

@boboldehampsink
Copy link
Author

@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 liamdebeasi added triage and removed ionitron: needs reproduction a code reproduction is needed from the issue author labels Nov 15, 2021
@liamdebeasi
Copy link
Contributor

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)

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Nov 15, 2021
@ionitron-bot ionitron-bot bot removed the triage label Nov 15, 2021
@boboldehampsink
Copy link
Author

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.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Nov 16, 2021
@liamdebeasi
Copy link
Contributor

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.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Nov 16, 2021
@ionitron-bot ionitron-bot bot removed the triage label Nov 16, 2021
@boboldehampsink
Copy link
Author

hi @liamdebeasi its specifically this commit: boboldehampsink/test@9240dcd

Try a shallowMount with an ion-input in place

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Nov 16, 2021
@liamdebeasi
Copy link
Contributor

Thanks, I can reproduce this behavior. The issue does indeed seem to be related to vuejs/vue-test-utils#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!

@liamdebeasi liamdebeasi changed the title bug: upgrading from Ionic Vue v5 to v6 yields test errors bug: vue jest utils, shallowMount does not work when property values are symbols Nov 16, 2021
@liamdebeasi liamdebeasi added bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) type: bug a confirmed bug report labels Nov 16, 2021
@boboldehampsink
Copy link
Author

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 vuejs/vue-test-utils#1833 is about vue-test-utils 1x, while I'm experiencing this in 2x. I told them about this issue as well: vuejs/test-utils#985 (comment). Maybe a new issue should be openend?

@liamdebeasi
Copy link
Contributor

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
Copy link
Author

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

@liamdebeasi
Copy link
Contributor

Filed an issue: vuejs/test-utils#1076

@lmiller1990
Copy link

lmiller1990 commented Nov 17, 2021

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: vuejs/test-utils#1076 (comment)

@lmiller1990
Copy link

@lmiller1990
Copy link

lmiller1990 commented Nov 17, 2021

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: jsdom/webidl-conversions#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
Copy link
Author

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

@lmiller1990
Copy link

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
Copy link
Author

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

@boboldehampsink
Copy link
Author

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

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Nov 17, 2021

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 #24181 (comment).

@lmiller1990
Copy link

lmiller1990 commented Nov 17, 2021

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
Copy link

Edit, I just implemented it, much faster, please give me some 👀 vuejs/test-utils#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
Copy link
Contributor

Thanks! I tested this on the repo found on vuejs/test-utils#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 vuejs/test-utils#1086.

@lmiller1990
Copy link

lmiller1990 commented Nov 18, 2021

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
Copy link
Author

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

@lmiller1990
Copy link

We actually found a way to fix this in Vue core if anyone is curious: vuejs/core#4964

@boboldehampsink
Copy link
Author

I think this can be closed in favor of vuejs/vue-test-utils#1833

@lmiller1990
Copy link

Fixed for VTU v2, we will release this week.

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

@ionitron-bot
Copy link

ionitron-bot bot commented Jan 13, 2022

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.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

3 participants