Add tests using async components #105

lmiller1990 · 2020-05-02T13:49:58Z

They work as expected! Caveats:

since they are async, you need to do flushPromises to make them resolve. This is pretty common in VTU. Just need good education around this. Evan does something similar in core to test them, see here
It errors out with cannot find innerHTML of parentElement if an AsyncComponent is the root component. return this.vm.$el.parentElement is null - I am not sure why. 🤔
- Work around is to wrap with a div. See my "basic usage" test.

Anyone see any other test cases we could add?

afontcu · 2020-05-02T18:46:46Z

tests/features/async-components.spec.ts

+      onError(error, retry, fail, attempts) {
+        fail()
+      }


I think this onError is a bit redundant? The option is used to have some retry control, but here we're just failing no matter what.

Suggested change

onError(error, retry, fail, attempts) {

fail()

}

Hm, also the test is passing but it yields some warns and errors:

Not sure if these error logs are somewhat expected, and we should just mock console for the test and call it a day… 🤔

I had this thought too. Some other tests also throw and error. Is there any benefit to mocking the console? If printing the error is correct, I don't keep it.

I tend to mute them to make it the output cleaner in the long run, but I don't care much either. Unless the console error is part of the expected output, then I spy it and assert the times called and so.

(I still believe the onError bit can be removed)

We can have our own Vue errorHandler, to easily assert on it without mocking console.error

I will make this change, thanks

SergiCL · 2020-05-03T10:23:26Z

tests/features/async-components.spec.ts

+    setTimeout(() => {
+      expect(wrapper.html()).toContain('Loading Component')
+    }, 35)
+
+    setTimeout(() => {
+      expect(wrapper.html()).toContain('Async Component')
+      done()
+    }, 100)


Why not use jest.useFakeTimers? Mocking the setTimeout would make the test 10 times faster.
Something like this seems to work fine, but there may be something I'm not seeing:

Suggested change

setTimeout(() => {

expect(wrapper.html()).toContain('Loading Component')

}, 35)

setTimeout(() => {

expect(wrapper.html()).toContain('Async Component')

done()

}, 100)

await jest.advanceTimersByTime(35)

expect(wrapper.html()).toContain('Loading Component')

jest.advanceTimersByTime(100)

await flushPromises()

expect(wrapper.html()).toContain('Async Component')

I did not know jest had a fake timer, that is pretty cool.

dobromir-hristov · 2020-05-03T12:26:40Z

tests/components/LoadingComponent.vue

@@ -0,0 +1,3 @@
+<template>


Can we just use any sfc?

Sure, I can use one of the existing ones

dobromir-hristov · 2020-05-03T12:35:32Z

tests/features/async-components.spec.ts

+    }, 100)
+  })
+
+  it('works with vue files', async () => {


isnt this test the same as the first one?

Oops, it is.

I can change this one to test this usage

defineAsyncComponent({ errorComponent: Hello })

where errorComponent is a regular import at the top of the file

dobromir-hristov · 2020-05-03T12:46:04Z

tests/features/async-components.spec.ts

+      onError(error, retry, fail, attempts) {
+        fail()
+      }


We can have our own Vue errorHandler, to easily assert on it without mocking console.error

lmiller1990 · 2020-05-04T00:29:41Z

Great reviews everyone, I will make the requested changes and update the PR.

cexbrayat · 2020-05-04T06:04:02Z

@lmiller1990 👍 I'm also in favor of using jest.useFakeTimers (I already thought so when seeing the Suspense test). Also note that flushPromises is not necessary, as you could write a bunch a few nextTick and achieve the same. It just far more convenient. I have a few tests in my app that use it, and was also wondering if we should not expose it ourselves 🤔

You could also add a test of a component with an async setup, meant to be used in a Suspense? They are a bit cumbersome to test currently, as you need a wrapper component defining the Suspense but it works.

lmiller1990 · 2020-05-04T10:05:29Z

For async setup components, I have not found a good way to test these - in my apps I either test the component "one level up" (with <Suspense> or wrap the component in a <Suspense> in the test. I then call await flushPromises to make it render. If you just mount it, Vue complains about "No suspense wrapper".

Does anyone have any good ideas for this?

I had the idea for a mountInSuspense function - but it's just as easy for a user to implement that themselves. I see two options:

have some simple FAQs and show the user how to write a helper
provide a helper

I am leaning towards the first, if something is simple enough the user can do it in user-and, just let them do it. People should absolutely write helpers if it makes their tests more concise. Give the power to the user!

That said, if this going to come up a lot, we can preemptively solve it with a helper. My only fear is we end up many helper functions, which makes the library larger and more complex than it needs to be.

I will also make the nextTick change and fakeTimers change.

cexbrayat · 2020-05-04T10:15:18Z

@lmiller1990 I tested a few async components in my app and ended up with the same conclusions 👍 I wanted to open an issue to talk about this, and gather your thoughts.

I think it would be nice to offer a helper that would allow to mount an async component easily, either a different mount, or with a mount option, or just a helper to wrap the component to test. I completely understand that this would make the library bigger and that we might not want to do it, and simply write a good cookbook on the topic.

lmiller1990 · 2020-05-04T10:36:44Z

Alternatively, if we can know ahead of time it's an async setup function... can we just do the work for them in the background so it "just works"?

afontcu · 2020-05-04T10:57:26Z

I am leaning towards the first, if something is simple enough the user can do it in user-and, just let them do it. People should absolutely write helpers if it makes their tests more concise. Give the power to the user!
That said, if this going to come up a lot, we can preemptively solve it with a helper.

Agree 100%

lmiller1990 · 2020-05-04T11:00:11Z

I made most of the changes except flushPromises to nextTick (did not try the Suspense helper yet, I will dot hat now). flushPromises to nextTick are different - nextTick is DOM updates only, flushPromises does what the name suggests. Eg, in the first test we need flushPromises to the defineAsyncComponent promise is resolved. Or maybe I am missing something? When I changed it to nextTick it would fail sometimes.

With trigger, setProps etc now returning nextTick, there should be no scenarios where you next to do await nextTick yourself, (right?) If you need a promise (eg async import, axios whatever) you need to use flushPromises.

lmiller1990 · 2020-05-04T11:04:33Z

@afontcu can you confirm which thing you are 100% agreeing to (util function or user-land)?

pikax · 2020-05-04T11:11:01Z

When I changed it to nextTick it would fail sometimes.

nextTick does a microTask but flushPromises is a macroTask

EDIT: https://stackoverflow.com/questions/25915634/difference-between-microtask-and-macrotask-within-an-event-loop-context

EDIT2: Basically the Vue updates the DOM with a micro task, but the asyncComponent gets updated with a macrotask, using nextTick for asyncComponent is not reliable because microtasks run before the macrotask

lmiller1990 · 2020-05-04T11:19:13Z

^ This was my understanding - thanks for clarifying. We will stick with flush-promises here for this reason.

afontcu · 2020-05-04T11:21:09Z

@afontcu can you confirm which thing you are 100% agreeing to (util function or user-land)?

both! I'd start with docs and let user-land figure out a way if needed, and if this comes up a lot during alpha/beta stages, then we could reconsider.

lmiller1990 · 2020-05-04T11:30:50Z

Let's continue discussing mount helpers in another issue: #108

We still have a console warning " is an experimental feature and its API will likely change." Don't know we need to silence this warnings - it's coming from Vue itself, for a good reason.

I made the changes, I'll leave this open another day or so for anyone who might like to go over it again.

lmiller1990 added 2 commits May 2, 2020 23:46

tests: add tests for async components

8e0ef44

fix: revert file

0d56136

lmiller1990 requested review from afontcu, dobromir-hristov and pikax May 2, 2020 13:50

afontcu reviewed May 2, 2020

View reviewed changes

SergiCL reviewed May 3, 2020

View reviewed changes

dobromir-hristov reviewed May 3, 2020

View reviewed changes

refactor: add custom error handler

f967819

lmiller1990 mentioned this pull request May 4, 2020

Helpers: Suspense (and others?) #108

Closed

refactor: pass errorHandler on a test by test basis

9c13813

lmiller1990 merged commit 2fc394b into master May 7, 2020

lmiller1990 deleted the tests/async-component branch May 7, 2020 10:06

Add tests using async components #105

Add tests using async components #105

Uh oh!

Conversation

lmiller1990 commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afontcu May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmiller1990 commented May 4, 2020

Uh oh!

cexbrayat commented May 4, 2020

Uh oh!

lmiller1990 commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cexbrayat commented May 4, 2020

Uh oh!

lmiller1990 commented May 4, 2020

Uh oh!

afontcu commented May 4, 2020

Uh oh!

lmiller1990 commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmiller1990 commented May 4, 2020

Uh oh!

pikax commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmiller1990 commented May 4, 2020

Uh oh!

afontcu commented May 4, 2020

Uh oh!

lmiller1990 commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lmiller1990 commented May 2, 2020 •

edited

Loading

afontcu May 2, 2020 •

edited

Loading

lmiller1990 commented May 4, 2020 •

edited

Loading

lmiller1990 commented May 4, 2020 •

edited

Loading

pikax commented May 4, 2020 •

edited

Loading

lmiller1990 commented May 4, 2020 •

edited

Loading