Skip to content

Using a non deprecated function should not print a deprecation warning #1532

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

Closed
maxarndt opened this issue May 6, 2020 · 29 comments
Closed

Comments

@maxarndt
Copy link

maxarndt commented May 6, 2020

Version

1.0.0

Reproduction link

https://github.com/maxarndt/repro-vue-test-utils-deprecation-warning

Steps to reproduce

  1. Clone repro repo
  2. npm i
  3. npm test

What is expected?

Using a non deprecated function as .destroy() should not print deprecation warnings.

What is actually happening?

Using the above mentioned function prints a deprecation warning.


The implementation of .destroy() needs to be updated to use the successor of isVueInstance.

@TheDutchCoder
Copy link

We can also add isVisible to this list (as it uses isEmpty for which a deprecation warning is thrown).

The error also has a mistake in it: "isVueInstance is deprecated and will removed in the next major version" should be "isVueInstance is deprecated and will be removed in the next major version"

Honestly I'm disappointed by this release, it feels like their own code wasn't tested?

@lmiller1990
Copy link
Member

lmiller1990 commented May 6, 2020

Ah, I forgot we call isVueInstance in destroy. I will fix this one now.

Ideally we would have moved into release candidates to avoid missing things, but after literally years in beta and about 12 months of no-one maintaining the repo, and with Vue 3 in the near future, it felt a bit silly to drag it out any longer.

Please review so I can do a patch: #1533

@lmiller1990
Copy link
Member

lmiller1990 commented May 6, 2020

This is fixed in the latest version (1.0.2). Try installing that one.

Lmk if any others come up. And yes, we should have had better tests around the deprecations. Focus has largely been elsewhere, mainly around Vue 3, but that's no excuse. Let's make sure we keep the standards high.

@TheDutchCoder
Copy link

Thanks I'll give it a whirl in a bit to see if it's resolved.

I appreciate all the hard work though, just found it odd that the internal build has these warnings suppressed on purpose, that seems to sort of defeat their purpose a bit ;)

@lmiller1990
Copy link
Member

Cool. LMK if other things sneaked through.

@TheDutchCoder
Copy link

So far it's not going too well, it looks like some of the deprecations don't have any solutions.

Example:

// This throws 2 warnings: find, and isVisible
expect(wrapper.find(Loader).isVisible()).toBe(true)

// This fails, because findComponent returns an object, and `toBeVisible` (as suggested by vue-test-utils) expects markup
expect(wrapper.findComponent(Loader)).toBeVisible()

@lmiller1990
Copy link
Member

lmiller1990 commented May 7, 2020

Hm, tricky one. I always search for a DOM node and assert against that, so I did not catch this case. The two deprecations make sense individually but not when shown together.

The general idea moving forward to favor finding and asserting against DOM nodes, rather than components (DOM node are more reliable - there are, and likely always be, some edge cases with finding components).

Edit: people smarter than me pointed out expect(wrapper.findComponent(Foo.element)).toBeVisible(). I will update the deprecation to suggest element as well.

@lmiller1990
Copy link
Member

lmiller1990 commented May 7, 2020

#1534 This should improve the message @TheDutchCoder

@TheDutchCoder
Copy link

Ah ok, .element makes sense, as I was confused why there was such an emphasis on finding components over DOM nodes before and now that thought has switched?

I'm okay with either, as long as I can write tests of course :)

Thanks for the heads up, I will upgrade and update my tests.

@p0psicles
Copy link

Maybe this is related. But when I'm running this test:
expect(wrapper.find('#element-id')).not.toBeVisible();
im getting the error: TypeError: expect(...).not.toBeVisible is not a function

I'm trying to apply as stated in the deprecation warning, but i'm not getting any further? Can anyone help?

@afontcu
Copy link
Member

afontcu commented May 15, 2020

Hi @p0psicles! toBeVisible is part of jest-dom, you might want to install that library to use its assertions.

@p0psicles
Copy link

As you might already have noticed I'm not as well versed in front end testing as you guys. The documentation on using jest-dom icw vue-test-utils is really minimal. As it's mostly focused on react. Can anyone redirect me to some valuable docs on this? I was doing fine until the deprecation warnings popped up.

@afontcu
Copy link
Member

afontcu commented May 15, 2020

@p0psicles no problem!

the deprecation messages are just that – deprecation messages. Your tests will keep doing fine for a while, you don't need to worry about them right now!

That being said, jest-dom is framework-agnostic, so there's nothing on it that makes it React-specific. After you install the library, its matchers become available so you can write expect(...).toBeVisible().

Hope this helps 🙌

@hiendv
Copy link
Contributor

hiendv commented May 25, 2020

If you're here looking for a transition for
[vue-test-utils]: isVisible is deprecated and will be removed in the next major version. Consider a custom matcher such as those provided in jest-dom: https://github.com/testing-library/jest-dom#tobevisible. When using with findComponent, access the DOM element with findComponent(Comp).element.
Let me summary above comments and hope it could save you some time

