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

v1.1.4 regression #1820

Closed
Ky6uk opened this issue Apr 12, 2021 · 43 comments · Fixed by #1835 or #1857
Closed

v1.1.4 regression #1820

Ky6uk opened this issue Apr 12, 2021 · 43 comments · Fixed by #1835 or #1857

Comments

@Ky6uk
Copy link

Ky6uk commented Apr 12, 2021

Subject of the issue

After upgrading from v1.1.3 to v1.1.4 some tests have started to fail.

Steps to reproduce

const wrapper = mount(MyComponent, {
  propsData: {
    items: [
      { name: 'woof' },
      { name: 'bork' }
    ]
  }
});

const items = wrapper.findAllComponents({ name: 'MyComponentItem' });
const firstItem = wrapper.at(0);
//                           ^--- [vue-test-utils]: no item exists at 0

// ...

Expected behaviour

Tests should pass as it was before the upgrade.

Actual behaviour

Tests failed.

Possible Solution

Downgrade to v1.1.3.

@Bastczuak
Copy link

Bastczuak commented Apr 12, 2021

Here's another example for a failing test after upgrading to 1.1.4

import SupportFaq from '@/views/SupportFaq'
import { routes } from '@/router'

jest.mock('@/views/SupportFaq.vue', () => ({
  name: 'SupportFaq',
  render: h => h('div'),
}))

it('works in 1.1.3 fails in 1.1.4', async () => {
  const router = new VueRouter({ routes })
  const localVue = createLocalVue()
  localVue.use(VueRouter)
  const wrapper = mount(
    {
      template: '<router-view/>',
    },
    {
      localVue,
      router,
    },
  )

  await router.push('/support/faq')

  expect(wrapper.findComponent(SupportFaq).exists()).toBe(true)
})

@lmiller1990
Copy link
Member

Interesting, what on earth would have caused this I wonder.

@lmiller1990
Copy link
Member

Actually, no I don't wonder, I bet it is this: #1817

I cannot look few a few days, if someone wants to try reverting that commit and see if that's the problem? This seems like it's pretty important to fix and patch asap.

@Bastczuak
Copy link

Bastczuak commented Apr 21, 2021

@lmiller1990 reverting these two changes in 1.1.4 fixes my problem with the posted example above. The change in line 59-65 in matches.js breaks my test.

@lmiller1990
Copy link
Member

If we just revert it, we will break the issue it was trying to fix. What I think we should do is

  1. add an example that fails dud to the regression to the specs in this code base
  2. put back the code that makes it pass
  3. make sure this example case continues to pass

Are you interested in making this change?

@Bastczuak
Copy link

Bastczuak commented Apr 23, 2021

@lmiller1990 I'll give it a try.

@Bastczuak
Copy link

@lmiller1990 I can't reproduce the error. I wrote a test #1832 like @Ky6uk posted. Unfortunately the test succeeds. I used his example because mine is more complex. Maybe I miss something.

@BeeMargarida
Copy link
Contributor

I confirm, could not reproduce a failing test with both the examples provided in this issue. I'm not sure if all information was provided in them for us to be able to reproduce

@Bastczuak
Copy link

Bastczuak commented Apr 23, 2021

Ok I was able to create a failing test in #1832 depending on my posted example! I added some comments because I encountered some problems. Please have a look @BeeMargarida. I still have no clue what causes it.

@BeeMargarida
Copy link
Contributor

BeeMargarida commented Apr 23, 2021

@Bastczuak I think this is another problem not related to the changes made in #1817, even with the regression the test continues to fail (only with shallowMount)...hum. If I'm understanding this correctly 😅

@Bastczuak
Copy link

@BeeMargarida Yes, but that should be normal behaviour because shallowMount stubs the <router-view/>. I describe this in the added code comments. I don't know how to unstub <router-view/> using shallowMount.

@BeeMargarida
Copy link
Contributor

BeeMargarida commented Apr 23, 2021

@Bastczuak Yes, you are right 😄
With the test you wrote, I wrapped it with the itDoNotRunIf and it ignores shallowMount, like so:

itDoNotRunIf(
    mountingMethod.name === 'shallowMount',
    'returns a VueWrapper instance by CSS selector if the element binds a Vue instance',
    async () => {
        ....
    }
)

So, I think I found the problem. The "old" logic checked if the selector was functional and, if not, used the node.child. In your test-case, the component is not functional, and in the "old" logic, it would end up using node.child. However, with the "new" logic, it will default to the functionalOptions if they are defined. In the router use case, it is defined, but is pointing to the RouterView. Problem: when entering vmCtorMatches,the node.child contains the extendOptions that match with the wanted selector, which the functionalOptions of the node RouterView does not contain (since its not this node we want to compare to).

So, for this use case, we need the child, but for the test made in the other PR we need the functionalOptions. I don't know enough to know how can we distinct between functional nodes and the RouterView node.

("new" logic with the test case created by @Bastczuak, in this case the _Ctor won't have the extendedOptions` that match the selector)
image

TLDR: when using the RouterView node, we want to use the node.child in the matches, however in the functional components, we want the inverse. The current logic will always default to the functionalOptions of the routerview node instead of this child.

@leungk712
Copy link

leungk712 commented Apr 23, 2021

Hello, would this by chance have affected v1.1.1? At my job, we've had router tests that have passed for 7-8 months until this past week where things have just failed out of nowhere. We haven't even touched the codebase either within the past 2 weeks.

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 26, 2021

@leungk712 no, this was introduced in 1.1.4 according to the bug report. You must have changed something (updating the dependencies?)

Regarding " I don't know how to unstub using shallowMount" - you can't, it's shallowMount. The point is to stub all the things.

@BeeMargarida great investigation. Is there any reason we cannot just revert to the previous logic, then add in the additional code that was added in the PR that caused this regression? That (should) cover all the cases (I think... the shallowMount and findComponent logic is so complicated, I would need to re-familiarize myself with it).

@BeeMargarida
Copy link
Contributor

BeeMargarida commented Apr 26, 2021

@BeeMargarida great investigation. Is there any reason we cannot just revert to the previous logic, then add in the additional code that was added in the PR that caused this regression? That (should) cover all the cases (I think... the shallowMount and findComponent logic is so complicated, I would need to re-familiarize myself with it).

@lmiller1990 We can maintain the old logic (and fix this bug) and support the use case added in the PR #1817 if we changed this test

wrapper.find({ name: 'test-functional-component' }).exists()
to wrapper.find({ name: 'test-functional-component', functional: true }).exists(). But I don't know if it goes against the feature added in the PR.

@Bastczuak
Copy link

Bastczuak commented Apr 27, 2021

@BeeMargarida @lmiller1990 Ok I updated my merge request #1832 with a really naive approach, but all tests pass now. Oh and btw, I am sorry, I didn't really know why the commit hooks failed using webstorm. Now I know that my commit messages are crap. I guess I have to create a new merge request later.

@stefpb
Copy link

stefpb commented Apr 28, 2021

After upgrade to 1.1.4 one of our thousands tests also fails. We don't know why, because the one is pretty similar to other working tests. How we can test the new new version of 1.1.4?

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 29, 2021

Thanks for the PR @Bastczuak, I will give this a look today.

@stefpb if you could test this out with your code base, that would be great! You could just clone this repo, yarn build, then copy the built files, eg this directory into your own project inside node_modules/@vue/test-utils/dist. Simple but effective 👍

Could you please do this? That would give me more confidence when releasing this patch.

@stefpb
Copy link

stefpb commented Apr 29, 2021

Yes, i did exactly what you said, but it has the same behavior: One Test out of our thousend tests now fails. So for the user view nothing changed.

Don't work for me :/

@wolfgangwalther
Copy link

Yes, i did exactly what you said, but it has the same behavior: One Test out of our thousend tests now fails. So for the user view nothing changed.

Are you sure you built the PR and not the current dev branch? The PR doesn't even build for me (same as CI), so you probably did not test the fix.


Thanks for the PR @Bastczuak, I will give this a look today.

@stefpb if you could test this out with your code base, that would be great! You could just clone this repo, yarn build, then copy the built files, eg this directory into your own project inside node_modules/@vue/test-utils/dist. Simple but effective +1

I cloned the PR, fixed the build error and tested it. Out of 54 failed tests with plain 1.1.4, 53 worked again. I think those were all related to RouterView. I do have one remaining failing test where I do wrapper.getComponent(RouterLinkStub). Didn't look any deeper.

@stefpb
Copy link

stefpb commented Apr 29, 2021

Ah, could be! I did this steps:
In our project: Checkout 1.1.3, run the tests => all tests green.

Outside the project:

git clone https://github.com/vuejs/vue-test-utils.git
cd vue-test-utils  # now i'm in the branch 'dev'
yarn
yarn build
cp packages/test-utils/dist/* <project-root>/node_modules/@vue/test-utils/dist/

Again inside the project: Run again tests, but then 1 tests failed.

How i can checkout the code of the PR?

@wolfgangwalther
Copy link

wolfgangwalther commented Apr 29, 2021

Instead of cloning the official repo, clone @Bastczuak's fork:

git clone --branch issue-1820 https://github.com/Bastczuak/vue-test-utils.git

then proceed the same way.

(Alternatively add this repo as a remote, if you know how to do it)

@lmiller1990
Copy link
Member

Right, you will need the fork with the branch. Let me know when you try it out. If it is successful, I can merge and release straight away.

@wolfgangwalther
Copy link

Right, you will need the fork with the branch. Let me know when you try it out. If it is successful, I can merge and release straight away.

Have you read my message above? I tested it, too - and it does not solve the regression fully, yet.

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 29, 2021

Right, I missed your message. I see - one bug remaining.

Can you share the failing test (ideally, simplify it as much as possible). Is it just:

import { RouterLinkStub } from '@vue/test-utils'

const Comp = {
  stubs: { RouterLink: RouterLinkStub },
  template: `<router-link to="/foo" />`
}

shallowMount(Comp).getComponent(RouterLinkStub)

?

We can add it to this code base (probably with the current PR) and get this merged and released. I think it should not be too difficult once we have a reproduction.

Weird it's only a RouterLinkStub test failing. That's the only component we export. I wonder if that's related. Are you using this exported component or a default stub?

@wolfgangwalther
Copy link

Didn't have the time to make it a reproducible test-case, but I use this:

stubs: {
  RouterLink: RouterLinkStub
},

and

wrapper.getComponent(RouterLinkStub)

The getComponent fails now, but passed before. I'm sure the component is still rendered.

@wolfgangwalther
Copy link

Is it just:

import { RouterLinkStub } from '@vue/test-utils'

const Comp = {
  stubs: { RouterLink: RouterLinkStub },
  template: `<router-link to="/foo" />`
}

shallowMount(Comp).getComponent(RouterLinkStub)

?

Yeah, that's about it, not much more to it. If it doesn't fail for you, I can dig deeper.

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 29, 2021

This is working perfectly for me:

  it.only('findComponent on a RouterLinkStub', () => {
    const Comp = {
      name: 'Comp',
      template: `<router-link to="/foo" />`
    }
    const wrapper = mountingMethod(Comp, {
      stubs: {
        RouterLink: RouterLinkStub
      },
    })
    wrapper.getComponent(RouterLinkStub)
  })

Will need a reproducible test case.

If we cannot solve by the end of the week, we can release what we have and keep exploring. If you can reproduce the problem I can take a look.

@wolfgangwalther
Copy link

This is working perfectly for me:

  it.only('findComponent on a RouterLinkStub', () => {
    const Comp = {
      name: 'Comp',
      template: `<router-link to="/foo" />`
    }
    const wrapper = mountingMethod(Comp, {
      stubs: {
        RouterLink: RouterLinkStub
      },
    })
    wrapper.getComponent(RouterLinkStub)
  })

Will need a reproducible test case.

If we cannot solve by the end of the week, we can release what we have and keep exploring. If you can reproduce the problem I can take a look.

Right. I copied your test into my codebase - works, too! I noticed my component, that uses <router-link> is actually a functional component.

Changing your test to:

  it.only('findComponent on a RouterLinkStub', () => {
    const Comp = {
      name: 'Comp',
      functional: true,
      render (h) { return h('router-link', { props: { to: "/foo" } }) }
    }
    const wrapper = mountingMethod(Comp, {
      stubs: {
        RouterLink: RouterLinkStub
      },
    })
    wrapper.getComponent(RouterLinkStub)
  })

makes it fail for me, similar to my test-case.

I tried the following as well, but this results in a different error:

  it.only('findComponent on a RouterLinkStub', () => {
    const Comp = {
      name: 'Comp',
      functional: true,
      template: `<router-link to="/foo" />`
    }
    const wrapper = mountingMethod(Comp, {
      stubs: {
        RouterLink: RouterLinkStub
      },
    })
    wrapper.getComponent(RouterLinkStub)
  })

results in

    TypeError: Cannot read property 'call' of undefined

       9 |       template: `<router-link to="/foo" />`
      10 |     }
    > 11 |     const wrapper = shallowMount(Comp, {
         |                     ^
      12 |       stubs: {
      13 |         RouterLink: RouterLinkStub
      14 |       },

      at createFunctionalComponent (node_modules/vue/dist/vue.runtime.common.dev.js:3052:30)
      at createComponent (node_modules/vue/dist/vue.runtime.common.dev.js:3225:12)
      at _createElement (node_modules/vue/dist/vue.runtime.common.dev.js:3427:13)
      at createElement (node_modules/vue/dist/vue.runtime.common.dev.js:3347:10)
      at vm.$createElement (node_modules/vue/dist/vue.runtime.common.dev.js:3487:54)
      at Proxy.parentComponentOptions.render (node_modules/@vue/test-utils/dist/vue-test-utils.js:2643:12)
      at VueComponent.Vue._render (node_modules/vue/dist/vue.runtime.common.dev.js:3538:22)
      at VueComponent.updateComponent (node_modules/vue/dist/vue.runtime.common.dev.js:4054:21)
      at Watcher.get (node_modules/vue/dist/vue.runtime.common.dev.js:4465:25)
      at new Watcher (node_modules/vue/dist/vue.runtime.common.dev.js:4454:12)
      at mountComponent (node_modules/vue/dist/vue.runtime.common.dev.js:4061:3)
      at VueComponent.Object.<anonymous>.Vue.$mount (node_modules/vue/dist/vue.runtime.common.dev.js:8392:10)
      at mount (node_modules/@vue/test-utils/dist/vue-test-utils.js:14032:21)
      at shallowMount (node_modules/@vue/test-utils/dist/vue-test-utils.js:14058:10)

@wolfgangwalther
Copy link

wolfgangwalther commented Apr 29, 2021

This seems to be unrelated to RouterLinkStub - I tried replacing that component with a different simple component Comp2 and it's the same problem, even without any stub.

  it.only('findComponent in functional component', () => {
    const Comp2 = {
      name: 'test',
      render (h) { return h('div', 'test') }
    }
    const Comp = {
      name: 'Comp',
      functional: true,
      render (h) { return h(Comp2) }
    }
    const wrapper = shallowMount(Comp, {
    })
    wrapper.getComponent(Comp2)
  })

This fails for me. In both 1.1.4 and the open PR. Confirmed again: This very test-case passes just fine in 1.1.3.

@lmiller1990
Copy link
Member

I see. So the problem is related to getComponent and functional: true. Good to know - now we have a repro, this should be easy to fix. Let's get a fix in for this asap. I can look at it soonish. If someone else wants to give it a try, that'd be great, too.

@lmiller1990
Copy link
Member

Worked on this a bit: #1835

Grabbed some code from @Bastczuak's branch and the regressions here. This part of the code base is so complicated. I just added all the tests cases and wrote code until they all passed ✅

🤞 CI passes.

@lmiller1990
Copy link
Member

@wolfgangwalther @stefpb could either of you pull and build this branch and run it against your code-bases? #1835

There is one test with routing that fails on versions < 2.6. I am not entirely sure why, the matching logic so complicated, I just added all the regressions and hacked until it passed.

@wolfgangwalther
Copy link

@wolfgangwalther @stefpb could either of you pull and build this branch and run it against your code-bases? #1835

Works for me, all tests pass.

@Bastczuak
Copy link

@lmiller1990 I also can confirm that all tests pass in my code-base with your branch.

@lmiller1990
Copy link
Member

Great, I will test a bit more and release a new version this weekend.

@lmiller1990
Copy link
Member

Some tests are only failing on CI, but not locally. Please wait a bit all - I need to figure out what's going on...

@lmiller1990
Copy link
Member

lmiller1990 commented May 1, 2021

Ok, it's out: https://github.com/vuejs/vue-test-utils/releases/tag/v1.2.0

A little nervous, I hope we did not introduced any more regressions. Please test and let me know how this goes.

@stefpb
Copy link

stefpb commented May 1, 2021

When i try git clone --branch issue-1820 https://github.com/Bastczuak/vue-test-utils.git
i get this error:

Klone nach 'vue-test-utils' ...
remote: Repository not found.
fatal: Repository 'https://github.com/Bastczuak/vue-test-utils.git/' not found.

And also the new version 1.2.0 have the same behavior. So for me it isn't solved :/

@lmiller1990
Copy link
Member

@stefpb can you post a minimal reproduction? All the tests are passing for the all reproductions in this thread - I think you mentioned you have a problem with RouterLinkStub. I tried to reproduce problems with it but I could not do so - will need a reproduction to add to our test suite so we can fix this.

@stefpb
Copy link

stefpb commented May 10, 2021

In this special case the test emits on a component that is also a stub. We used the selector for the child component, which isn't there anymore, when we upgrade to vue-test-utils 1.1.4. But now we use the stub-selector and now the test is passing.

@ravjsdev
Copy link

ravjsdev commented May 10, 2021

since updating to 1.1.4 (or greater) my unit tests are now breaking - I have a custom component (renderer) which renders form components (e.g. input, text area, checkbox etc) dynamically using a config provided via props. The component iterates through the config and renders the inputs (mapping them to appropriate vue components) - now in my test when i do the following:

...
const dynamicFormConfig = ['text-area', 'text-input'];

const options = merge({
    propsData: {
      dynamicFormConfig,
    },
    store,
    localVue,
  }, params.options);
textInput = mount(RendererComponent, options).findComponent({` name: 'MyTextInput' })
expect(textInput.exists()).toBe(true); // i should get true, but now my tests fail with false.

I should get true from the test expectation but now im getting false. When i downgrade to 1.1.3 everything works fine and the tests pass.

@lmiller1990
Copy link
Member

@ravjsdev thanks for the bug report. Since it seems there's a bunch of edge cases, let's solved them one by one. Please open a new issue with your exact reproduction. I cannot reproduce your issue with your comment alone. Thanks!

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