jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.25k stars 6.47k forks source link

Use the "expect" npm package #1679

Closed mjackson closed 7 years ago

mjackson commented 8 years ago

I'd like to donate use of the expect npm package to the Jest project. We're both working on very similar expect-style APIs, except you guys are killing it with awesome error messages and I'm not. So I wanna let you run with it :)

AFAICT there are only a few places where our APIs differ. Please correct me if I'm wrong. They are:

Overall I think this should be fairly easy from my side. Not sure what this looks like from your side though. Anything I missed?

/cc @cpojer

aaronabramov commented 8 years ago

@mjackson i think the biggest issue will be migrating expect(x).toMatchSnapshot() and we haven't quite figured it out ourselves yet :)

the issue here is that it depends on some jest global state (like current running test, filename, whether it's running in --update mode or not, etc). and that means that there has to be some pretty complicated integration with the test runner. and also whenever something doesn't match snapshot, we don't really throw an error right away. Instead, we log the failure and decide what to do with it after the test finishes.

mjackson commented 8 years ago

@dmitriiabramov Help me understand. Right now, expect doesn't have any concept of snapshotting. So it's essentially just a new feature for expect users. What is there to migrate?

phated commented 8 years ago

Coupling this to the global state of a specific runner for snapshots would make this a non-starter. I'm also confused why nothing would be thrown immediately (also seems like that would break usage in other test runners).

mjackson commented 8 years ago

@phated It's my understanding that expect(x).toMatchSnapshot depends on global state, but the other methods do not and will continue to throw immediately. Since toMatchSnapshot isn't actually something that currently exists in expect, it shouldn't adversely affect existing users.

cpojer commented 8 years ago

We won't be adding the snapshot matcher to expect from the beginning. For now it will just be jest-matchers. Snapshotting is done in its own package, jest-snapshot. We will explore ways to bring snapshotting to other runners through expect (cc @suchipi) but it isn't our immediate goal. @dmitriiabramov didn't know this was coming because I failed to communicate with him.

suchipi commented 8 years ago

I don't know how expect is architected, but in https://github.com/facebook/jest/pull/1413 I was trying to make the snapshot matcher pretty agnostic; with the proposed changes, this is how it would be used:

import {createSnapshotState, processSnapshot} from 'jest-snapshot';

const fileName = "/home/whoever/whatever.snap.js";
const snapshotName = "renders correctly 1";
const actual = {/* ... */};
const options = {updateSnapshot: true}; // or false
const snapshotState = createSnapshotState(fileName);
const {pass, expected} = processSnapshot(snapshotState, snapshotName, actual, options);

So as long as you can bring your own actual, path to the snapshot file, and whether it should update, you could implement it anywhere.

cpojer commented 7 years ago

Small update: we just added expect.extend :) We are working on making this happen!

mjackson commented 7 years ago

@cpojer woo! That was definitely one of the largest differences. Is there a branch I can try out? Or did you put it in a release already? I'll make some time soon to check out jest-matchers and see what's still needed to make this happen so we can have a clear roadmap.

cpojer commented 7 years ago

Hey @mjackson,

we are tracking this internally right now. The JS Tools team is pretty busy until the end of year with our three projects (Yarn, Jest and react-native-packager) so we may not make a ton of progress on this immediately but it's definitely one of our goals to make this work well. I'll share something more soon :)

jeffbski commented 7 years ago

@mjackson FWIW after using both the chai style .not.equal and expect .toNotEqual, I always preferred the mjackson/expect style better. I know the code is probably simpler the other way, but I prefer less dots. Here is an even worse example from chai expect('foo').to.have.length.of.at.least(2);. Painful to type (for me). I don't even like how it looks/reads either. Just my opinions of course, I'm sure there are others who like the dot style.

mjackson commented 7 years ago

I agree, Jeff. I don't like the chai style either. We're moving to the Jasmine style. So .not.toEqual instead of .not.to.equal.

See the difference? On Tue, Nov 15, 2016 at 11:21 PM Jeff Barczewski notifications@github.com wrote:

@mjackson https://github.com/mjackson FWIW after using both the chai style .not.equal and expect .toNotEqual, I always preferred the mjackson/expect style better. I know the code is probably simpler the other way, but I prefer less dots. Here is an even worse example from chai expect('foo').to.have.length.of.at.least(2);. Painful to type (for me). I don't even like how it looks/reads either. Just my opinions of course, I'm sure there are others who like the dot style.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/jest/issues/1679#issuecomment-260850514, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFqp4Xzrs2nOqdO0hFBdFlj2rsMSOo-ks5q-oTOgaJpZM4J63Xy .

Michael Jackson @mjackson

cpojer commented 7 years ago

I believe there is value in the not negator but any more dots to chain matchers is not really useful imho. I understand this is subjective but I believe the current style in Jest is clean and straightforward. We can also build simple modules that will bring back things like toNotEqual because it's implementation is basically just expect(foo).not.toEqual().

jeffbski commented 7 years ago

@mjackson yes, I agree that wouldn't be too bad if we are just adding one dot clause. @cpojer I like that idea too, I'd certainly load up that extra module to keep the old style :-) Thanks for your consideration. I know it isn't life or death decision, but it does influence the developer experience. :-)

mjackson commented 7 years ago

Absolutely, it affects the UX. Thanks for chiming in here and caring, Jeff :)

FWIW, I think that .not is the best UX for both plugin authors and users.

For plugins, authors only have to write one assertion instead of two. So, e.g. if I want to make a toFlyLikeABird assertion, I don't have to make a toNotFlyLikeABird version as well. .not gives me the negated version for free.

For users, I only have to remember one form of the assertion instead of 2.

niksajanjic commented 7 years ago

I would also like to add that I use jasmine API sometimes, like spyOn that will be covered with jest once this comes out, but there's one more example where I use jasmine:

expect(someModule.someFunction).toHaveBeenCalledWith(arg1, arg2, jasmine.any(Function));

Because we are also passing anonymous callback function as 3rd parameter we have to expect it because .toHaveBeenCalledWith function will fail if we try to expect only first 2 arguments without selecting last one. I don't mind using jasmine, just giving an example for you guys to decide if it's worth covering this with jest.

cpojer commented 7 years ago

@vjeux is helping out to make this a reality.

Here is what was done so far, among adding a ton of features to jest-matchers:

TODO

Assuming we'll figure out these things soon, are you happy to transfer ownership of the project to us?

phated commented 7 years ago

I forget if this has been brought up but can this remain es5-compatible?

cpojer commented 7 years ago

(edit): We'll likely support the syntax that is supported in node 4 and up. You can add an additional transformation step for the environment you are using.

Also, the current expect library is great and works well. If you've been happy about using it in the past and you don't want to upgrade on existing projects, I think that's fine too.

vjeux commented 7 years ago

Here's an example of the before/after

jest-matchers: image

current expect: image

suchipi commented 7 years ago

It doesn't make much difference, but my vote for spyOn would be towards jest-mock, just because "expect.spyOn" sounds kinda weird

mjackson commented 7 years ago

Awesome, thanks @vjeux! :D

expect.assert doesn't really make much sense because node comes with assert. I'd rather not add it and include it in the codemod.

expect currently works in non-node environments. Is jest-matchers currently node-only? I can understand Jest only running in node, but I don't see any reason why an assertion lib should depend on it.

cpojer commented 7 years ago

Can you provide us with an integration test? How does expect work in the browser? Do you ship a single build file of it or do you let webpack/browserify take care of the bundling? There is nothing that should prevent jest-matchers to work in the browser (assuming chalk is shimmed properly) but we haven't tested it yet.

cc @zertosh who knows how to create bundles for webpack. Can you help us get a build file of the "jest-matchers" package?

thymikee commented 7 years ago

@mjackson Jest is also running in jsdom

mjackson commented 7 years ago

Sorry, clicked the "Comment" button too soon.

Add spyOn to Jest: we are currently unsure whether it should be a separate module (jest-mock) or whether these should be on expect. I would prefer to make this a breaking change and have the codemod take care of it.

spyOn is just sugar over createSpy. The current implementation is just a few lines. I've found this API useful for doing things like

const obj = {
  func: () => {
    // ...
  }
}

const spy = expect.spyOn(obj, 'func').andCallThrough()

// call it
obj.func()

// make your assertions
expect(spy).toHaveBeenCalled()

// teardown
spy.destroy()

Assuming we'll figure out these things soon, are you happy to transfer ownership of the project to us?

Of course! :D

cpojer commented 7 years ago

Yeah, we are of course happy to support spy and spyOn but I think it is better to keep them separate inside of jest-mock as mock.fn and mock.spyOn. expect.spy and expect.fn doesn't really work that well imho and I'd prefer not to add too much to expect, it's already a lot.

mjackson commented 7 years ago

Do you ship a single build file of it or do you let webpack/browserify take care of the bundling?

I currently do both for all of my projects. UMD builds are good for things like codepens. But I also make sure everything works if you just require it using a bundler.

My current webpack config is pretty straightforward and just performs the babel transform.

mjackson commented 7 years ago

Also, I have the feeling that this is kind of like the clash of two different development philosophies. You guys may not ever need to actually run your tests in a non-node environment, which is fine. I can pitch in and do the work to make stuff work in the browser if needed. Just NEED MOAR TIME ;)

gaearon commented 7 years ago

FWIW I've been using expect in online courses on JSBin a lot.

cpojer commented 7 years ago

@gaearon you can use Jest in the browser via repl.it: https://repl.it/languages/jest

@mjackson if you can send a PR to Jest that bundles up jest-matchers, that would be great. If there are any issues for running the browser, we'll obviously fix them but I don't expect any real issues once we have a bundle. I'd like to switch the name over to expect in early January.

gaearon commented 7 years ago

@cpojer My content for Egghead includes specifically JSBin links because they are integrated, adding another third party service to the mix is going to be confusing. It doesn't matter for me (although expect@latest will likely break and I need to replace it) but just saying other people may use it in training materials too and might not want to use replit for one reason or another.

vjeux commented 7 years ago

@mjackson, @gaearon do you have examples of codebases using expect? I'm writing a codemod to convert to jest-matchers and would like to try it.

http://astexplorer.net/#/P1ejE5e6sh/1

gaearon commented 7 years ago

https://github.com/ReactTraining/react-router

mjackson commented 7 years ago

In addition to the router, you should be able to just npm install and npm test almost any of my stuff:

cpojer commented 7 years ago

Update: http://facebook.github.io/jest/blog/2017/02/21/jest-19-immersive-watch-mode-test-platform-improvements.html#expect-improvements

Done:

Todo:

I think we are ready to cut over any day now :)

mjackson commented 7 years ago

Great news! So if I clone this repo and npm link jest-matchers, should I expect it to work more or less like expect currently does? i.e. can I

import expect from 'jest-matchers'

expect(stuff).toWork()

? Just wondering because I'm a Jest newb and I wanna give it a go this week. I'd like to convert all my projects to jest-matchers first, and then go full-on jest at some point in the future when I have time to do the code mods and refactor my test suite.

cpojer commented 7 years ago

Here is the process:

When hitting errors, take a note on what doesn't work and list them here for us so that we can either make a decision not to support it (breaking change) or update the codemod. Then advise on the next steps please.

I recommend the following:

Since the current version of the expect package is kind of at the end of life, if people aren't willing to upgrade to the new version of expect, there isn't really any issue with people staying on the old version. It works and is fine.

tomitrescak commented 7 years ago

Hmm, I'm running into problems when trying to use jest-matchers in the browser, it is requiring console package which is made for node environment.

kentor commented 7 years ago

Here are some additional changes that I have come across

I think expect.restoreSpies() functionality is missing from jest

cpojer commented 7 years ago

Hey @skovhus, you are a codemod pro. Do you think you could help us out here by taking https://github.com/cpojer/js-codemod/blob/master/transforms/expect.js and putting it into jest-codemods and adding a few of the fixes that @kentor just mentioned?

skovhus commented 7 years ago

@jpojer thanks, would love to add that to Jest Codemods. On vacation this week, but will look into it next week. ; )

AndersDJohnson commented 7 years ago

FYI spyOn seems to have been added with #2537.

skovhus commented 7 years ago

Happy to say that the transformer for this in Jest-Codemods is almost there (see progress in https://github.com/skovhus/jest-codemods/pull/39). Think it will get most projects 95% of the way when transitioning from expect@1 to Jest/Jest-matchers.

Trying it out on a few ReactTraining repos, shows that we got some ES2015 in jest-matchers. This breaks the tests as jest-matchers is used in older browser. See https://github.com/facebook/jest/issues/3360

skovhus commented 7 years ago

@cpojer @SimenB any recommendation how expect.restoreSpies() should be code modded?

(related to https://github.com/facebook/jest/issues/2965)

cpojer commented 7 years ago

@skovhus if it isn't supported right now, we should add that API to jest-mock. Happy to accept PRs.

SimenB commented 7 years ago

I think resetAllMocks is what you're after. It restores stuff that's been called with jest.spyOn. Or do you think it's too broad (since it takes all spies/mocks that ever were)?

SimenB commented 7 years ago

PR for it: #4345

ghost commented 6 years ago

@gaearon You can load the new Expect library (as maintained by facebook/jest) using the awesome service unpkg. Just use the URL https://unpkg.com/expect@latest/build-es5/index.js, which resolves to the latest version 22.1.0 (as of 23.01.2018). Hope that helps! :smile:

SimenB commented 6 years ago

jest-mock is also available in a build-es5 directory if you need that

ghost commented 6 years ago

@gaearon Proof of concept of running jest/expect inside JS Bin: jsbin/nucimeceve.

valera-rozuvan commented 6 years ago

I decided to wrap the expect package from facebook/jest as a standalone UMD module. It is located at jest-expect-standalone.

Include jest-expect-standalone as a script tag, and you will have the library available via window.expect.

<script src="https://unpkg.com/jest-expect-standalone@latest/dist/expect.min.js"></script>

See sample JS Bin jsbin/wapokahaxe.

@gaearon Heads up!