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

jwang1919 · 2019-03-11T12:35:39Z

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 · 2019-03-14T01:18:45Z

@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 · 2019-03-14T01:31:28Z

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

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

?

jwang1919 · 2019-03-14T02:22:15Z

@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!

src/logic/__tests__/unlinkGithubIdentityTest.js

src/logic/index.js

yarn.lock

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.

package.json

…gic. Still need to get react-saga and react-logic to play nicely.

These libraries are set up in code that runs at module load time, which is probably not a great practice, but for now we just mock out the libraries themselves so our modules will load without error.

jwang1919 · 2019-03-17T21:43:34Z

@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 · 2019-03-17T23:19:48Z

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 · 2019-03-17T23:26:56Z

@jwang1919 i got u

outoftime · 2019-03-17T23:31:58Z

@jwang1919 0f4523b

jwang1919 · 2019-03-19T02:05:41Z

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

outoftime · 2019-03-19T23:33:37Z

@jwang1919 checkin it out!!

outoftime

woop woop!

jwang1919 force-pushed the jest branch from d2d41fc to 5e79aee Compare March 14, 2019 00:45

outoftime reviewed Mar 14, 2019

View reviewed changes

outoftime requested changes Mar 14, 2019

View reviewed changes

src/logic/__tests__/unlinkGithubIdentityTest.js Outdated Show resolved Hide resolved

src/logic/__tests__/unlinkGithubIdentityTest.js Outdated Show resolved Hide resolved

src/logic/index.js Outdated Show resolved Hide resolved

yarn.lock Outdated Show resolved Hide resolved

jwang1919 changed the title ~~Attempt at using jest tests~~ Transition from redux-saga to redux-logic and using Jest to test - Part One Mar 15, 2019

jwang1919 force-pushed the jest branch from 155a701 to dfdce16 Compare March 15, 2019 00:55

outoftime requested changes Mar 15, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

outoftime and others added 16 commits March 17, 2019 17:31

CHECKPOINT jest

5968455

Fix unnecessary addition of an eslint-disable directive

01116f1

Initial transition to react-logic. Moved over unlickGithubIdentity lo…

2214081

…gic. Still need to get react-saga and react-logic to play nicely.

Got redux-saga and redux-logic working at the same time.

a3e7c65

Attempt to get jest working on IntelliJ

cd69fcc

Make jest config more consistent; transform lodash-es

1fd6c2e

Mocks for bugsnag and firebase libraries

32cc3d3

These libraries are set up in code that runs at module load time, which is probably not a great practice, but for now we just mock out the libraries themselves so our modules will load without error.

Revert unnecessary change to karma config

d515b5c

Add some jest rules to eslint config

8e6b2db

Write test for unlinkGithubIdentityTest.js

3e1babc

Small refactor

d5bce83

Fix yarn.lock with deduplicate

e9ed80c

Fix yarn.lock again?

25b6603

Refactored test, installed jest-extended (but didn't use it)

8aa9e02

Fix package.json after rebase

785dbde

Fix yarn.lock again

7d59962

jwang1919 force-pushed the jest branch from 6fb46d3 to 7d59962 Compare March 17, 2019 21:31

Fix incorrect merge with .eslintrc

41914c3

Add jest override to .eslintrc

0f4523b

Lint fixing

ca43651

outoftime approved these changes Mar 19, 2019

View reviewed changes

outoftime merged commit 7ed21f2 into popcodeorg:master Mar 19, 2019

outoftime mentioned this pull request Nov 1, 2019

Migrate from redux-saga to redux-logic #1934

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jwang1919 commented Mar 11, 2019 •

edited

Loading

jwang1919 commented Mar 14, 2019

outoftime left a comment

outoftime commented Mar 14, 2019

jwang1919 commented Mar 14, 2019 •

edited

Loading

outoftime left a comment

outoftime left a comment

jwang1919 commented Mar 17, 2019 •

edited

Loading

jwang1919 commented Mar 17, 2019

outoftime commented Mar 17, 2019

outoftime commented Mar 17, 2019

jwang1919 commented Mar 19, 2019

outoftime commented Mar 19, 2019

outoftime left a comment

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

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

Conversation

jwang1919 commented Mar 11, 2019 • edited Loading

jwang1919 commented Mar 14, 2019

outoftime left a comment

Choose a reason for hiding this comment

outoftime commented Mar 14, 2019

jwang1919 commented Mar 14, 2019 • edited Loading

outoftime left a comment

Choose a reason for hiding this comment

outoftime left a comment

Choose a reason for hiding this comment

jwang1919 commented Mar 17, 2019 • edited Loading

jwang1919 commented Mar 17, 2019

outoftime commented Mar 17, 2019

outoftime commented Mar 17, 2019

jwang1919 commented Mar 19, 2019

outoftime commented Mar 19, 2019

outoftime left a comment

Choose a reason for hiding this comment

jwang1919 commented Mar 11, 2019 •

edited

Loading

jwang1919 commented Mar 14, 2019 •

edited

Loading

jwang1919 commented Mar 17, 2019 •

edited

Loading