Refactor any mocha/ava tests to use Jest

darahayes commented 4 years ago

Background

Each package in /packages has a test command in package.json. Over time our team tried different testing frameworks in different packages but now we are starting to standardise around using Jest.

It would be really beneficial if we could use the one test runner across all packages. For anybody interested in picking up this issue I suggest that the tasks below are worked on in separate Pull Requests.

[x] (Done in #198) offix-cache is currently using mocha. This previous commit shows how we migrated from mocha to jest in offix-offline which had the exact same setup. Migration should be really simple.
[x] (Done in #194) offix-conflicts-server is using ava. It shouldn't be too hard to also migrate that.
[x] (Done in #209) offix-client is using jest for its unit tests but it uses a combination of karma and mocha to run the integration-test command. Karma is a tool that lets us run our tests in a headless browser but the test files themselves are regular mocha. There is a karma config file in offix-client that specifies mocha. I would say the migration will be simple enough and will require a small bit of research in the karma docs to find how to use Karma + Jest.

dewstend commented 4 years ago

Hi! I would like to try my hand at these.

wtrocki commented 4 years ago

@dewstend Offix server was migrated already but if you have alternative PR feel free to create it.

darahayes commented 4 years ago

Hey @dewstend the first two items in the list have been done but there's still one more if you're interested. We're here to help if you have any questions!

dewstend commented 4 years ago

I investigated a bit, since Jest runs in JSDOM, it is not a trivial thing to run it in the browser, as briefly stated here. There's the issue facebook/jest#848, still open, for that matter.

I've seen a few community libraries that try to achieve Jest in headless, but nothing beyond obscure documentation. Any suggestions?

darahayes commented 4 years ago

@dewstend thanks for your investigation! So we've actually struggled a lot with those karma tests in the past. They're really hard to debug, the source maps support is kinda bad and the turnaround time to make edits and retest them are pretty bad.

Would you be interested in migrating those tests to use jest directly and we could simply remove karma? We already did some slight refactoring recently that should make this a bit easier to do.

I understand the downside here is that we wouldn't be testing in a real browser, but honestly the workflow and experience of using karma has been so poor that we probably would have written many more tests without it 😅

dewstend commented 4 years ago

Is there documentation for the integration tests? I tried analyzing the code but it's quite sizeable, this migration seems like an issue on its own, also considering they are in JavaScript, maybe it would be preferable to standardize TypeScript and possibly using the same /test/ folder for both tests, and filename conventions for Jest pattern matching like so:

└───test
    ├───int
    │       File.int.test.ts
    │
    └───unit
            File.unit.test.ts

I investigated further and found out you the project runs Chrome Headless on Karma, Puppeteer (Headless Chrome Node.js API) can be implemented with Jest and there's documentation on it. This could be separated into tasks to achieve a full, better implementation of the current integration tests.

wtrocki commented 4 years ago

Technically there will be two elements that are browser specific:

NetworkState (Mocked in integration tests
Indexeddb (Also mocked)

So running tests on node should not be a problem at all. I would recommend to simply create PR that will change test runner to jest and then let team to resolve rest of the issues - if there are some

darahayes commented 4 years ago

@dewstend thanks a lot for doing more investigation. @wtrocki made a strong point that in the current 'integration' tests, the NetworkState and IndexedDb interfaces are actually mocked. To us, running the tests in their current form in an actual browser environment adds little to no value. They are also harder to debug and they are written in JavaScript while the rest of the project is TypeScript.

I think ideally for now we would like to.

move all those tests over to the test folder to sit alongside the existing unit tests.
convert them to TypeScript and change the chai assertions to use Jest's expect.
have all tests being run by the test command.

I think there is still a lot value in creating a new issue to discuss/investigate using Jest + Puppeteer for a new set of integration tests that has very little mocking and covers some more advanced use cases. What do you folks think?

wtrocki commented 4 years ago

In proposed form this will be task for maintainer. I think we might just create couple smaller tasks for this with hacktoberfest label or do this ourselves

aerogear / offix

Refactor any mocha/ava tests to use Jest #192

Background