testing-library / vue-testing-library

🦎 Simple and complete Vue.js testing utilities that encourage good testing practices.
http://testing-library.com/vue
MIT License
1.08k stars 110 forks source link

VTL for Vue 3: The routes option does not work correctly #195

Closed lbavendanos closed 3 years ago

lbavendanos commented 3 years ago

Describe the bug When using the routes option of the render function, it throws up some warnings that have to do with the _history property and the asynchronic handling that vue router 4 has. This causes you to not be able to use the routes option correctly.

To Reproduce

import { render, screen } from '@testing-library/vue'
import '@testing-library/jest-dom'
import { defineComponent } from 'vue'
import { RouteRecordRaw } from 'vue-router'

test('Component with route', () => {
  const ComponentA = defineComponent({
    name: 'ComponentA',
    props: {
      to: {
        type: Object,
        required: true
      }
    },
    template: `<router-link :to="to" role="button">Learn More</router-link>`
  })

  const ComponentB = defineComponent({
    name: 'ComponentB'
  })

  const routeRecordRaw: RouteRecordRaw = {
    path: '/',
    name: 'componentB',
    component: ComponentB
  }

  const to = { name: routeRecordRaw.name }

  render(ComponentA, {
    props: { to },
    routes: [routeRecordRaw]
  })

  const button = screen.getByRole('button')

  expect(button).toBeInTheDocument()
  expect(button?.getAttribute('href')).toBe(routeRecordRaw.path)
})

This throws the following warning

console.warn node_modules/vue-router/dist/vue-router.cjs.js:75
    [Vue Router warn]: Unexpected error when starting the router: TypeError: Cannot read property '_history' of null

The warning is known. This is explained in the vue-test-utils v2 documentation https://vue-test-utils.vuejs.org/v2/guide/vue-router.html#with-a-real-router

So to solve it apply what is indicated in documentation

import { render, screen } from '@testing-library/vue'
import '@testing-library/jest-dom'
import { defineComponent } from 'vue'
import { createRouter, createWebHistory, RouteRecordRaw } from 'vue-router'

test('Component with route', async () => {
  const ComponentA = defineComponent({
    name: 'ComponentA',
    props: {
      to: {
        type: Object,
        required: true
      }
    },
    template: `<router-link :to="to" role="button">Learn More</router-link>`
  })

  const ComponentB = defineComponent({
    name: 'ComponentB'
  })

  const routeRecordRaw: RouteRecordRaw = {
    path: '/',
    name: 'componentB',
    component: ComponentB
  }

  const router = createRouter({
    history: createWebHistory(process.env.BASE_URL),
    routes: [routeRecordRaw]
  })

  router.push('/')
  await router.isReady()

  const to = { name: routeRecordRaw.name }

  render(ComponentA, {
    props: { to },
    global: {
      plugins: [router]
    }
  })

  const button = screen.getByRole('button')

  expect(button).toBeInTheDocument()
  expect(button?.getAttribute('href')).toBe(routeRecordRaw.path)
})

Expected behavior Be able to correctly use the routes option provided by the render function

Related information:

afontcu commented 3 years ago

Hi, thanks for the report!

We're already setting up a router with createWebHistory. Looks like we could simply add

routerPlugin.push('/')
await routerPlugin.isReady()

after this line and your first test would pass, but that would make render async, which is undesirable.

A potential solution is to expose router.isReady, so users could wait for it and then, in the test:

const {isRouterReady} = render(ComponentA, {
  props: {to},
  routes: [routeRecordRaw],
})

await isRouterReady() // isRouterReady is just routerPlugin.isReady

Gave it a go with your test and it works, but it's not… well, user-focused, and people need to be aware of it. I wonder why no combination of async waiting (using waitFor) works…

I'm gonna ping @lmiller1990 as he has already put in some work to integrate VTU and vue router, so he might come up with a better solution :)

lmiller1990 commented 3 years ago

This is pretty much what I've been doing so far - await router.isReady().

I am not sure of a more ergonomic way of handling this :thinking: docs? Or can we do this internally somewhere?

afontcu commented 3 years ago

Or can we do this internally somewhere?

This was my first approach, but this would make render async, and I'd like to avoid that. Is there any workaround here? I don't think so 🤔

Maybe adding a isRouterReady key isn't that bad, given that we have types and autocompletion and it will be limited to tests involving a router.

lmiller1990 commented 3 years ago

If we don't want render to be async then we will not be able to do this for the user. I suppose we just expose a method and document it well.