// old assertion
expect(wrapper.find('.selector').isVisible()).toBeTruthy() // .toBe(true)

// new assertion
import '@testing-library/jest-dom' // leave it in the top of the file or in the test setup file
expect(wrapper.find('.selector').element).toBeVisible()

You should have @testing-library/jest-dom installed.

@p0psicles
Copy link

Yeah with the help of you guys I figured this out.
What does trigger my curiousity is why this Method is removed from vue-test-utils. What is the direction of this Utility?

@hiendv
Copy link
Contributor

hiendv commented May 25, 2020

As @lmlife2016 already said (I hardly remember where and when 🤣), the team wanted to focus on the core values of cli-test-utils.

@afontcu
Copy link
Member

afontcu commented May 25, 2020

Hi! Getting visibility right is harder than expected, and jest-dom is already doing a solid job. As @hiendv just said, we decided that focusing on Vue-related stuff was smarter than re-inventing the wheel :)

@hiendv
Copy link
Contributor

hiendv commented May 25, 2020

Right decision IMO 👍 When #1557 lands in the official docs, the transition should be smooth. Keep up the good work, guys.

@lmiller1990
Copy link
Member

Thanks @hiendv, I added your example to the migration docs.

@hiendv
Copy link
Contributor

hiendv commented May 27, 2020

@lmiller1990 Thank you :D
Anyway, I do notice some deprecation warnings mention this

When using with findComponent, access the DOM element with findComponent(Comp).element`.

This is true but find({string} selector) also returns an object so .element is also needed for find.

@lmiller1990
Copy link
Member

Oh, I see. If you have time it'd be great if you can make a PR improving that message. I will do a release soon.

@richardeschloss
Copy link

richardeschloss commented Aug 18, 2020

Hi, can you please not deprecate the function? jest-dom is not test framework agnostic (i.e., you need jest already to use it; ava tests will break). The bigger risk is in breaking tests that currently exist that use the function. Perhaps just leave the warning message in with a user's ability to mute the warning?

Can I suggest just using the exact same code from jest-dom in vue-test-utils? That way people don't need to go install another dependency for this. One of the things I like about vue-test-utils it's just one install and pretty much most of my vue testing needs are met. I don't want the abilities to just disappear. Even if methods are imperfect, I can live with imperfect.

https://github.com/testing-library/jest-dom/blob/e4d61c2ef16018197c316135f57f905bf5b2ca2a/src/to-be-visible.js#L26

@lmiller1990
Copy link
Member

I was thinking about this a bit recently too. I think some of our deprecations make perfect sense, but this one a little bit too aggressive. The general idea was "don't reinvent the wheel, leave it up to your test runner" where possible.

If you really enjoy this method, you could just add it back in yourself; import { Wrapper } fro '@vue/test-utils' and modify the prototype. We actually have a plugin system in V2 of this library to make it easier to add your own methods.

That said, we do have exists for v-if, having visible for v-show might not be the worst idea. I really like the parallel of v-if -> exists, v-show -> visible.

I think the other deprecations still make sense, though. I would like to get another opinion from @afontcu or @dobromir-hristov, and finally if anyone else is reading this and feels strong about porting visible from jest-dom to VTU, please thumbs up this.

@richardeschloss
Copy link

I agree. I appreciate your response and reasoning. Thanks for reconsidering.

@xanf
Copy link
Contributor

xanf commented Aug 29, 2020

@lmiller1990 while we are here speaking about deprecations - could we possibly undeprecate .is() for checking against Component class (not the tag). It is extremely useful when checking that dynamic component was resolved to correct class, and workaround is very ugly

@lmiller1990
Copy link
Member

That is a pretty interesting use case - I didn't think about this one at all.

Proposal:

  • port this and bring back visible. This is not perfect but we should have something good for v-show testing. Custom matches are good for fancy things, but v-show is not exactly an unusual use case.
  • I don't really have a strong opinion on is. Personally I'm not a fan of specific component assertions like that, but implementing is in user-land is extremely difficult because it uses matches (see here), so this seems like a strong enough reason to bring it back.

If someone wants to make a PR with these two (or even just one) I think that'd be fine. I think we (the VTU team) had the right idea with some deprecations - I think some of them make sense - but we definitely should have put these through a more rigorous RFC process. Sorry about the inconvenience that we could have avoided with a bit of planning.

What do you all think?

@xanf
Copy link
Contributor

xanf commented Aug 30, 2020

@lmiller1990 I know the VTU team are not big fan of shallowMount :) neither part of GitLab is :). But what we agree (in GitLab) that shallowMount + is is important in "documenting" component intentions like "it should render component Foo" under certain conditions

I'll open PRs for this for review :)

@lmiller1990
Copy link
Member

lmiller1990 commented Aug 30, 2020

Sounds good. I think we can all agree that the ultimate goal of this library and testing in general is to give you confidence in your application. If is enables you to be more confident in your application, we should include it 👍

@xanf
Copy link
Contributor

xanf commented Oct 5, 2020

@lmiller1990 I believe we could close this one as relevant MRs were merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants