Add: 'expect.js' to Jest expect codeshift #67
Conversation
jordalgo
force-pushed
the
expectJsTransformer
branch
from
6d28231 to
af0c932
Compare
src/transformers/expect-js.test.js
Outdated
| ` | ||
| const test = require("testlib"); | ||
| test(t => { | ||
| expect(stuff).toExist(); |
There was a problem hiding this comment.
It seems expect.js doesn't have toExist, so for this test to be more valuable it would be nice to change this to to.be.ok(); (that expect.js) supports
src/transformers/expect-js.test.js
Outdated
| test(() => { | ||
| expect(stuff).toBeTruthy(); | ||
| expect(stuff).toBeFalsy(); | ||
| expect(stuff).toBeFalsy(); |
There was a problem hiding this comment.
empty: asserts that an array is empty or not
So expect(stuff).to.be.empty(); should be translated to expect(stuff).toHaveLength(0).
src/transformers/expect-js.test.js
Outdated
| `, | ||
| ` | ||
| test(() => { | ||
| fail('message'); |
There was a problem hiding this comment.
This doesn't look like valid Jest code. I think this should be converted to:
test(done => {
done.fail('message');
})See the ava transformer that does something similar
There was a problem hiding this comment.
Even though it's not in the docs, I believe this is a Jasmine convention for explicit sync test failures which maps to expect.js expect().fail('message'); Is there a reason the done is needed?
There was a problem hiding this comment.
Jest is using their own version of Jasmine now, so please investigate what works.
As my code example shows done is passed in as a parameter to the test.
There was a problem hiding this comment.
I tested and fail does work as expected and I think it's supported here but perhaps the best thing to do is just throw a new Error instead of using this.
There was a problem hiding this comment.
I might actually prefer throw Error(msg) since fail isn't in the Jest docs -- do you have a preference?
|
|
||
| No support yet for the follow expect.js conventions: | ||
|
|
||
| expect({ a: 'b', c: 'd' }).to.only.have.keys(['a', 'c']); |
There was a problem hiding this comment.
Please log a warning if this is used. See some of the other transformers where we also log an error if you are using unsupported features. And add a test. : )
And if you want to implement it, the chai-should does something similar.
There was a problem hiding this comment.
Sounds good. I'll check that out.
src/transformers/expect-js.test.js
Outdated
| testChanged( | ||
| 'standaloneMode: keeps expect import', | ||
| ` | ||
| import exp from 'expect'; |
There was a problem hiding this comment.
I guess this should be import exp from 'expect.js'; that is converted to import expect from 'expect';
|
@skovhus Thanks so much for the fast and great review! Will get those changes in shortly. |
jordalgo
force-pushed
the
expectJsTransformer
branch
from
af0c932 to
ec69a2f
Compare
|
@skovhus - I believe I addressed all your comments in the latest update. |
|
Actually, hold off on merging -- I need to fix one more thing. |
jordalgo
force-pushed
the
expectJsTransformer
branch
from
ec69a2f to
c78b326
Compare
|
Ok should be good to go now. Added a few more test cases for |
jordalgo
force-pushed
the
expectJsTransformer
branch
from
c78b326 to
861503c
Compare
|
@skovhus Done 👍 |
|
Awesome! Small linting (I'll add a pre commit hook one day) issue, else good to go. |
jordalgo
force-pushed
the
expectJsTransformer
branch
from
861503c to
e71cd18
Compare
|
Oops. Fixed. |
|
@jordalgo Thank you for the beautiful contribution! https://github.com/skovhus/jest-codemods/releases/tag/0.12.0 🎉 |
|
Out of curiosity, did you use this at Spotify? : ) |
|
@skovhus Awesome! Thanks for the fast feedback. And yes, this codemod got used a lot this past week at Spotify as we're migrating the unit tests in one of our big repos from Mocha (and expect.js) to Jest -- know any good codemods for sinon to Jest mocks 😜 ? |
https://github.com/Automattic/expect.js