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

support error boundaries on ReactTestRenderer #7558

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

millermedeiros
Copy link
Contributor

@millermedeiros millermedeiros commented Aug 25, 2016

transaction.checkpoint() and transaction.rollback() was missing from ReactTestReconcileTransaction; so every time I tried to generate snapshots tests with jest for components that implemented unstable_handleError I would get the following error:

  TypeError: transaction.checkpoint is not a function

      at ReactCompositeComponentWrapper.performInitialMountWithErrorHandling (react/renderers/shared/stack/reconciler/ReactCompositeComponent.js:434:28)
      at ReactCompositeComponentWrapper.mountComponent (react/renderers/shared/stack/reconciler/ReactCompositeComponent.js:337:13)
      at Object.mountComponent (react/renderers/shared/stack/reconciler/ReactReconciler.js:57:29)
      at ReactTestComponent.mountChildren (react/renderers/shared/stack/reconciler/ReactMultiChild.js:275:43)
      at ReactTestComponent.mountComponent (react/renderers/testing/ReactTestRenderer.js:53:6)
      at Object.mountComponent (react/renderers/shared/stack/reconciler/ReactReconciler.js:57:29)
      at ReactTestComponent.mountChildren (react/renderers/shared/stack/reconciler/ReactMultiChild.js:275:43)
      at ReactTestComponent.mountComponent (react/renderers/testing/ReactTestRenderer.js:53:6)
      at Object.mountComponent (react/renderers/shared/stack/reconciler/ReactReconciler.js:57:29)
      at ReactTestComponent.mountChildren (react/renderers/shared/stack/reconciler/ReactMultiChild.js:275:43)
      at ReactTestComponent.mountComponent (react/renderers/testing/ReactTestRenderer.js:53:6)
      at Object.mountComponent (react/renderers/shared/stack/reconciler/ReactReconciler.js:57:29)
      at ReactCompositeComponentWrapper.performInitialMount (react/renderers/shared/stack/reconciler/ReactCompositeComponent.js:494:39)
      at ReactCompositeComponentWrapper.mountComponent (react/renderers/shared/stack/reconciler/ReactCompositeComponent.js:345:13)
      at Object.mountComponent (react/renderers/shared/stack/reconciler/ReactReconciler.js:57:29)
      at ReactCompositeComponentWrapper.performInitialMount (react/renderers/shared/stack/reconciler/ReactCompositeComponent.js:494:39)
      at ReactCompositeComponentWrapper.mountComponent (react/renderers/shared/stack/reconciler/ReactCompositeComponent.js:345:13)
      at Object.mountComponent (react/renderers/shared/stack/reconciler/ReactReconciler.js:57:29)
      at mountComponentIntoNode (react/renderers/testing/ReactTestMount.js:49:38)
      at ReactTestReconcileTransaction.perform (react/renderers/shared/utils/Transaction.js:140:12)
      at batchedMountComponentIntoNode (react/renderers/testing/ReactTestMount.js:70:23)
      at ReactDefaultBatchingStrategyTransaction.perform (react/renderers/shared/utils/Transaction.js:140:12)
      at Object.batchedUpdates (react/renderers/shared/stack/reconciler/ReactDefaultBatchingStrategy.js:65:20)
      at Object.batchedUpdates (react/renderers/shared/stack/reconciler/ReactUpdates.js:111:25)
      at Object.render (react/renderers/testing/ReactTestMount.js:151:25)

this diff fixes the error

@millermedeiros
Copy link
Contributor Author

/cc @spicyj and @cpojer

@ghost ghost added the CLA Signed label Aug 25, 2016
@sophiebits
Copy link
Collaborator

Can you make a custom componentDidMount instead of mocking putListener? This current test won't verify that the queue is reset if we were to refactor the events system and how putListener works. Also, no need for the parens within render.

@millermedeiros
Copy link
Contributor Author

millermedeiros commented Aug 25, 2016

@spicyj I basically copied this test

it('renders an error state (ssr)', function() {
class Angry extends React.Component {
render() {
throw new Error('Please, do not render me.');
}
}
class Boundary extends React.Component {
constructor(props) {
super(props);
this.state = {error: false};
}
render() {
if (!this.state.error) {
return (<div><button onClick={this.onClick}>ClickMe</button><Angry /></div>);
} else {
return (<div>Happy Birthday!</div>);
}
}
onClick() {
/* do nothing */
}
unstable_handleError() {
this.setState({error: true});
}
}
var EventPluginHub = require('EventPluginHub');
var container = document.createElement('div');
EventPluginHub.putListener = jest.fn();
container.innerHTML = ReactDOMServer.renderToString(<Boundary />);
expect(container.firstChild.innerHTML).toBe('Happy Birthday!');
expect(EventPluginHub.putListener).not.toBeCalled();
});
- but I agree that it makes sense to refactor it.

would also be good if we had some AbstractReconcileTransaction that implemented the common methods to make it easier to keep things in sync (see #6689) but since I'm not familiar with the React codebase and don't know how things might diverge in the future, I decided that it would be a premature abstraction...

Edit: I guess I was wrong about the premature abstraction, ReactNativeReconcileTransaction seems to have the same problem.

@sophiebits
Copy link
Collaborator

Okay, this seems fine then. Interested in sending a follow-up to change both tests as I described?

@sophiebits sophiebits merged commit 38f74bc into facebook:master Aug 25, 2016
@sophiebits sophiebits added this to the 15-next milestone Aug 25, 2016
@millermedeiros
Copy link
Contributor Author

created #7567 and #7569 as follow ups

@zpao zpao modified the milestones: 15.3.2, 15-next Sep 8, 2016
zpao pushed a commit that referenced this pull request Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants