Transition from redux-saga to redux-logic and using Jest to test - Part One

[jwang1919]

Converted only one saga method: unlinkGithubCredentials.

Was able to get redux-saga and redux-logic working at the same time.

Did not complete the jest test because was trying to get it running correctly through IntelliJ, seems to run correctly when using yarn test.

Still very much WIP.

Currently experiencing issues where import keyword is not recognized by jest.

 Jest encountered an unexpected token
    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.
    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".
    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.
    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html
    Details:
    D:\Projects\ScriptEd\popcode\node_modules\lodash-es\get.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import baseGet from './_baseGet.js';
                                                                                             ^^^^^^
    SyntaxError: Unexpected token import
      1 | import Cookies from 'js-cookie';
    > 2 | import get from 'lodash-es/get';
        | ^
      3 | import isEmpty from 'lodash-es/isEmpty';
      4 | import isNil from 'lodash-es/isNil';
      5 | import isNull from 'lodash-es/isNull';
      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:451:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:493:19)
      at Object.<anonymous> (src/clients/firebase.js:2:1)
      at Object.<anonymous> (src/logic/unlinkGithubIdentity.js:3:1)

To reproduce the error, install jest globally and run jest unlinkGithubCredentialsTest.js.
So far tried solutions listed here:
jestjs/jest#5920
jestjs/jest#2081

[jwang1919]

@outoftime Finished test for unlinkGithubIdentityTest. I tried to be thorough with testing the return. What do you think?

[outoftime]

@jwang1919 I think the approach looks solid overall. One high-level question I have is: should we strive to have one assertion per test? Some testing frameworks strongly prefer this, others do not; I am not sure where jest falls on that. Maybe it's worth quickly doing a survey of popular projects that use jest for testing (which is basically all of them)?

Also have some more line-level feedback but will hold off on that until you're ready for it.

[outoftime]

Also I think yarn.lock is in a bad state again. Can you do:

$ git checkout master -- yarn.lock
$ yarn install
$ yarn deduplicate

?

[jwang1919]

@jwang1919 I think the approach looks solid overall. One high-level question I have is: should we strive to have one assertion per test? Some testing frameworks strongly prefer this, others do not; I am not sure where jest falls on that. Maybe it's worth quickly doing a survey of popular projects that use jest for testing (which is basically all of them)?

I'm not sure. I like to have multiple assertions in a test as long as they are all testing the same functionality. I usually like to group tests by happy paths, bad paths and edge cases. I will defer the answer to you since you have more jest experience.

EDIT: Seems like the loud majority is to aim for one assertion per test to avoid Assertion Roulette. I still think the test is fine because the method itself is to unlinkGithub. It doesn't make sense to separate it into something like: "should unlink identity" and "should unlink from provider github.com".

Also have some more line-level feedback but will hold off on that until you're ready for it.

Bring it on

[outoftime]

Lookin' good! Just a few line-level comments.

Also: can you update the PR title and description to reflect the current state of the world? Thanks!

[outoftime]

See comment on package.json—I’m also going to push a couple of changes to fix the linting failures and remove some unnecessary comments.

[jwang1919]

@outoftime After the rebase with your .eslintrc change, there are a lot more warnings/errors mostly coming from the saga files.

Oh wait nm, I think I did an incorrect merge

[jwang1919]

The issue now is that because jest is called as a global library, eslint is not happy with it.
I wasn't able to find a way to import the jest library, the closest thing I found so far is a PR that addresses this issue: jestjs/jest#7571

[outoftime]

@jwang1919 i got u

[outoftime]

@jwang1919 0f4523b

[jwang1919]

@outoftime I addressed the package.json several commits go. Is the PR merge good to go?

[outoftime]

@jwang1919 checkin it out!!

[outoftime]

woop woop!