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

test: Add tests for shallowMount issue with dynamic components #1312

Conversation

phobetron
Copy link
Contributor

@phobetron phobetron commented Feb 14, 2022

This PR adds a minimal set of tests to describe issue #1277, which regards how dynamic components, when shallow mounted, are stubbed with the name of a computed property that returns a component's name instead of the component itself.

The test component outlines the most extreme error case: a computed property does not need to be referenced in order to trigger the issue, it merely needs to both exist and return the component that is directly referenced.

This PR now also implements a possible simple fix for the issue.

cc: @cexbrayat

@netlify
Copy link

netlify bot commented Feb 14, 2022

✔️ Deploy Preview for vue-test-utils-docs ready!

🔨 Explore the source changes: eeaaf1e

🔍 Inspect the deploy log: https://app.netlify.com/sites/vue-test-utils-docs/deploys/620cd8afa772250007adff95

😎 Browse the preview: https://deploy-preview-1312--vue-test-utils-docs.netlify.app

tests/shallowMount.spec.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
<template>
<component :is="Hello" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't that use computedProperty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cexbrayat This is what I meant when I added the note. It does not matter whether you use the computed property, its name is still what shallowMount will give to component. That's how weird this bug is.

Copy link
Contributor Author

@phobetron phobetron Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a third test, one that will pass, to better outline the issue.

@phobetron phobetron force-pushed the test/1277_adding-failing-tests-for-dynamic-component-shallow-mount-issue branch from 66604e9 to 254bd9e Compare February 14, 2022 14:55
@phobetron phobetron force-pushed the test/1277_adding-failing-tests-for-dynamic-component-shallow-mount-issue branch from 254bd9e to 89fe6a7 Compare February 14, 2022 15:00
@cexbrayat
Copy link
Member

Hmm strange indeed, thanks for the repo 👍

If you feel adventurous, you can try to inspect what's going on when the tests run, and you may be able to fix the issue: we would be very happy to accept the PR!

@phobetron
Copy link
Contributor Author

@cexbrayat The issue was exactly where @freakzlike had suggested. My fix is to ensure getComponentNameInSetup doesn't get tripped up by any getter functions that may return a component object. All the tests now pass. Please let me know what you think.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine! @cexbrayat ?

@lmiller1990
Copy link
Member

@phobetron can you fix the linting errors?

@phobetron phobetron force-pushed the test/1277_adding-failing-tests-for-dynamic-component-shallow-mount-issue branch from 416dd8d to 710d68f Compare February 16, 2022 08:31
@phobetron
Copy link
Contributor Author

@lmiller1990 Sorry about that! I think my local has a different default line length. Should be fixed now.

Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks 👍

@cexbrayat
Copy link
Member

@phobetron : the CI is failing because computedProperty is never used. Maybe you could now use it in the template (I know the original issue is there even if not, but that would make more sense?)

@phobetron
Copy link
Contributor Author

@cexbrayat That makes sense. I've reduced the test code to the minimum necessary to cover the error case. Should pass everything now 😅

@phobetron phobetron force-pushed the test/1277_adding-failing-tests-for-dynamic-component-shallow-mount-issue branch from 9e5563d to eeaaf1e Compare February 16, 2022 10:57
@phobetron
Copy link
Contributor Author

@cexbrayat Sorry, decided to make another minor style adjustment.

@cexbrayat
Copy link
Member

Looks great, thank you for taking the time to investigate and fix this, we really appreciate it!

@cexbrayat cexbrayat merged commit c3046b6 into vuejs:main Feb 16, 2022
@phobetron phobetron deleted the test/1277_adding-failing-tests-for-dynamic-component-shallow-mount-issue branch February 16, 2022 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants