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

Jasmine fails on errors in React 16 components #1453

Closed
the-spyke opened this issue Nov 27, 2017 · 12 comments
Closed

Jasmine fails on errors in React 16 components #1453

the-spyke opened this issue Nov 27, 2017 · 12 comments

Comments

@the-spyke
Copy link

Expected Behavior

The test should be green

Current Behavior

Test fails.

Possible Solution

Suite that reproduces the behavior (for bugs)

A React component has an error which is caught by an Error Boundary. React prints the error in console and Jasmine fails the test.

CodePen

Context

Not sure if it's desired behavior in Jasmine.

Your Environment

  • Version used: 2.8.0
  • Environment name and version (e.g. Chrome 39, node.js 5.4): Chrome 62, Node 8.9.1
  • Operating System and version (desktop or mobile): Ubuntu 17.04 (4.10.0-40-generic)
  • Link to your project:
@sgravrock
Copy link
Member

I was able to reproduce the issue without using an Error Boundary. It looks like React is triggering an error event whenever a component throws an exception. Failing the current spec in response to an error event is very much expected behavior -- error events during spec execution almost always indicate the kind of bugs that a test suite should catch. But I can see how it makes it difficult to do things like write integration tests for error handling in a React app. I'm open to suggestions for how to improve that scenario without making error handling worse in general.

@the-spyke
Copy link
Author

Yeah, I've added an Error Boundary to my sample to show that I can't catch this error.

Does Jasmine have a way to expect on error events?

@the-spyke
Copy link
Author

My temporary solution for now:

function catchWindowError(testSpec, handleError) {
    const backup = window.onerror;
    const unhandledErrors = [];

    function onUnhandledError(error) {
        unhandledErrors.push(error);
    }

    let regularError;

    try {
        window.onerror = onUnhandledError;

        testSpec();
    } catch (error) {
        regularError = error;
    }

    window.onerror = backup;

    handleError(regularError, unhandledErrors);
}

it("should throw an error on empty props", () => {
    catchWindowError(() => {
        mount(<MyComp />);
    }, error => {
        expect(error).toEqual(new Error("MockCmp requires <name> prop"));
    });
});

@slackersoft
Copy link
Member

Jasmine doesn't currently have any way to expect an error event to occur. We'd be happy to review a pull request to add a new matcher similar to toThrowError that registers itself with Jasmines existing GlobalError handler to receive these without failing the spec.

Thanks for using Jasmine!

@craigkovatch
Copy link

craigkovatch commented Dec 27, 2017

@slackersoft I think this issue should be renamed -- as @sgravrock mentioned, Error Boundary usage is not required for Jasmine to fail to catch thrown errors in React 16 components.

I'd also like to suggest this get some kind of prioritization. Right now there doesn't seem to be a built-in way to properly handle throws.

@the-spyke the-spyke changed the title Jasmine fails with React 16 Error Boundaries Jasmine fails on errors in React 16 components Dec 27, 2017
@sgravrock
Copy link
Member

sgravrock commented May 9, 2018

I spent a bit of time digging into this recently. I'm seeing some inconsistency in whether or not a global error event is triggered when React catches an exception from a component. In the example above, it is. In the examples I've set up myself, it isn't. I'm not sure if the difference is due to React versions, use of Enzyme, where in the lifecycle the exception is thrown, or something else. In the case where no error event is triggered, there's nothing that Jasmine can do because it has no way of knowing about the error.

I've had some success with using an error boundary to take control of the behavior, e.g.:

export class SpecErrorBoundary extends React.Component {
   componentDidCatch(error, info) {
      if (this.props.errorHandler) {
         this.props.errorHandler(error);
      } else {
         // Rethrow from a setTimeout so react can't catch it.
         // This way it will fail the test suite even if the error
         // might not be associated with the test that caused it.
         setTimeout(() => {
            throw error;
         });
      }
   }

   render() {
      return this.props.children;
   }
}

This can be used to write tests that expect an error to occur, that fail if an error occurs, or (if you really want to) that don't care if an error occurs:

it('throws an exception', function() {
   const errorHandler = jasmine.createSpy('errorHandler');
   mount(
      <SpecErrorBoundary errorHandler={errorHandler}>
         <Throws />
      </SpecErrorBoundary>
   );
   expect(errorHandler).toHaveBeenCalledWith(
      jasmine.objectContaining({message: 'nope!'})
   );
});

it('fails the suite if an exception is thrown', function() {
   mount(
      <SpecErrorBoundary>
         <Throws />
      </SpecErrorBoundary>
   );
});

There are a couple of major issues still: React will log the error to the console even if it's caught by an error boundary, and Enzyme will throw if you try to do anything to the wrapper after the error is caught. I don't believe there's anything Jasmine can do about those issues, and to my way of thinking they seriously limit the usefulness of any test that intentionally causes a component to throw.

To cover all the bases around React 16 errors and error boundaries, I think we need three things:

  1. A way to control whether a thrown error fails the test.
  2. A way to control whether the error is logged to the console.
  3. A way to inspect the render tree of an error boundary after it catches an error.

I don't think Jasmine itself can or should solve any of those problems. The first is best handled with individual test code, as above. The second is ultimately up to react-dom, and the third seems to be solidly on Enzyme's turf (or on the turf of whatever it is that people use instead of Enzyme).

Further discussion and new ideas are welcome but as it is I'm inclined to close this since I don't think there's anything that Jasmine can do to help the situation.

@sgravrock sgravrock added the React label May 9, 2018
@the-spyke
Copy link
Author

the-spyke commented May 14, 2018

Not sure how it works for you. I've used MyComp from CodePen, copied you error boundary and tests, and added one more test that always pass.

Code:

class SpecErrorBoundary extends React.Component {
    constructor(props) {
        super(props);

        this.state = { isError: false };
    }

    componentDidCatch(error, info) {
        if (this.props.errorHandler) {
            this.props.errorHandler(error);
        } else {
            // Rethrow from a setTimeout so react can't catch it.
            // This way it will fail the test suite even if the error
            // might not be associated with the test that caused it.
            setTimeout(() => {
                throw error;
            });
        }

        this.setState({ isError: true });
    }

    render() {
        if (this.state.isError) {
            return "Error";
        }

        return this.props.children;
    }
}

class MyComp extends React.Component {
    componentDidMount() {
        throw new Error("Ha-ha!");
    }
    render() {
        return <span>{"No error"}</span>;
    }
}

describe("React 16 issues", () => {

    it("should always pass", () => {
        expect(true).toBe(true);
    });

    it("throws an exception", () => {
        const errorHandler = jasmine.createSpy("errorHandler");

        mount(
            <SpecErrorBoundary errorHandler={errorHandler}>
                <MyComp />
            </SpecErrorBoundary>
        );

        expect(errorHandler).toHaveBeenCalledWith(
            jasmine.objectContaining({ message: "Ha-ha!" })
        );
    });

    xit("fails the suite if an exception is thrown", () => {
        mount(
            <SpecErrorBoundary>
                <MyComp />
            </SpecErrorBoundary>
        );
    });

});

Results:

START:
  React 16 issues
    ✔ should always pass
ERROR: 'The above error occurred in the <MyComp> component:
    in MyComp (at react-tests.js:81)
    in SpecErrorBoundary (created by WrapperComponent)
    in WrapperComponent

React will try to recreate this component tree from scratch using the error boundary you provided, SpecErrorBoundary.'
    ✖ throws an exception

Finished in 0.024 secs / 0.021 secs @ 11:14:18 GMT+0300 (+03)

SUMMARY:
✔ 1 test completed
ℹ 1 test skipped
✖ 1 test failed

FAILED TESTS:
    ✖ throws an exception
      Chrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

@the-spyke
Copy link
Author

There is also another weird issue with results duplication: it there're many other tests (even excluded), output will be duplicated. But maybe this is an issue with karma-jasmine...

START:
14 05 2018 11:18:42.583:INFO [karma]: Karma v1.7.1 server started at http://0.0.0.0:9876/
14 05 2018 11:18:42.583:INFO [launcher]: Launching browser ChromeHeadless with unlimited concurrency
14 05 2018 11:18:42.671:INFO [launcher]: Starting browser ChromeHeadless
14 05 2018 11:18:43.850:INFO [HeadlessChrome 66.0.3359 (Linux 0.0.0)]: Connected on socket 9xMlh1OXCqkqK0WFAAAA with id 82693832

  React 16 issues
    ✔ always pass
ERROR: 'The above error occurred in the <MyComp> component:
    in MyComp (at signallable_test.js:155)
    in SpecErrorBoundary (created by WrapperComponent)
    in WrapperComponent

React will try to recreate this component tree from scratch using the error boundary you provided, SpecErrorBoundary.'
    ✖ throws an exception
ERROR: 'The above error occurred in the <MyComp> component:
    in MyComp (at signallable_test.js:165)
    in SpecErrorBoundary (created by WrapperComponent)
    in WrapperComponent

React will try to recreate this component tree from scratch using the error boundary you provided, SpecErrorBoundary.'
    ✖ fails the suite if an exception is thrown
    ✖ throws an exception 
    ✖ fails the suite if an exception is thrown 

Finished in 0.119 secs / 0.025 secs @ 11:18:45 GMT+0300 (+03)

SUMMARY:
✔ 1 test completed
ℹ 275 tests skipped
✖ 4 tests failed

FAILED TESTS:
    ✖ throws an exception
      HeadlessChrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

    ✖ fails the suite if an exception is thrown
      HeadlessChrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

    ✖ throws an exception 
      HeadlessChrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

    ✖ fails the suite if an exception is thrown 
      HeadlessChrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

@the-spyke
Copy link
Author

Okay, with jasmine@3.1.0 I've got duplication even in simple test file:

START:
  React 16 issues
    ✔ should always pass
ERROR: 'The above error occurred in the <MyComp> component:
    in MyComp (at react-tests.js:81)
    in SpecErrorBoundary (created by WrapperComponent)
    in WrapperComponent

React will try to recreate this component tree from scratch using the error boundary you provided, SpecErrorBoundary.'
    ✖ throws an exception
    ✖ throws an exception 

Finished in 0.029 secs / 0.021 secs @ 11:34:29 GMT+0300 (+03)

SUMMARY:
✔ 1 test completed
ℹ 1 test skipped
✖ 2 tests failed

FAILED TESTS:
  React 16 issues
    ✖ throws an exception
      Chrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

    ✖ throws an exception 
      Chrome 66.0.3359 (Linux 0.0.0)
    Uncaught Error: Ha-ha! thrown

@sgravrock
Copy link
Member

I haven't been able to reproduce either the test failure or the duplication. I wonder if the duplication has something to do with the reporter that you're using. Here's what I get with ConsoleReporter, Jasmine's default reporter:

Randomized with seed 67197
Started
*.Error: Uncaught [Error: Ha-ha!]
    at reportException (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
    at invokeEventListeners (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:209:9)
    at HTMLUnknownElementImpl._dispatch (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
    at HTMLUnknownElementImpl.dispatchEvent (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
    at HTMLUnknownElementImpl.dispatchEvent (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
    at HTMLUnknownElement.dispatchEvent (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
    at Object.invokeGuardedCallbackDev (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:138:16)
    at invokeGuardedCallback (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:187:29)
    at commitRoot (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:11594:9)
    at completeRoot (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:12502:36) Error: Ha-ha!
    at MyComp.componentDidMount (/Users/work/workspace/jasmine-enzyme-update-exception-demo/src/Throws.test.js:12:15)
    at commitLifeCycles (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:9784:26)
    at commitAllLifeCycles (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:11455:9)
    at HTMLUnknownElement.callCallback (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:100:14)
    at invokeEventListeners (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:193:27)
    at HTMLUnknownElementImpl._dispatch (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
    at HTMLUnknownElementImpl.dispatchEvent (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
    at HTMLUnknownElementImpl.dispatchEvent (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
    at HTMLUnknownElement.dispatchEvent (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
    at Object.invokeGuardedCallbackDev (/Users/work/workspace/jasmine-enzyme-update-exception-demo/node_modules/react-dom/cjs/react-dom.development.js:138:16)
The above error occurred in the <MyComp> component:
    in MyComp
    in SpecErrorBoundary (created by WrapperComponent)
    in WrapperComponent

React will try to recreate this component tree from scratch using the error boundary you provided, SpecErrorBoundary.
.

Pending:

1) React 16 issues fails the suite if an exception is thrown
  Temporarily disabled with xit

3 specs, 0 failures, 1 pending spec
Finished in 0.036 seconds
Randomized with seed 67197 (jasmine --random=true --seed=67197)
✨  Done in 1.07s.

@jillesme
Copy link

We ran into this issue, we fixed it by spying on onerror

it('should handle an error correctly', () => {
  const onerror = spyOn(window, 'onerror');
  // do something that throws the error
  expect(onerror).toHaveBeenCalled();
})

@craigkovatch
Copy link

Yay! Thank you!

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

5 participants