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

refactor: jest #215

Merged
merged 3 commits into from
Apr 30, 2020
Merged

refactor: jest #215

merged 3 commits into from
Apr 30, 2020

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Apr 27, 2020

This migrates from ava to jest. The PR keeps the existing ava tests/assertions exactly as they are, but as follow-ups, we can:

  • use jest.fn() to make logging assertions (and drop sinon as a dependency)
  • use inline snapshot testing
  • stop compiling the tests - ts-jest does this in-memory. This can simplify our tsconfig build output from lib/src/index.js to lib/index.js etc.
  • add vscode debugger configs (or document recommended configs)
  • use codemods to migrate legacy-tests from mocha and into typescript (used in this PR to convert from ava)

@mmkal mmkal requested a review from papb April 27, 2020 13:29
papb
papb previously approved these changes Apr 30, 2020
Copy link
Member

@papb papb left a comment

Choose a reason for hiding this comment

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

Awesome. I like how the amount of changes was quite small. Nice work!

@papb
Copy link
Member

papb commented Apr 30, 2020

I am in favor of all follow-ups you mentioned!!

P.S.: I will try to look at your PRs faster now. Sorry about that!

@papb papb self-requested a review April 30, 2020 14:14
@papb papb dismissed their stale review April 30, 2020 14:15

Dismissing review after master update

Copy link
Member

@papb papb left a comment

Choose a reason for hiding this comment

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

I had just approved your PR, but I decided to merge #208 first, otherwise I would have to ask the contributor to refactor his test once again, and I got worried this might make him upset. So instead I would like to ask you to reapply your codemod to convert his test to jest as well. I hope you don't mind :)

@papb
Copy link
Member

papb commented Apr 30, 2020

(by the way, he added a serial test, which is not ideal but I decided to accept anyway because a parallel test would require usage of another file for json storage, and I didn't want to add this extra "cognitive load" to that PR. If you feel like refactoring in order to make it parallel, please do, but it can wait to be done in the future as well)

@papb
Copy link
Member

papb commented Apr 30, 2020

Does jest run tests serially or in parallel? Since Umzug uses ./umzug.json by default, testing in parallel might give unexpected results due to reuse of the same file. I like the fact that ava runs tests in parallel, I think it's a good way to ensure all tests are independent, how does jest work in this case?

@mmkal
Copy link
Contributor Author

mmkal commented Apr 30, 2020

Jest runs test files in parallel, but individual tests within the file serially: jestjs/jest#6957

To run tests within a file in parallel, there's test.concurrent, but it isn't widely used.

@papb
Copy link
Member

papb commented Apr 30, 2020

Ready to merge 🚀

@mmkal mmkal merged commit cf32c6e into master Apr 30, 2020
@mmkal mmkal deleted the jest branch April 30, 2020 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants