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

Enzyme matchers do not match correctly children of React Fragments #1799

Closed
Tracked by #1553
astorije opened this issue Aug 28, 2018 · 24 comments · Fixed by #2001
Closed
Tracked by #1553

Enzyme matchers do not match correctly children of React Fragments #1799

astorije opened this issue Aug 28, 2018 · 24 comments · Fixed by #2001
Assignees

Comments

@astorije
Copy link

Describe the bug

This is a dedicated issue corresponding to a comment I initially left at #1213 (comment).

When using React Fragments, it looks like only the first child is being picked up by the matchers.

To Reproduce

Assuming the following code:

const Foobar = () => (
  <>
    <div>Foo</div>
    <div>Bar</div>
  </>
);

And the following test:

describe('<Foobar />', () => {
  test('should include Foo and Bar', () => {
    const wrapper = mountWithIntl(<Foobar />);

    expect(wrapper).toIncludeText('Foo');
    expect(wrapper).toIncludeText('Bar');
  });
});

Bar simply does not get rendered:

 FAIL  test/src/Foobar.test.jsx
  ● <Foobar /> › should include Foo and Bar

    Expected <IntlProvider> to contain "Bar" but it did not.
    Expected HTML: "Bar"
    Actual HTML: "Foo"

       8 |
       9 |     expect(wrapper).toIncludeText('Foo');
    > 10 |     expect(wrapper).toIncludeText('Bar');
         |                     ^
      11 |   });
      12 | });
      13 |

      at Object.test (test/src/Foobar.test.tsx:10:21)

mountWithIntl is a simple wrapper that adds an IntlProvider (from react-intl) to help us deal with i18n in our tests. If I remove it and replace with bare mount, I get:

  ● <Foobar /> › should include Foo and Bar

    Trying to get host node of an array

       7 |     const wrapper = mount(<Foobar />);
       8 |
    >  9 |     expect(wrapper).toIncludeText('Foo');
         |                     ^
      10 |     expect(wrapper).toIncludeText('Bar');
      11 |   });
      12 | });

I get the same results if I swap the <>...</> shorthand with <React.Fragment>...</React.Fragment>.

However, if the code is:

const Foobar = () => (
  <div>
    <div>Foo</div>
    <div>Bar</div>
  </div>
);

then now the tests pass fine.

For what it's worth, the built code for Foobar (built with tsc) looks like:

const Foobar = () => (React__default.createElement("div", null,
    React__default.createElement("div", null, "Foo"),
    React__default.createElement("div", null, "Bar")));

Expected behavior

I'm very confused as Enzyme 3.5 has Fragment support. Do you see something just obviously wrong above?

I'm wondering if the TypeScript compilation doesn't compile into something that's causing #1178. Or I'm just doing something wrong...

Additional context

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

Since toIncludeText isn't part of enzyme itself, can you try with expect(wrapper.text()).toInclude('Foo') etc? What's wrapper.text() here?

@lencioni
Copy link
Contributor

I'm also curious if reversing the order of the assertions to be

expect(wrapper).toIncludeText('Bar');
expect(wrapper).toIncludeText('Foo');

will cause the same assertion to fail, i.e. is this always failing on the second assertion or on the text of the second child?

@ljharb
Copy link
Member

ljharb commented Aug 29, 2018

That’s also a very good question. There’s some issues with statefulness, shown in #1781, that may be related, if so.

@ljharb
Copy link
Member

ljharb commented Sep 1, 2018

I'm able to reproduce this for mount only, regardless of order.

ljharb added a commit that referenced this issue Sep 1, 2018
@ljharb ljharb added the bug label Sep 1, 2018
@ljharb
Copy link
Member

ljharb commented Sep 1, 2018

I've pushed up failing test cases; the issue seems to be that the adapter's nodeToHostNode only returns the first node - even though for fragments, there's multiple. Additionally, there's no way in the tree to identify that there's a fragment.

I'm not sure what the best option is - perhaps including the fragment in the tree and flattening it only where needed? or perhaps overloading nodeToHostNode to return an array, or better, a DocumentFragment, in this case? cc @madicap

@astorije
Copy link
Author

astorije commented Sep 5, 2018

@ljharb, sorry for the delay! It seems to me that you have already identified the source of the issue, but I'll still reply here with what I can figure out in case that's useful:

Since toIncludeText isn't part of enzyme itself, can you try with expect(wrapper.text()).toInclude('Foo') etc?

