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

Use jest-matchers for assertions instead of expect #61

Closed
wants to merge 28 commits into from

Conversation

skovhus
Copy link

@skovhus skovhus commented Apr 24, 2017

Currently expect@1.x is being integrated into Jest (see more background in jestjs/jest#1679).

In jest-codemods I'm working on a codemod to automate the process of going from mjackson/expect to jest-matchers (if running in a browser) or plain Jest (if running in node). 🤖

I've tested the codemod on this project and everything should still work.

Let me know what you think.

Blocked: currently fails in older browsers 💣

Seems like jest-matchers have untranspiled code SyntaxError: Unexpected keyword 'const'. Const declarations are not supported in strict mode..

Will look into a solution for this.

VinSpee and others added 20 commits February 27, 2017 21:54
Add test case to check for matching `queries` prop
- changes proptypes to make query optional and to accept the `queries`
prop
- creates a list of MatchMedia objects for the `queries` prop
- update the `match` property of state to contain an object in the
format of
  ```
    {
      [mq name: string]: [matches: bool],
    }
  ```
Use JSX syntax highlighting for JSX in readme
This should ensure backwards compatibility
Previously, mediaQueryList could have multiple types. Now, it's always
an array.
Allow for specifying defaultMatches
`mediaQueryList` was not actually a `MediaQueryList`, so name it
appropriately.
`mm` _is_ an acutal `MediaQueryList`, so name it appropriately.
@mjackson
Copy link
Member

Thanks, @skovhus! I have a LOT of code that uses expect that will benefit from this codemod if we can get it to work seamlessly.

@skovhus
Copy link
Author

skovhus commented Apr 25, 2017

I kept the running in a browser. Let me know if you just want to run these tests in node and with JS Dom. That will remove a lot of karma boilerplate. But might also lower your confidence that this code works in IE.

package.json Outdated
@@ -38,7 +38,7 @@
"eslint-plugin-react": "^6.0.0",
"gzip-size": "^3.0.0",
"in-publish": "^2.0.0",
"jest-matchers": "^19.0.0",
"jest-matchers": "19.2.0-alpha.993e64af",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -133,7 +133,7 @@ describe('A <Media>', () => {
)

render(element, node, () => {
expect(node.firstChild.innerHTML).toNotMatch(/hello/)
expect(node.firstChild.innerHTML).not.toBeDefined()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, should be .not.toMatch(/hello/). Or is it on purpose?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw your comment.

toMatch can also be on objects, so cannot be translated 1:1 with Jest toMatch... See mjackson/expect#tomatch (maybe toMatchObject?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks funky. Thanks for noticing it. Will have a look... I need to get back into the code I was doing in April. 🙀

Unchecked this point in my TODO:
skovhus/jest-codemods#39

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB If I remember correctly this might be a manual change I did, as node.firstChild.innerHTML is not defined AND Jest .not.toMatch(/hello/) fails in that case. Not sure if that should be considered a bug in Jest.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB

Error: expect(string)[.not].toMatch(expected)
  
        string value must be a string.
        Received: undefined

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... Not sure if it's a bug or not.

You can do toBeUndefined() instead of .not.toBeDefined(), btw.

Green build though, woo!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, been waiting some months to see that pretty green ✅

Yay!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You always change my matchers into something nicer. :)

@mjackson
Copy link
Member

Thanks for the PR, @skovhus. I chose to convert everything to using Jest in 20fcf76, so I think we can close this. We were able to ditch a lot of Karma boilerplate, as well as a flaky BrowserStack integration, which was a huge plus. I'm just stubbing out the window.matchMedia API, so we're not going to catch different browser implementations of that API. But browsers have a fairly consistent implementation of that spec, so I'm not too concerned about that.

@mjackson mjackson closed this Jul 13, 2017
@skovhus skovhus deleted the use-jest branch July 16, 2017 08:08
@skovhus
Copy link
Author

skovhus commented Jul 16, 2017

@mjackson sounds good :)

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

Successfully merging this pull request may close these issues.

7 participants