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

Upgrading from enzyme-adapter-react-16@1.15.1 -> 1.15.2 breaks connected component tests #2390

Open
2 of 13 tasks
ryanvanbelkum opened this issue Apr 27, 2020 · 15 comments
Open
2 of 13 tasks

Comments

@ryanvanbelkum
Copy link

ryanvanbelkum commented Apr 27, 2020

Current behavior

When I upgrade from 1.15.1 of enzyme-adapter-react-16 to version 1.15.2, I am running into JSX comparison test failures with connected components.

Ex.

  const wrapper = shallow(<SomeComponent />).get(0);
  expect(wrapper).toEqualJSX(
    <ConnectedRouter history={history}>
          ...childrenHere
    </ConnectedRouter>
  )

// FAILURE
- <ConnectFunction
+ <Connect(ConnectedRouterWithContext)

Note: toEqualJSX -> https://www.npmjs.com/package/jasmine-expect-jsx is a simple wrapper around react-element-to-jsx-string -> https://github.com/algolia/react-element-to-jsx-string and turns a react element into a JSX string.

Expected behavior

Expect tests to still pass without change after bumping minor version

Your environment

API

  • shallow
  • mount
  • render

Version

library version
enzyme 3.11.0
react 16.13.1
react-dom 16.13.1
react-test-renderer 16.13.1
adapter (below) 1.15.2

Adapter

  • enzyme-adapter-react-16
  • enzyme-adapter-react-16.3
  • enzyme-adapter-react-16.2
  • enzyme-adapter-react-16.1
  • enzyme-adapter-react-15
  • enzyme-adapter-react-15.4
  • enzyme-adapter-react-14
  • enzyme-adapter-react-13
  • enzyme-adapter-react-helper
  • others ( )
@bdwain
Copy link
Contributor

bdwain commented May 6, 2020

@ljharb i dug into this a bit more and narrowed it down to #2300, specifically this commit, which fixes #2297

I don't totally understand why react change the way they do things, but the change to enzyme's own isMemo seems to be causing this issue. It seems like the component is not typeof(Memo) but does have the $$typeof for a memoized component.

I'll try to get a codesandbox repro up and dig into what is going wrong more.

@ljharb
Copy link
Member

ljharb commented May 7, 2020

@bdwain thanks, appreciate that

@bdwain
Copy link
Contributor

bdwain commented May 8, 2020

@ljharb here's a code sandbox (open the console) https://codesandbox.io/s/quirky-sun-rpwd3.

Notice that the displayname of the rendered element from the shallow wrapper does not match the display name of the actual element. because it took the inner "type" when calculating it in enzyme.

I'll keep looking at this, but please let me know if something stands out.

@bdwain
Copy link
Contributor

bdwain commented May 8, 2020

notice in the sandbox that isMemo is false on wrapper.get(0) but true on the plain element. it seems like isMemo returns true on an element normally, but not if it's rendered in the shallow renderer (or maybe mount too). If we can adress that issue, i bet we could go back to using the published isMemo functions.

@bdwain
Copy link
Contributor

bdwain commented May 8, 2020

i think i found the issue actually @ljharb

There we get the output from the renderer (which i ran through react's isMemo and it returned true) and we just throw it away and lose the type info that react gave it that their new isMemo and isLazy checks are based on.

Is the point of that to give a consistent interface between different react versions? It seems we need some way of maintaining the object's type while creating an object with a consistent interface. does that sound like i'm on the right track?

@bdwain
Copy link
Contributor

bdwain commented Jun 26, 2020

Hey @ljharb were you ever able to look at this? It'd be great to get this fixed. I'm happy to help I just want to make sure I'm on the right path before I go too far, and to see if anything jumps out at you given what I found so far.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2020

Sorry, i haven’t had time to look at it yet.

If you can provide a test case in a PR, with or without a fix, i can get it reviewed and merged and released ASAP :-)

bdwain added a commit to bdwain/enzyme that referenced this issue Jun 26, 2020
bdwain added a commit to bdwain/enzyme that referenced this issue Jun 26, 2020
@bdwain
Copy link
Contributor

bdwain commented Jun 26, 2020

Thanks @ljharb. I made #2408 and I think that covers it though it was hard to find the right spot for the unit test.

@ljharb
Copy link
Member

ljharb commented Dec 20, 2020

@bdwain your failing test is helpful, but isMemo(rendererOutput) is returning false for me (update: isMemo(output.type) is passing, however)

bdwain added a commit to bdwain/enzyme that referenced this issue May 9, 2021
@bdwain
Copy link
Contributor

bdwain commented May 9, 2021

@ljharb not really sure what you mean. It's returning false for me too, but that's why the test is failing. It should be returning true. Is there something else you need from me to get this fixed?

@ljharb
Copy link
Member

ljharb commented May 9, 2021

@bdwain you said:

There we get the output from the renderer (which i ran through react's isMemo and it returned true)

but react’s isMemo returned false for that for me.

@ljharb
Copy link
Member

ljharb commented May 9, 2021

So the bug, in other words, seems like it’s in react-is, if that is supposed to return true?

@bdwain
Copy link
Contributor

bdwain commented May 10, 2021

you ran isMemo on the output from the shallow renderer? const output = renderer.getRenderOutput(); isMemo(output) ?

It was a year ago but I can try to redo my test. I feel like at the time though, I understood the issue to be that the renderer output has some type information on it, but a node is being manually created at

and that throws away the type info that react-is relies on.

@ljharb
Copy link
Member

ljharb commented May 10, 2021

hmm, ok - if that's the case then that seems like an easy fix to make. want to make a PR? :-)

@bdwain
Copy link
Contributor

bdwain commented May 10, 2021

well i wasn't sure what the fix was exactly.

There we get the output from the renderer (which i ran through react's isMemo and it returned true) and we just throw it away and lose the type info that react gave it that their new isMemo and isLazy checks are based on.

Is the point of that to give a consistent interface between different react versions? It seems we need some way of maintaining the object's type while creating an object with a consistent interface. does that sound like i'm on the right track?

it seemed like we threw away the type info on purpose, to make a consistent interface?

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