Maybe we should just expose the entire router - that seems useful. Eg:

const {router} = render(ComponentA, {
  props: {to},
  routes: [routeRecordRaw],
})
await router.isReady()

Although this may not fit with the VTL philosophy. I think this idea (expose some kind of router key) is probably the way to go.

afontcu commented 3 years ago

Gave it a go. I'm not 100% sold on the idea of exposing the whole router, mostly because, as you mentioned, is something one should not expect from Testing Lib. However we still need to wait for router to be ready… so I see two valid alternatives:

1) Expose a isRouterReady helper (https://github.com/testing-library/vue-testing-library/issues/195#issuecomment-748664373).

2) Detect if people is passing a routes option, and then make render async. People would then need to await render(). It might a bit too magic from a user perspective, though.

Thoughts?

lmiller1990 commented 3 years ago

I think the first one is probably the lesser of the two evils. I have not written many router tests with VTL yet, though, so I am not sure how this actually works out in practice.

Either way, we are leaking implementation details into the test, but I can't see any way around this. I wonder if any of the other integrations have this problem (I don't think React Router is async, but surely some other plugin is? Are we really the first ones to experience this problem?)

afontcu commented 3 years ago

I wonder if any of the other integrations have this problem (I don't think React Router is async, but surely some other plugin is?

Yeah, I'll take a look, but I think VTL is a bit special here because we offer built-in support for external libraries (Vuex, Router), and others don't. That being said, I'll take a look at other implementations and see what they came up with

ITenthusiasm commented 3 years ago

I see router.isReady primarily being a concern here. But is it safe to assume that refers only to the initialization? If so, users would still have the problem of performing awaits while clicking links according to the beta docs. And we're just getting started there. There's also the possibility that maybe certain interactions with the store cause URL updates.

In that case, neither the render solution nor the isRouterReady solution would be sufficient in entirety. (To me, it's a little weird to imply "handling routers is synchronous" in one place and say in another "eh, it's async actually".)

I'm trying to think more about this, but I'm not sure there's a clear workaround. It may just be that people have to be told to await X before continuing when handling router interactions because Vue's Router is async. But that's undesirable for obvious reasons.

ITenthusiasm commented 3 years ago

Would it make sense to have a guide where people could create mocks with the different testing frameworks they're using? Or for VTU to export a mock sync router? I'm not sure how strange that would be. And I'm guessing there's a chance some people may want to verify URL changes still. :thinking:

lmiller1990 commented 3 years ago

Yes, everything about router is now async so this likely will be a problem in a few different respects. I have not written many tests w/ Vue 3 + new Router + testing lib to get a full idea yet.

There is a new project for mocking the router: https://github.com/posva/vue-router-mock but I don't know this will be ideal all situations, and I am also not sure it fits the testing library philosophy.

afontcu commented 3 years ago

Yeah… with Vue 3, all route handling in tests will be async because router is async. So here in VTL we'll need to use waitFor or findByXXXX. I guess we will need to add more examples and docs, but that's how it's gonna be (it is not that bad, to be honest).

That being said, we remain with this issue: how do we wait for the initial router render?

afontcu commented 3 years ago

I wonder if asyncWrapper could help us somehow

ITenthusiasm commented 3 years ago

Yeah. In terms of the options presented earlier, I think I'd probably prefer the conditionally async render. Although it feels a bit weird, it reduces boilerplate (slightly) and potentially reduces the number of things the user can forget. Either way, the user is going to have to await something and be told why that await is necessary unlike in other libraries (at least with those two options). Maybe a warning reminding people to use await when the history error occurs could be helpful too.

As for asyncWrapper, I don't have any familiarity with that one. If it simplifies things that'd be cool. Do you know exactly how it operates over in RTL land?

ITenthusiasm commented 3 years ago

And with the mock router, I guess XTL philosophy could be a problem. I assume the preference would be to use the real router where possible, but that brings things back to square one.

afontcu commented 3 years ago

Another option is to simply accept router object on render (instead of current routes), and do not perform any kind of integration other than pushing it to plugins array.

This would make router handling available to users, but then we'd introduce an imbalance between Router and Vuex.

It goes without saying that this would be a breaking change from VTL for Vue 2

lmiller1990 commented 3 years ago

If the only real problem is the isReady call (eg, use waitFor should solve most other problems) we could think about perhaps adding some special sync behavior to vue-router that is behind a process.env.development flag just for testing. Not sure how practical this is but just throwing it out there.

afontcu commented 3 years ago

adding some special sync behavior to vue-router that is behind a process.env.development flag just for testing

Yeah, I guess that would fix the issue (here and in VTU), but then we'd be testing different behavior than the one in production… which might lead to false positives/negatives, and a decrease in confidence. Not sure it is worth the cost.

ITenthusiasm commented 3 years ago

This would make router handling available to users, but then we'd introduce an imbalance between Router and Vuex.

If the users supplied the router, I guess the store could also be supplied in order to make things consistent. Creating a Vuex store is pretty simple, so there wouldn't be a significant difference in what people are already doing to setup smaller/fake stores or use their real ones.

lmiller1990 commented 3 years ago

I thought about my previous comment, it's a bad idea, actually. We did something similar for Vue 2 ("sync mode") and it had a lot of knock on effects where tests vs the real thing were different. We had both "sync mode" for test utils and something in Vue core.

afontcu commented 3 years ago

Going back to this. As I see it, we're down to two options:

1) Make render async when routes is present, and then call router.isReady() internally. API change: you'd need to await render(Comp, { routes }), but not when routes is not there. We could warn users somehow (plus, TS types would help, too).

2) Remove vuex/router integration for Vue 3, and leave it to users. This would make users responsible for setting up router (and store) and call router.isReady(), which is an implementation detail (even though it is on the Arrange section of tests, which is OK, I guess), and would keep render sync. This solution makes VTL more aligned to other Testing Lib flavors.

To be fair, setting up your own router/vuex isn't that bad now with VTU 2 since there's no need to set up a local vue instance. On the other hand, "hiding" the need to mke router ready feels… right for users.

ITenthusiasm commented 3 years ago

Yeah I'm a little torn on it. I think I'd be fine with either of those two options.

Make render async when routes is present, and then call router.isReady() internally. API change: you'd need to await render(Comp, { routes }), but not when routes is not there. We could warn users somehow (plus, TS types would help, too).

Definitely keeps life simple... in a sense. Having to occasionally await could be weird. And it would probably be new amidst the other testing libraries -- at least for right now. But overall, the user experience wouldn't change too much, I think. People would still have a lot done for them and would still have little to worry about.

Remove vuex/router integration for Vue 3, and leave it to users. This would make users responsible for setting up router (and store) and call router.isReady(), which is an implementation detail (even though it is on the Arrange section of tests, which is OK, I guess), and would keep render sync.

I'm not intimidated by this because unless I'm mistaken, RTL already has to deal with this issue. Things like Routers and Providers need to be provided by the user when using RTL. This leaves the responsibility of setting up routers/stores with the correct configuration up to the user. In his testing videos, Kent C. Dodds shows people how to make a utility function that wraps around the original render to simplify the boilerplate. But the point is that boilerplate is ending up in the user's code -- whether as a utility function or as repeated code in all related tests.

If the second option is chosen, maybe people could be shown how to make a utility function that simplifies the boilerplate for setting up routers and stores. Since RTL already does it, it's not necessarily a bad thing. (As you said, though, it would be a breaking change.)

afontcu commented 3 years ago

Just wrote an RFC with a proposal to fix this issue. In short: remove integration with Router/Vuex, leave plugin management to users, and make the library more similar to other Testing Lib flavors.

https://github.com/testing-library/vue-testing-library/issues/205

any feedback is more than appreciated 🙌

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 6.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

luukdv commented 3 years ago

This seems to work initially, but not on a subsequent route update.

The following succeeds as expected (with Home rendering <h1>Title</h1>):

const router = createRouter({
  history: createWebHistory(),
  routes: [{ path: '/', component: Home }],
});

it('', async () => {
  render(Home, { global: { plugins: [router] }});
  await router.isReady();
  expect(screen.getByText('Title')).toBeInTheDocument();
});

When you click a button that updates a route query param (for example router.push({ query: { a: 'b' } })):

it('', async () => {
  render(Home, { global: { plugins: [router] }});
  await router.isReady();
  expect(screen.getByText('Title')).toBeInTheDocument();

  await fireEvent.click(screen.getByLabelText('Button'));
  await router.isReady();
});

It will throw the Cannot read property '_history' of null error.

Let me know if you'd prefer a separate issue. Thanks in advance!

lmiller1990 commented 3 years ago

I have definitely seen the _history error before. It was related to awaiting the router to be read, but you've already done that.

I wonder if this is a bug here? I feel like it might be either in Test Utils or something else. I wonder if we can reproduce it without Testing Library (VTU + Jest). I wonder if you need to have a top level component (like <app>) with a <router-view /> for this to work as expected.