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

Fix to make tests compatible with Material UI v3 and v4 #832

Merged
merged 1 commit into from
Aug 29, 2019
Merged

Fix to make tests compatible with Material UI v3 and v4 #832

merged 1 commit into from
Aug 29, 2019

Conversation

patorjk
Copy link
Collaborator

@patorjk patorjk commented Aug 14, 2019

I've made 2 small tweaks that allow the tests to work on both Material UI v3 and v4. In this PR I only change the tests and not the package.json or package-lock.json files (more on that below). Steps to test:

  1. Do everything as usual, run the tests and see that they still work properly with v3.
  2. Remove package-lock.json and ./node_modules
  3. Update package.json: Change @materual-ui/core version to "4.3.0", react to "16.9.0" and react-dom to "16.9.0".
  4. run "npm i".
  5. run "npm run test" and see that the tests also work and pass with v4.

I didn't update package.json and package-lock.json since I figure it's in the cards to do that later down the road - though I'll note that I've been using the table with MUI v4 and I haven't seen any issues, though I know there are a few small things, ex: #595 #676).

Let me know if I'm overwhelming you with PRs, I'm going to be pretty busy after this coming Friday, so this will most likely be my last one for a while.

Anyway, while doing one of my other PRs I noticed the tests didn't work in MUI v4. I decided to look into it, and it appears to be because the MUI folks are doing instanceOf checks for HTMLInputElement [1] and Element [2], which the tests look for on the global object (since the MUI code assumes they're in the global scope). Attaching them to the global object allows 24 of the failing tests to pass when run under Material v4 (and to still work under v3).

I fixed the last remaining test by allowing "ForwardRef(Paper)" to also be a valid value. I toyed around with the dive().name calls, and it looks like something just changed under the hood to make it so it became "ForwardRef(Paper)" in newer versions of the framework.

[1] https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Popover/Popover.js#L144
[2] https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/InputBase/InputBase.js#L197

@gabrielliwerant gabrielliwerant self-requested a review August 15, 2019 20:58
@gabrielliwerant gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Aug 15, 2019
@gabrielliwerant
Copy link
Collaborator

This is great! I did a bunch of digging on this a while ago and I found the exact line in material ui (alogn with the version) that was causing the trouble, but didn't get around to a final solution. Also, removing the package-lock was key to testing this, and might have been a problem for me when I was looking into it before. Pretty unfortunate that the lockflle is getting in the way of certain updates.

This is a great step in the upgrade path (which is in the cards, just wanted to fix other things first).

I'm definitely overwhelmed, but it's great to have these contributions! :D

@gabrielliwerant gabrielliwerant added enhancement lgtm and removed needs review Useful to mark PRs for what's up next to review labels Aug 28, 2019
@gabrielliwerant gabrielliwerant merged commit 6396aa1 into gregnb:master Aug 29, 2019
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.

2 participants