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

Async exceptions in beforeMount not caught in unit tests #1503

Closed
Yodablues opened this issue Apr 10, 2020 · 16 comments
Closed

Async exceptions in beforeMount not caught in unit tests #1503

Yodablues opened this issue Apr 10, 2020 · 16 comments

Comments

@Yodablues
Copy link

Version

1.0.0-beta.33

Reproduction link

https://codesandbox.io/s/white-dream-1vmod

Steps to reproduce

Create a new component that throws an exception in an async beforeMount. In my Test.vue component, I create a simple component that waits 500ms and then throws an exception.

Create a test that asserts exceptions are being thrown when mount/shallowMount are called.
In my test, I expect.assertions(1) to let Jest know to expect a single exception. Then I wrapped shallowMount in a try/catch and call done() in finally.

What is expected?

Exceptions would be caught in the test suite.

What is actually happening?

Exceptions are not caught and the test fails.


In non-async beforeMount, exceptions are caught, however they still get logged to the console.

@lmiller1990
Copy link
Member

I see the exception in the console, but the test is passing in the sandbox you shared.

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 11, 2020

That said this fails on my local machine.

Should this be getting caught in the test runner? Maybe relevant: vuejs/vue#10826 and a few others.

I am not sure how we can catch this error in the runner from the test.

@Yodablues
Copy link
Author

Yodablues commented Apr 11, 2020

Yeah, for some reason it passed in the sandbox, but fails locally too. I assume it's something with the way codesandbox is setup. Sorry for the confusion.

I would expect it to be caught since I'm telling Jest to expect an assertion. And if you remove the async/await from beforeMount, it does get caught. So I'm not sure if it's an issue with vue-test-utils or how my test is setup when testing async lifecycle methods.

@Yodablues
Copy link
Author

I move the reproduction steps a git repo to make it easier to test.

https://github.com/Yodablues/async-lifecycle-example

There are 2 components, Async.vue, NonAsync.vue and each has a single unit test. Both components throw exceptions in beforeMount, but it doesn't look like either is being caught correctly.

Basically, I want to to test that an exception gets thrown in an Async lifecycle hook and I'm just not sure if I'm doing it correctly.

@lmiller1990
Copy link
Member

Thanks for this. I haven't found a way to catch this error - I'll try a little bit more... let me know if you come up with anything.

VTU has some logic around catching errors, potential bug there:

export function throwIfInstancesThrew(vm) {

@AtofStryker
Copy link
Contributor

Hey @Yodablues. I took a look into your reproduction repo. There was a conflict with the VTU version in devDependencies vs dependencies, so I bumped the version in my local branch to 1.0.3 just to get the most recent behavior.

I was able to catch the error in the NonAsync test, but not Async test. This is due to Vue executing those lifecycle hooks in a synchronous fashion, even though those hooks can return a Promise or be defined in an async manner. There was discussion about this in the past and I believe there are not plans to support async execution.

This gives us a few options we could potentially pursue

  1. Make shallowMount and mount async functions to be await able. There currently isn't a very clean way to integrate this with Vue, so it would likely be somewhat hacky, and could have some serious performance implications. We would also be introducing a breaking change here. I am more of the opinion that this isn't a very common use case and do not think the tradeoffs are worth it, considering the impact.

  2. Do nothing. Probably not the most preferable option, but that error could be intercepted right now by specifying a errorHandler and using the err and info arguments to determine where they came from. The below snippet should work to intercept the async error.

import Async from "@/components/Async.vue";
import { shallowMount, createLocalVue } from "@vue/test-utils";
import Vue from 'vue'

describe("Async.vue", () => {
  it("catch exception", async done => {
    Vue.config.errorHandler = function (err, vm, info) {
      if(info.includes("beforeMount")){
        expect(err.message).toBe("test");
      }
      done()
    }
    const localVue = createLocalVue()
    shallowMount(Async, {
      localVue
    });
  });
});

You will also get this annoying error if you implement the above

[vue-test-utils]: Global error handler detected (Vue.config.errorHandler). 
Vue Test Utils sets a custom error handler to throw errors thrown by instances. If you want this behavior in your tests, you must remove the global error handler.
  1. Give createLocalVue a constructor option to take a global config options object. The problem with option 2 in the above example is it is adding a errorHandler to the global config, which totally defeats the purpose of using createLocalVue! I believe something like this was once supported, but is no longer the case.

Unfortunately, the below does not work due to how createLocalVue is implemented

import Async from "@/components/Async.vue";
import { shallowMount, createLocalVue } from "@vue/test-utils";
import Vue from 'vue'

describe("Async.vue", () => {
  it("catch exception", async done => {
    const localVue = createLocalVue()
    localVue.config.errorHandler = function (err, vm, info) {
      if(info.includes("beforeMount")){
        expect(err.message).toBe("test");
      }
      done()
    }
    shallowMount(Async, {
      localVue
    });
  });
});

But this is the behavior we would desire. I believe, fairly easily, we could implement something that could look like this:

const localVue = createLocalVue({
  errorHandler(){
     // if provided, call this error handler, followed by the global error handler if one exists
  },
  optionMergeStrategies(){
  // some implementation
  }
  // other global config options
})

We can support virtually any of the options present in the Global Config and handle this behavior accordingly. I think this would allow, in this case, the errorHandler to be cleaned up after the test.

Sorry for the essay here, but I think in general the community could likely benefit from a feature like this. What do you think @lmiller1990 ?

@lmiller1990
Copy link
Member

lmiller1990 commented Aug 1, 2020

Great summary.

  1. is a no go imo, introducing breaking change for a fairly uncommon scenario doesn't feel great. I really value stability and avoiding breaking changes.
  2. doing nothing is not really ideal (that's the current status of this ticket, though).
  3. this looks the best. Passing an option to createLocalVue seems pretty clean!

If there is an easy way to implement this feature via createLocalVue, I don't see any reason why we shouldn't :) most of my OSS time is spent on vue-jest and the Vue 3 version of this library, but if you are motivated to implement this feature I am sure people will be thankful!

Also, we should see how VTU next handles this problem/if the same thing happens there. Vue 3 internals (and VTU next) is quite different, although the API is the same.

@AtofStryker
Copy link
Contributor

@lmiller1990 Does VTU 2.x handle Vue 3.x's Application Config at all? Do we think there is a sound use case to bake that functionality like we are talking about here? Maybe we can construct an RFC and go from there? Sorry for so many questions 😆

@lmiller1990
Copy link
Member

Yes VTU 2.x has it see here. Looks like it isn't documented. I made an issue for documenting it vuejs/test-utils-docs#49

I have no idea if Vue 3.x has custom errorHandler (probably does). createLocalVue does not exist anymore since it doesn't make sense, each Vue app is independent via createApp, so the mutable global instance problem doesn't really exist anymore.

We should find out if this is a problem in VTU 2.x and go from there.

@AtofStryker
Copy link
Contributor

Sounds like a plan. I can put an example together that we talked about above using VTU 2.x / Vue 3.x . Since VTU supports the App config, we should just be able to use the error handler option. My guess is this shouldn't be problematic, especially since createApp scopes these configs on a per app basis and we don't need to worry about the global instance issues (so glad for this being a thing in Vue 3 ecosystem). I don't think this will be a problem in VTU 2.x, but the example will be telling. Will update when I have something!

@AtofStryker
Copy link
Contributor

@lmiller1990 finally had a chance to try this out with VTU 2.x. Using the Async component @Yodablues listed above, I was able to come up with the following. It worked pretty much like we expected it to 🎉 . I did have some problems with typings for the config handler. I will make an issue on vue-test-utils-next with a reprod. I am probably doing something wrong since this my first foray into vue 3 and VTU 2.x 😆 .

The reprod link for this is here. The question now is, how do we want to support this functionality here?

// @ts-ignore
import Async from "./Async.vue";
import { shallowMount } from '@vue/test-utils'
import { createApp } from 'vue'

describe("Test Error Handler Vue 3", () => {
  it("Mounts a global errorhandler without issue", async (done) => {
    const app = createApp({})
    app.config.errorHandler = (err: any, vm, info) => {
      if(info.includes("beforeMount")){
        expect(err.message).toBe("test")
      }
      done()
    }

    shallowMount(Async, {
      global: {
        // @ts-ignore
        config: {
          errorHandler: app.config.errorHandler
        }
      }
    });
  });
});

@lmiller1990
Copy link
Member

Just to confirm: using the custom errorHandler strategy works fine in VTU V2?

If we can implement it here easily with createLocalVue, seems like we should.

@AtofStryker
Copy link
Contributor

Just to confirm: using the custom errorHandler strategy works fine in VTU V2?

@lmiller1990 Correct. Besides the typings issue it worked great!

If we can implement it here easily with createLocalVue, seems like we should.

We should be able to add it relatively easy without a lot of effort. From what I can see, createLocalVue takes an argument, which is the vue instance. I don't think this argument is used anywhere, except for the in the default case ( IE, not the public API). It doesn't look to be documented. Based on that knowledge, I believe it should be safe to either

a) move the argument to the second argument space to implement the below RFC
b) remove the argument entirely to implement the below RFC

const localVue = createLocalVue({
  errorHandler(){
     // if provided, call this error handler, followed by the global error handler if one exists
  },
  optionMergeStrategies(){
  // some implementation
  }
  // other global config options
})

I can (or anyone else if they want to!) start work on getting the feature implemented and open a PR with the proposed design/ RFC. Not really a true RFC, but probably for this use case it should be fine. And we can take it from there. What do you think?

@lmiller1990
Copy link
Member

I have never seen anyone use the first argument to createLocalVue. If this is undocumented, I don't think we need make an RFC. I am happy to go with (b).

We could make this next release 1.1, since it has a both adds a new feature and has a "breaking" change (which is fine for an internal, undocumented API).

@AtofStryker
Copy link
Contributor

@lmiller1990 @Yodablues I took a stab at implementing this feature in #1670. Lemme know what you all think!

@AtofStryker
Copy link
Contributor

@lmiller1990 @Yodablues Should we close this issue out since the feature to solve this issue is now implemented in 1.1.0?

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

3 participants