enzymejs / enzyme-matchers

Jasmine/Jest assertions for enzyme
MIT License
892 stars 116 forks source link

Update toContainMatchingElement to return explicit booleans #282

Closed shawyu closed 5 years ago

shawyu commented 5 years ago

jest is throwing an error on the .toContainMatchingElement() matcher (see output below). The easiest fix seems to stop using truthy return values for pass and using an explicit > 0 operator to return boolean true/false rather than 0 or something >0.

Possibly related to #276.

I've created a minimal example repository to reproduce this issue: https://github.com/shawyu/enzyme-matchers-example

which produces this output:

$ npm test

> enzyme-matchers-example@1.0.0 test /Users/shawyu/external/enzyme-matchers-example
> jest example.test.js

 FAIL  ./example.test.js
  failing test
    ✕ runs (16ms)

  ● failing test › runs

    Unexpected return from a matcher function.
    Matcher functions should return an object in the following format:
      {message?: string | function, pass: boolean}
    '{"contextualInformation": {"actual": "HTML Output of <div>:
     <div><ul><li>Item</li></ul></div>"}, "message": [Function message], "negatedMessage": "Expected <div> to not contain an element matching ul but it did.", "pass": 1}' was returned

      17 |     const wrapper = shallow(<List />);
      18 |
    > 19 |     expect(wrapper).toContainMatchingElement('ul');
         |                     ^
      20 |   });
      21 | });
      22 |

      at _validateResult (node_modules/expect/build/index.js:364:11)
      at processResult (node_modules/expect/build/index.js:261:7)
      at Object.throwingMatcher [as toContainMatchingElement] (node_modules/expect/build/index.js:331:16)
      at Object.toContainMatchingElement (example.test.js:19:21)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.163s
Ran all test suites matching /example.test.js/i.
npm ERR! Test failed.  See above for more details.
blainekasten commented 5 years ago

@shawyu good research! Would you mind taking a stab at fixing it? The matcher is located at packages/enzyme-matchers/src/assertions and you can run the jest tests in packages/jest-enzyme with yarn test

shawyu commented 5 years ago

hey @blainekasten thanks for taking a look! I think the change in 9159433 is all that is needed for a fix, unless there is something else I should submit? I'm not sure how to write a unit test for this inside this repository since the problem seems to lie on the boundary of jest and enzyme-matchers. I could update assertions in the unit tests to assert on toBe(true) rather than toBeTruthy (and similar for falsy)

blainekasten commented 5 years ago

lololol. I'm so sorry. I thought this was an issue.

I think that is actually smart to change the tests to check for literal true/false. Could you do that for all the assertions and make sure they all pass? Thanks so much for the contribution!

shawyu commented 5 years ago

done! the changes also turned up a bad assertion that was passing correctly, but for the wrong reasons

blainekasten commented 5 years ago

Awesome! I'm happy to merge this if you can squash it down to 1 commit!

shawyu commented 5 years ago

I rewrote the squashed commit once on my fork and it had a PR reference in the title so the history above this comment is confusing, but there is actually only one commit to merge now (see the Commits tab)

blainekasten commented 5 years ago

Awesome! Thanks @shawyu, do you want me to cut a release immediately? Is this bug blocking you?

shawyu commented 5 years ago

@blainekasten that would be really helpful if you could cut one! I could work around the issue for now since I can use other assertion patterns so not a huge blocker, though.

blainekasten commented 5 years ago

Sorry for the delay! Was a crazy busy week. Just published 7.0.1