Jest Migration

[kwelch]

I have started a (new branch)[https://github.com/coryhouse/react-slingshot/tree/jest-migration].

The initial move was rather painless, but here are a few steps that I would see as required before merging in this change.

• Documentation Update
  • Remove unused packages
  • Add jest documents and help links
• Code Coverage - [https://facebook.github.io/jest/docs/configuration.html#configuration-options-configuration]
• Add Snapshot tests (EDIT No longer stretch, this is a simple add and very nice to use. We should absolutely add at least one.)
• Address Issues with Windows build (I think this is a path issue in the script)
• Remove extra comments that were referencing chai assertions that no longer apply (I noticed reference to deep equal)
• Create Progress Reporter (This will be addressed directly in jest, see thread on [Feature] Add Summary Flag to reduce output jestjs/jest#2703 for more details)

Please comment with any question or if I am missing anything that should be added to the list above.

[coryhouse]

Good stuff. Yep, this lists looks like it covers the bases. Thanks for tackling this! 👍

[rstudner]

Couple quick questions/curious:

1. Why Jest vs Mocha?
2. Will enzyme stick around?
3. Anyone looked at Tape?

I'm not asking because I have an expert opinion, just curious the "whys".

(and i'm thinking of using this boilerplate on a new project because it looks great and want to know a potential upcoming change before I get started hah)

[kwelch]

Few notes before my answers. This is just my opinion on the test frameworks. Just because I finish this, it may not be merged and approved at the test framework for Slingshot. My intent with this was to learn more about jest and provide a how to if anyone wanted to us it.

1. Mocha is a more popular framework, but it is simply a test runner out of the box. It requires you to select an assertion framework, and separately integrate code coverage & mocking.
   Jest on the other hand has more built in our of the box including assertions, mocking, code coverage, and snapshots. Snapshots are one that is still a mystery to me slightly on how it works/ how to implement, but I have seen benefits on certain things like ensuring consistent Component output and can be used during code review for simple comparison of function changes. Jest is used by the core react team and is intended for large codebase, but should work on a non-react codebase.
2. Yes enzyme will still be around they are not exclusive of eachothet. One of the advantages of jest is that although it does have built-in parts it allows for you to bring you own.
3. I have looked at tape after reading a few articles from Eric Elliot. It has a similar advantage to Jest of having everything packaged together. However once compared to Jest, it is easy to see that you get more out of gate with Jest.

If you are starting something new I would recommend looking at my jest-mirgration branch linked in the first comment. I ported all of the tests so it should show the major differences on how you use it. Most of the work left, it around getting docs in order and build/test process.

Additionally, I will make sure I include these questions and answers into the FAQ page on my branch.

[rstudner]

@kwelch Thanks for the excellent response. I've been ramping up on lots of the tooling lately and am just trying to educate myself. Appreciate the time you spent replying :)

[kwelch]

I am caught up in a few different projects currently and unable to finish this off, if someone want to carry it across the finish line it is appreciated.

[coryhouse]

I spent some time on this awhile back and hit a wall when I realized two roadblocks:

1. Jest is substantially slower on the demo app's suite of tests. This is to be expected due to the extra overhead built into Jest. It's optimized for large test suites.
2. The output wasn't compatible with our style of outputting everything to the same command line. I opened an issue with Jest for supporting minimal output.

[kwelch]

I am starting to free up and make look into continuing this transition. I will also look into the feedback on the minimal output.

[kwelch]

In effort to move this forward I have submitted jestjs/jest#2703 to add a summary flag to jest.

This will reduce the output to just the summary section on success. See image below.

[kwelch]

They are requesting a progress reporter instead, which I will handle but make take some time.

Does anyone have the knowledge/bandwidth to look into code coverage with Jest?

I want to move this forward however my time is evading me and I am starting up two new projects. Additionally, my knowledge of coverage is non-existent.

[nickytonline]

@kwelch, I can probably look at this this weekend.

…

On Thu, Jan 26, 2017, 9:19 AM Kyle Welch ***@***.***> wrote: They are requesting a progress reporter instead, which I will handle but make take some time. Does anyone have the knowledge/bandwidth to look into code coverage with Jest? I want to move this forward however my time is evading me and I am starting up two new projects. Additionally, my knowledge of coverage is non-existent. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#262 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy2z-1UV0hkEvGlVnxFiRtHWzxb0KaXks5rWKuJgaJpZM4KBLYM> .

[kwelch]

I just made a push, pretty simple to switch code coverage.

I would appreciate any review, it looks like it is working well. I am always nervous when things work the first time.

[nickytonline]

@kwelch nice! I'll take a peek at your branch tomorrow night.

[kwelch]

Finishing some clean up and should be pushing soon. Only items left should be progress reporter and documentation.

All updates have been pushed
My biggest area of concern are the docs. I started making edits their early, but I have removed a decent amount of packages and a few tools files.

@coryhouse,
How critical is the minimal reporter? I understand the need, but not sure if we want to slow/stop this from merging only because of the test reporter.

I will continue to do my best to keep updates on any progress made on the reporter.

[coryhouse]

@kwelch Excellent! I was going to review the branch, but looks like it's a few commits behind master. Can you pull latest so we can review an up to date PR?

Regarding the minimal, to me it's pretty important because Jest outputs so much by default that it's hard to see linting issues, but I'll have to try your branch again to see how it plays out.

[kwelch]

Rebased and pushed. 😃

I understand the need for the reporter, I am just not sure how long it will take to get it in place. Jest appears to have fixed/simplified a lot of the coverage that mocha had issues with.

[coryhouse]

Branch looks great to me! Only one suggestion: Why not simplify the test script to this?

"test": "jest",

That ran fine for me. Any reason you did the complicated line?

[nickytonline]

@kwelch, same comment as @coryhouse . I usually just run it as "test": "jest" too.

[nickytonline]

Looks good to me too, but there appear to be two errors that the linter is catching when I run npm start or npm run lint.

No tests found related to files changed since last commit.
Press `a` to run all tests, or run Jest with `--watchAll`.

Watch Usage
 › Press a to run all tests.
 › Press o to only run tests related to changed files.
 › Press p to filter by a filename regex pattern.
 › Press q to quit watch mode.
 › Press Enter to trigger a test run.
[BS] Access URLs:
 ------------------------------------
       Local: http://localhost:3000
    External: http://192.242.1.7:3000
 ------------------------------------
          UI: http://localhost:3001
 UI External: http://192.242.1.7:3001
 ------------------------------------
[BS] Serving files from: src
[BS] Watching files...
webpack: wait until bundle finished: /index.html
/home/nickytonline/dev/react-slingshot/src/store/store.spec.js (1/0)
  ✖  107:11  'lastGoodSavings' is assigned a value but never used  no-unused-vars

/home/nickytonline/dev/react-slingshot/src/utils/fuelSavingsCalculator.spec.js (1/0)
  ✖  70:15  'milesPerMonth' is assigned a value but never used  no-unused-vars

✖ 2 errors (11:29:36 PM)
webpack built 108b7b1a4bd879815256 in 4301ms
Child html-webpack-plugin for "index.html":

webpack: bundle is now VALID.

[kwelch]

I can try to switch it back. I believe at one point the build was breaking on Node v4.

Lint error is in the test, that should be a quick fix. Looks like it is also running in watch on dev server. I wonder how well that will work since it only tests based on changes since last commit, unless we specify --watchAll.

I will also start combing through the docs for references to mocha and ensure they are updated. I believe the dependency section is also out of date. If someone wants to look that over, I built a package dependency-scanner that will output all deps, it was specifically built for updating that page.

[kwelch]

I attempting to use just "test": "jest" but it fails on CI on Node 5 and below.

I have also pushed fixes for the lint issues.

[silentbobbert]

Test coverage is now working in slingshot on Windows! Great work Kyle!

[coryhouse]

@kwelch - You're awesome! Thanks so much for all your hard work on this! 💯

I'm updating the docs right now...

[coryhouse]

Doc updates are committed to the jest-migration branch.

[kwelch]

Awesome, are we ready to merge? Then put in a bug to update to new reporter when it is released?

[coryhouse]

Excellent and agreed!

[kwelch]

Closing. 🎉 Further discussion will take place in #363 or #361.