Note that I am highly unexperienced with Enzyme yet, and it's likely something it misconfigured on our end, but for some reason this gives me TypeError: expect(...).toInclude is not a function at the moment. I'm very confused why :/

What's wrapper.text() here?

By adding:

console.log('>' + wrapper.text() + '<');

I get:

  console.log test/src/Foobar.test.tsx:10
    >Foo<

I'm also curious if reversing the order of the assertions to be [...] will cause the same assertion to fail, i.e. is this always failing on the second assertion or on the text of the second child?

Reversing the order of the assertions causes the Bar one to change. In other words, both assertions fail individually.


Let me know if there is anything else I can test, or if there is a workaround we can apply for now (replacing the <> fragment in the code side with <div> to keep the tests happy is not an option unfortunately).

Thanks! 🙏

@ljharb
Copy link
Member

ljharb commented Sep 5, 2018

I don't think there's a workaround at the moment.

@madicap
Copy link
Contributor

madicap commented Sep 7, 2018

@astorije I'll try to see what I can do about this over the weekend

@astorije
Copy link
Author

astorije commented Sep 7, 2018

That's great to hear, thanks @madicap!

@astorije
Copy link
Author

Hey @madicap, did you end up being able to look into this? :)

@prageeth
Copy link

I'm pretty much having the same issue, happy to help with fixing it if you haven't already done so @madicap

Additional info:

const Comp = ({ children }) => (
   <React.Fragment>
       { children }
   </React.Fragment>
);

const wrapper = mount(<Comp>Hello World</Comp>);
console.log(wrapper.text()); // returns null

Seems to be affecting text() and children() too.

@chenesan
Copy link
Contributor

@ljharb I'll look into this and see if return DocumentFragment works well.

@ljharb
Copy link
Member

ljharb commented Jan 23, 2019

I believe @madicap is still working on it; anything based on DocumentFragment would only work on mount.

@chenesan
Copy link
Contributor

Thanks for reply. Is @madicap also working on rendering array? I'd also like to help with #1149 and #1178 to make rendering array work, but not sure if her work is related to them.

@ljharb
Copy link
Member

ljharb commented Jan 23, 2019

I believe the branch they have in progress will lay the foundation required to render arrays, yes. I'd prefer to wait until that lands before exploring that direction further.

ljharb added a commit that referenced this issue Feb 4, 2019
[Fix] `mount`/`shallow`: `.text`/`.html`: handle an array of nodes properly
[enzyme-adapter-react-{16,16.1,16.2,16.3}] [New] Make nodeToHostNode handle arrays

Fixes #1799
@astorije
Copy link
Author

astorije commented Feb 28, 2019

With Enzyme v3.9.0, I am now getting no text out of .text(), but some markup out of .html() (EDIT: which doesn't include what is within the fragment). The initial case presented above outputs this:

Expected HTML: "Bar"
Actual HTML: ""

@ljharb, is there anything I'm missing here? E.g. .text() do not play well with fragments, or some extra setup is required, etc.
Thanks!

@ljharb
Copy link
Member

ljharb commented Feb 28, 2019

@astorije be sure your react adapter is updated to the latest, otherwise it won't be able to handle fragments.

@astorije
Copy link
Author

Yeah I thought about that initially, but all dependencies (including the react adapter) are up-to-date 🤷‍♂️

@ljharb
Copy link
Member

ljharb commented Feb 28, 2019

@astorije in that case, can you file a new issue, and fill out the entire issue template?

@astorije
Copy link
Author

Will do, now that I have a reproducible case.

@lancempoe
Copy link

lancempoe commented May 1, 2019

Now that we have react hooks, this is becoming more of an issue. We have a file with hooks and it has React.Fragment and we need to test more than just the first node in the fragment. Currently, hooks can only be tested with mount. The link provided above is a different problem/solution. In my ideal world, we would be able to use shallow to test hooks, at which point this bug becomes mute. The only place in our repos that we mount is for hooks. Is this something that is coming soon?

    "enzyme": "^3.9.0",
    "enzyme-adapter-react-16": "^1.11.2",
    "react": "^16.8.6",

@ljharb
Copy link
Member

ljharb commented May 1, 2019

That’s up to Facebook; there’s an open issue for the shallow renderer to support useEffect.

@lancempoe
Copy link

Since this is an issue still, can we either open this issue until it is fixed or point me to an issue I can watch for this fix?

@ljharb
Copy link
Member

ljharb commented Aug 13, 2019

See #2086 and facebook/react#16168

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

Successfully merging a pull request may close this issue.

7 participants