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

[Testing] Upgrading to 1.1.0 and above causes tests to fail, Javascript heap out of memory #562

Closed
Ardhimas opened this issue Oct 3, 2017 · 14 comments
Labels

Comments

@Ardhimas
Copy link

Ardhimas commented Oct 3, 2017

Description

I upgraded react-md from 1.0.19 to 1.1.0 and ran into these issues, I just tried upgrading to 1.1.5 and still ran into the issues. I initialized the app using create-react-app and have never had any issues like this. It appears the issue is occurring in snapshot tests that I'm using enzyme to shallow render with the aid of enzyme-to-json. The issue isn't with either of the testing methods, as I tried downgrading and the issues disappeared. I've dug around and found an issue where jest can experience memory leaks when running tests with the --coverage flag but I'm experiencing the issue whether I run it with or without the flag.

I understand that this issue may be a problem with my app, and if that is the case I apologize for asking for help here. However, as the issue disappears when I downgrade back to 1.0.19 I believe it may be related to a change, though I can't seem to pinpoint the source of the problem.

If anyone has any idea where the problem could be, I would greatly appreciate any pointers.

EDIT: I believe the issue is related to this issue in Jest.

EDIT 2: I've pinpointed the issue to snapshot tests for components that contain either the Menu or SelectField components. Could this issue be related to the recent 'smart' positioning menus?

Images/Screenshots

image
image
After downgrading, the tests pass again.
image

Version

  • React - 15.6.1
  • React-MD - 1.1.5, but issue first seen in 1.1.1
@mlaursen mlaursen added the bug label Oct 3, 2017
@mlaursen
Copy link
Owner

mlaursen commented Oct 5, 2017

To help debug this, could you provide the version of jest and node you are using?

@Ardhimas
Copy link
Author

Ardhimas commented Oct 5, 2017

jest: 20.0.4
node: 8.2.1

@mlaursen
Copy link
Owner

mlaursen commented Oct 5, 2017

Thanks!

@Ardhimas
Copy link
Author

Ardhimas commented Oct 5, 2017

Thank you for the help! Let me know if there is anything else I could do to help you.

@Ardhimas
Copy link
Author

@mlaursen I spent a few hours attacking this issue today and I've come to a few definitive conclusions

  • The Layover component is the culprit causing memory issues, and it was something introduced in the 1.1.0 release.
  • The issue is reproducible reliably when running snapshot tests generated using shallow from enzyme, or the default export from react-test-renderer. Rendering any test containing the Layover component - even if it's nested within SelectField or Menu - either throws an error when attempting to render, or it passes until you add the line expect(wrapper).toMatchSnapshot() and fails after that.
  • When using react-test-renderer, the issue appears to come from within the _setContainer method of the Layover component, and I'm thinking it may be to do with a null argument.

The solution I came up with is using render from enzyme instead of shallow for snapshot tests, which for some reason works. The only downside is that if you have any wrapped child components, you have to make sure those can render as well, for example when using Link from react-router-dom you need to add context using MemoryRouter.

I'm going ahead with my solution for now, but the actual issue with Layover may manifest itself in a different way in the future. If it does, feel free to contact me and I may be able to help.

@mlaursen
Copy link
Owner

Thanks for looking into this so deeply @Ardhimas. I haven't really been able to debug it much since it wasn't happening for me. I'm curious if it has to do anything with this bug for snapshot testing. The Layover component defaults to setting the fixedTo: window which could be causing problems. It might work with mount because it does a full render? No idea.

If you have a test case that is guaranteed to fail each time, I would like to see it so I can debug a bit more. I think it would also be helpful to let me know what version of React and enzyme you are using.

@Ardhimas
Copy link
Author

@mlaursen [https://github.com/Ardhimas/react-sandbox](I whipped up a quick app to show you the issue.)

Go ahead and run npm install and then npm test and you should see an error like this:
image

@Skysplit
Copy link
Contributor

Skysplit commented Oct 20, 2017

@Ardhimas @mlaursen Most probably this bug is caused in combination with jest-environment-jsdom. Because by default fixedTo provides full window object, generating snapshot fails due to its size (component serialization to snapshot fails miserably).

@mlaursen
Copy link
Owner

It appears so. Setting the fixedTo prop in the Menu to {} allows the tests to work as expected. Ideally the bug I posted above will get fixed soon, but I can always update the default props for fixedTo to be typeof window !== 'undefined' && (typeof global === 'undefined' || window !== global) ? window : {} which would solve it for now.

@Ardhimas
Copy link
Author

I think updating the defaultProps is a pretty nice and succinct solution, even if it doesn't tackle the core problem at hand.

@mlaursen mlaursen modified the milestone: v1.2.4 Oct 20, 2017
@mlaursen
Copy link
Owner

Nevermind, doesn't look like that will work either. global === window is still true in the browser so layovers stop working. Might just have to wait :/

@Skysplit
Copy link
Contributor

It seems that update is on its way
jestjs/jest#4750

@Ardhimas
Copy link
Author

Awesome. It just got merged 18 minutes ago, just gotta wait for the next Jest release.

@mlaursen
Copy link
Owner

Closing this due to inactivity so I'm guessing this is no longer a problem? I'll re-open it and target to v2 if someone is still running into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants