Navigation Menu

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

Explain what the replacement for deprecations are #1541

Closed
Wmaarts opened this issue May 11, 2020 · 34 comments · Fixed by #1548
Closed

Explain what the replacement for deprecations are #1541

Wmaarts opened this issue May 11, 2020 · 34 comments · Fixed by #1548

Comments

@Wmaarts
Copy link

Wmaarts commented May 11, 2020

What problem does this feature solve?

The documentation for for example setMethods is unclear. It simply states: "setMethods is deprecated and will be removed in future releases.". Same goes for isVueInstance.

This does not explain what the replacement (if any) should be, or why this was deprecated.

These types of warnings also happens when running the tests:
"[vue-test-utils]: overwriting methods via the methods property is deprecated and will removed in the next major version"

Here it's also unclear what the fix should be or why this was deprecated.

What does the proposed API look like?

Not applicable.

@afontcu
Copy link
Member

afontcu commented May 11, 2020

Hi! Thanks for filling this in.

There's no clear path to replace setMethods or methods property, because it really depends on your previous usage. It will be removed because it might lead to flaky tests – your tests end up relying on implementation details. The suggestion is to rethink those tests, I believe! (more on that on the VTU RFC).

Still, if you have some ideas of guidance/suggestions to help users, we'll be more than happy to review a PR! 😃

@jimmerydad
Copy link

jimmerydad commented May 11, 2020

For isVueInstance , if the note included at https://github.com/dobromir-hristov/rfcs/blob/vtu-2-api/active-rfcs/0000-vtu-api.md#isvueinstance

find always returns an DOMWrapper and findComponent returns a VueWrapper. Both return ErrorWrapper if failed.

were added it would have saved me a little time finding this as well (ie to use findComponent )

@nicojs
Copy link

nicojs commented May 12, 2020

The suggestion is to rethink those tests, I believe! (more on that on the VTU RFC).

Would be awesome if this is reflected in the deprecation warning. Maybe link to that page. Right now we get 100's of deprecation warnings without a clear path on how to fix them.

@afontcu
Copy link
Member

afontcu commented May 12, 2020

Makes sense, thank you all for the suggestion. Anyone fancy to open up a PR with improved deprecation messages? 😇 otherwise I will.

cheers!

@mydnic
Copy link

mydnic commented May 12, 2020

I have the similar problem with isVueInstance
Exemple, I have a very small and basic test to assert that a component is correctly mounted without error.
For that I used expect(wrapper.isVueInstance()).toBeTruthy()

Now that I have the deprecation notice, I do not know how I can replace this basic test.

Any idea ?

Thanks a lot!

@mydnic
Copy link

mydnic commented May 12, 2020

Nevermind, looks like expect(wrapper.exists()).toBeTruthy() does the trick for me

@janosrusiczki
Copy link
Contributor

I'm just plus one-ing the need to explain what to use to replace isVueInstance(). Else you'll probably have this issue pop up many times as many tutorials use this assertion as a basic example and it tends to stick in people's tests.

@lmiller1990
Copy link
Member

lmiller1990 commented May 14, 2020

For isVueInstance, tests. I would generally recommend just deleting those tests - it's just testing an implementation detail, which is not usually a good thing.

You could do expect(wrapper.find(...).vm).toBeTruthy() if you feel very strongly about keeping those tests. isVueInstance just called !!this.vm internally, anyway, as seen here.

setMethods is tricky, very much case by case! @afontcu is putting some docs together to help with this.

If anyone has a complicated test with setMethods that cannot wait, feel free to post it here or message me on Twitter or Discord (Vue-land) or email (in my GH profile) and I can help you.

If the deprecations messages are super annoying, you can temporarily silence them:

import { config } from `@vue/test-utils`

config.showDeprecationWarnings = false

@afontcu
Copy link
Member

afontcu commented May 15, 2020

Hi folks! I'm pinging everyone on this issue because #1548 is up and we'd like to receive your feedback on the updated deprecation messages 😇

thanks!

lmiller1990 pushed a commit that referenced this issue May 17, 2020
* docs: improve title on deprecation warnings

* docs: add suggestion for isVueInstance

* refactor: improve migration path for setMethods and options.methods

* docs: fix link format

* docs: add more hints to replace setMethods and options.methods

* docs: reword messages

* docs: remove implicit statement
@begueradj
Copy link

begueradj commented Jun 16, 2020

@afontcu
You said: "your tests end up relying on implementation details." but personally I do not see how using isVueInstance relies on implementation details.

@begueradj
Copy link

begueradj commented Jun 16, 2020

@lmiller1990
You said: it's just testing an implementation detail, which is not usually a good thing.

For isVueInstance, tests. I would generally recommend just deleting those tests - it's just testing an implementation detail, which is not usually a good thing.

You could do expect(wrapper.find(...).vm).toBeTruthy() if you feel very strongly about keeping those tests. isVueInstance just called !!this.vm internally, anyway, as seen here.

setMethods is tricky, very much case by case! @afontcu is putting some docs together to help with this.

If anyone has a complicated test with setMethods that cannot wait, feel free to post it here or message me on Twitter or Discord (Vue-land) or email (in my GH profile) and I can help you.

If the deprecations messages are super annoying, you can temporarily silence them:

import { config } from `@vue/test-utils`

config.showDeprecationWarnings = false

How come it is testing an implementation detail ? I do not see that, if you can explain, thank you

@lmiller1990
Copy link
Member

lmiller1990 commented Jun 16, 2020

Ideally you should test the input and output of the component (or any function in general, when writing tests).

For example, for a Vue component, the inputs might be a user filling out a form and submitting it, and the output might be the component renders a "Success" message. We don't really care how the component works, but that the behavior is correct.

Asserting that something is a Vue instance is not very useful and provides little value; it's much better practice to test the behavior.

@begueradj
Copy link

I agree with the behavioral testing, you explained that very well and many times on your website. However, IMHO, I like to use isVueInstance() because it gives me a feedback whether the component is mounted properly or not (the simplest example is when we forget to close a "tag"/component)

@afontcu
Copy link
Member

afontcu commented Jun 16, 2020

I agree with the behavioral testing, you explained that very well and many times on your website. However, IMHO, I like to use isVueInstance() because it gives me a feedback whether the component is mounted properly or not (the simplest example is when we forget to close a "tag"/component)

Given that scenario, wouldn't any other test against that Component fail, too?

xbl added a commit to xbl/vue-tdd-demo that referenced this issue Jun 17, 2020
@daniel-butler
Copy link

I agree with the behavioral testing, you explained that very well and many times on your website. However, IMHO, I like to use isVueInstance() because it gives me a feedback whether the component is mounted properly or not (the simplest example is when we forget to close a "tag"/component)

Given that scenario, wouldn't any other test against that Component fail, too?

Its a smoke test. If that test is failing you know something major with the component off. When I have a whole slew of errors I've found it is easier to look for the most simple test that failed, fix that then continue up in complexity. Basically the single responsibility principle for tests.

@afontcu
Copy link
Member

afontcu commented Jun 24, 2020

In that (legit!) case, you might want to use expect((...).vm).toBeTruthy() (source) so the test will fail as soon as the component instance is not there

@kamalhm
Copy link

kamalhm commented Jul 15, 2020

Sorry I dont get what should we put in the ... part

In that (legit!) case, you might want to use expect((...).vm).toBeTruthy() (source) so the test will fail as soon as the component instance is not there

@afontcu
Copy link
Member

afontcu commented Jul 15, 2020

whatever you were doing in the first place. for example, from:

expect(wrapper.findComponent(myComponent)).isVueInstance()

to

expect(wrapper.findComponent(myComponent).vm).toBeTruthy()

@kamalhm
Copy link

kamalhm commented Jul 16, 2020

@afontcu
I usually do something like this to test for Vue instance

import Component from '@/components/ComponentName'

beforeEach(() => {
    wrapper = shallowMount(Component, {
      i18n,
      store,
      localVue,
      router
    })
    vm = wrapper.vm
  })

  test('is a Vue instance', () => {
    expect(wrapper.isVueInstance()).toBeTruthy()
  })

what should I do if I want to do the same smoke test?
wrapper.findComponent(Component).vm ?

@lmiller1990
Copy link
Member

lmiller1990 commented Jul 16, 2020

expect(wrapper.vm).toBeTruthy(). Although why you want to write this is beyond me - it's not even really smoke testing anything, you should at least make an assertion against wrapper.html().

@kamalhm
Copy link

kamalhm commented Jul 16, 2020

noted, i probably wont use this in my personal project but it's work project and we have lots of isVueInstance, so I'm just trying to

  1. remove the warn message
  2. try to find a better way to do it

thanks for the suggestion @lmiller1990 👍

nassimerrahoui added a commit to nassimerrahoui/jhipster-control-center that referenced this issue Jul 30, 2020
methods property is deprecated and will be removed (see : vuejs/vue-test-utils#1541)
+
some refactoring because of warnings in unit tests
@ogomaemmanuel
Copy link

ogomaemmanuel commented Sep 22, 2020

@afontcu there is a place i think this is usable, especially in the created hook, to test if a method is called on component create hook . here is a sample of what I have. Since getBanks is an external call, i need to replace it's implementation

import SendToBank from "../SendToBank";
import {mount} from "@vue/test-utils"

 describe("Send Money To Back Form", () => {
  it("should call getBanks methods  when component is created", () => {
    let getBanks = jest.fn();
    let wrapper = mount(SendToBank, {
        methods: {
            getBanks
        }
    });
    expect(getBanks).toBeCalledTimes(1);
   })
})

@lmiller1990
Copy link
Member

@ogomaemmanuel would you not just mock out the API lib (eg axios) and assert that was called?

@keremistan
Copy link

@afontcu there is a place i think this is usable, especially in the created hook, to test if a method is called on component create hook . here is a sample of what I have. Since getBanks is an external call, i need to replace it's implementation

import SendToBank from "../SendToBank";
import {mount} from "@vue/test-utils"

 describe("Send Money To Back Form", () => {
  it("should call getBanks methods  when component is created", () => {
    let getBanks = jest.fn();
    let wrapper = mount(SendToBank, {
        methods: {
            getBanks
        }
    });
    expect(getBankPays).toBeCalledTimes(1);
   })
})

That throws a deprecated warning though. How to do the same thing without getting a warning?

@ogomaemmanuel
Copy link

@lmiller1990 that one will work for external api's, but sometime you just want some functions to be called when a the component is created, I have been in situations where I either forget to put a function in the created hook or by accident the functions get removed. To ensure these functions aren't interfered with, I need to pass a mock that will be called during component creation

@lmiller1990
Copy link
Member

lmiller1990 commented Sep 24, 2020

I'm sorry, but methods mounting option will not be coming back at this point. If you can post a specific example of when you think you absolutely need it, happy to offer some alternatives - there is likely a better way to accomplish the same thing.

@keremistan
Copy link

I've solved it by inserting functions after the mounting completed.

const wrapper = shallowMount(Component)
wrapper.vm.methodToBeMocked = jest.fn();

@ogomaemmanuel
Copy link

@lmiller1990 I have gotten how to go about it, just mocking the function before calling mount

@kaiying
Copy link

kaiying commented Dec 11, 2020

@keremistan thanks for your share!
I have a new question, this way can not solved it.
I want test this

const created = function () {
  this.apiGetProducts();
};

old test code:

    shallowMount(component, {
      localVue,
      methods: {
        apiGetProducts,
      },
    });

    /** assert */
    expect(apiGetProducts).toBeCalledTimes(1);

Warning was throw because wrapper.vm timing is late for created.

How can I solve it?

@lmiller1990
Copy link
Member

What does apiGetProducts do? Can you just mock the API call (axios, etc)?

The idea is to discourage these kind of tests - your tests is not very useful, if you were to delete the this.apiGetProducts method your test would still pass.

@kaiying
Copy link

kaiying commented Dec 16, 2020

What does apiGetProducts do? Can you just mock the API call (axios, etc)?

The idea is to discourage these kind of tests - your tests is not very useful, if you were to delete the this.apiGetProducts method your test would still pass.

not completely API call.

const created = function () {
  this.apiGetProducts();
};

const methods = {
  apiGetProducts() {
    getProduct().then(res => {
       ... do Something
    })
    .... more then
  },
}

@lmiller1990
Copy link
Member

What is getProduct? Some API call? Is that imported from somewhere? If so, can you just mock that module (eg jest.mock('../getProduct')?

@kaiying
Copy link

kaiying commented Dec 27, 2020

What is getProduct? Some API call? Is that imported from somewhere? If so, can you just mock that module (eg jest.mock('../getProduct')?

Thanks for you answer 👍
I already mock getProduct.
I wanna test the flow when createdt was called.

I has solved this question.
This is my solution.

  test('[Main] `created`  execute `apiGetProducts`、`apiGetWishListStatus` methods. ', () => {
    /** arrange */
    const apiGetProducts = jest.fn();
    const apiGetWishListStatus = jest.fn();

    // mock methods
    localMixin.methods = { ...mixin.methods, apiGetProducts, apiGetWishListStatus };

    /** act */
    shallowMount(component, {
      localVue,
      ... abort
    });

    /** assert */
    expect(apiGetProducts).toBeCalledTimes(1);
    expect(apiGetWishListStatus).toBeCalledTimes(1);
  });

@YBFJ
Copy link

YBFJ commented Feb 23, 2021

@afontcu there is a place i think this is usable, especially in the created hook, to test if a method is called on component create hook . here is a sample of what I have. Since getBanks is an external call, i need to replace it's implementation

import SendToBank from "../SendToBank";
import {mount} from "@vue/test-utils"

 describe("Send Money To Back Form", () => {
  it("should call getBanks methods  when component is created", () => {
    let getBanks = jest.fn();
    let wrapper = mount(SendToBank, {
        methods: {
            getBanks
        }
    });
    expect(getBanks).toBeCalledTimes(1);
   })
})

I solved my problem with this method

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 a pull request may close this issue.