Completed redux-logic transition for linkGithubIdentity code by jwang1919 · Pull Request #1672 · popcodeorg/popcode

jwang1919 · 2019-03-25T01:12:12Z

Followed same code conventions of when I did unlinkGithubIdentity. Replicated the same tests but in Jest and added a few more assertions.

…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.

# Conflicts: # src/logic/index.js # src/sagas/user.js # test/unit/sagas/user.js

outoftime

@jwang1919 Nice work! Left some feedback, but it’s all either minor style nits or musings on my vision for the new test suite that you’d have had no way to anticipate : )

outoftime · 2019-03-25T18:11:36Z

src/logic/__tests__/linkGithubIdentityTest.js

+
+describe('linkGithubIdentity', () => {
+  test('success', async() => {
+    const mockCredential = {providerId: 'github.com'};


One thing that is really important to me as we build out this new test suite is that test data is always generated from a centralized, reusable place, rather than defined ad-hoc in test files. This was an ambition for the old test suite but it was not fully realized.

The specific approach I had in mind was this:

Create a __factories__ directory at the root of the project. This would have different submodules for different categories of test data:

__factories__/clients for data that comes from clients (such as mockCredential here, which comes from Firebase)

__factories__/state for data that comes from the Redux store

Maybe more categories will spring up

Use a factory definition library, probably Rosie, to define the factories (N.B. I think we want unregistered factories)

Use Jest’s moduleNameMapper configuration to map @factories to <rootDir>/__factories__ so we can do e.g. import {credential as createCredential} from '@factories/clients/firebase' without worrying about a bunch of ../.. nonsense

What do you think? Are you down to take this on? I was hoping to get some of the scaffolding set up myself so you wouldn’t have to deal with it, but you beat me to the first case of needing it : )

Hmm... okay, that sounds okay with me. Never worked with Rosie before so will take a bit more time to complete this.

I actually ended up running into a situation where I needed factories too… stay tuned, currently wrangling eslint 😭

outoftime · 2019-03-25T18:14:13Z

src/logic/__tests__/linkGithubIdentityTest.js

+    const mockCredential = {providerId: 'github.com'};
+
+    linkGithub.mockResolvedValue({
+      user: {},


A corollary to the above is that I think all of our test data should be realistic, even if realism doesn’t seem important to the test as written. So I would propose using a factory here to generate a realistic Firebase user object as well.

To answer your question the Firebase API docs are pretty good for figuring out the shape of a Firebase User; and again throwing a debugger or console.log() in the application code is a good way to see what the object actually looks like in a concrete instance.

src/logic/__tests__/linkGithubIdentityTest.js

outoftime · 2019-03-25T18:54:56Z

src/logic/__tests__/linkGithubIdentityTest.js

+  test('credential already in use in experimental mode', async() => {
+    const error = new Error('credential already in use');
+    error.code = 'auth/credential-already-in-use';
+    error.credential = {providerId: 'github.com', accessToken: 'abc123'};


For avoidance of doubt I would include any client-generated error objects in the factories.

outoftime · 2019-03-25T18:55:13Z

src/logic/__tests__/linkGithubIdentityTest.js

+    error.code = 'auth/credential-already-in-use';
+    error.credential = {providerId: 'github.com', accessToken: 'abc123'};
+
+    const githubProfile = {login: 'popcoder'};


Another one for the factories

src/logic/__tests__/linkGithubIdentityTest.js

src/logic/linkGithubIdentity.js

jwang1919 · 2019-04-04T01:00:32Z

@outoftime Can this be merged? As discussed in chat the Rosie implementation will be coming later rather than sooner.

outoftime · 2019-04-04T11:11:49Z

@jwang1919 it's not ideal to have this held up, but I think it's important not to introduce patterns into the new year suite that we don't want to see there in the long run—in this case, ad-hoc definition of test data. Once #1670 is merged, I think it should be pretty quick for you to add the factories you need

Cherry-picked from CodeMirror branch

outoftime · 2019-04-08T00:49:18Z

@jwang1919 the CodeMirror branch is going to take a bit more work before it’s ready for primetime, so I cherry-picked over the bit that sets you up to write factories and pushed it to your branch here. I recommend checking out the documentation for rosie but you can also take a look at the factory I set up in the CodeMirror branch and the reasonably non-annoying way to import factories into jest tests employed therein.

jwang1919 · 2019-04-17T02:06:13Z

@outoftime okay. Used Rosie with linkGithubIdentityTest.js. My approach to getting Rosie to create an Error object was a bit tricky, so let me know if I'm heading in the right direction with this.
Also what would a realistic Firebase object look like so I can Rosie it?

outoftime

@jwang1919 Rosie usage looking good! Left some comments on the details but the overall approach looks just right.

outoftime · 2019-04-18T11:07:41Z

__factories__/clients/firebase.js

+
+export const githubProfile = new Factory().attrs({
+  login: 'popcoder',
+});


The top-level principle for organizing factories should be where the data they’re generating would come from in the real application—in this case this profile object would come from the GitHub client, so this should live in __factories__/clients/github.js.

Also, as I mentioned before, we’ll want to generate factory data that is a reasonably complete representation of what the code we’re testing will see. I don’t necessarily think we need to capture every property of large objects like a GitHub profile, but we should try to capture a subset that generously covers all the properties Popcode might care about (this is a bit subjective, but I trust your judgement). As for where to find that information, API docs are probably your best bet; throwing a debugger or a console.log in the application code and then capturing a real example is another fine approach.

Looks like this is still outstanding?

Huh, I thought I changed this already. I'll check it out again.

Sorry, to be clear—this looks good (very complete!) but it belongs in a __factories__/clients/github.js module because it is synthesizing GitHub client data, not Firebase client data.

outoftime · 2019-04-18T11:07:54Z

__factories__/clients/firebase.js

+
+export const firebaseError = Factory.define(
+  'firebaseError',
+  FirebaseError,


This seems like the right approach to me! 👍

__factories__/clients/firebase.js

src/logic/__tests__/linkGithubIdentityTest.js

outoftime · 2019-04-18T11:15:35Z

src/logic/__tests__/linkGithubIdentityTest.js

+    const mockCredential = {providerId: 'github.com'};
+
+    linkGithub.mockResolvedValue({
+      user: {},


To answer your question the Firebase API docs are pretty good for figuring out the shape of a Firebase User; and again throwing a debugger or console.log() in the application code is a good way to see what the object actually looks like in a concrete instance.

src/logic/__tests__/linkGithubIdentityTest.js

… object.

outoftime

@jwang1919 looks like still a handful of lint errors—let me know if you get stuck with the import one, that plugin has been a Major Payne with the factory setup. i can jump in.

btw you know you don’t have to push and wait for Travis to run to see test results, right? you can run yarn test to get the whole thing, or yarn lint just to see lint errors…

outoftime · 2019-05-08T00:48:02Z

__factories__/clients/firebase.js

+
+export const githubProfile = new Factory().attrs({
+  login: 'popcoder',
+});


Looks like this is still outstanding?

jwang1919 · 2019-05-08T02:25:42Z

So the lint errors seem to only be showing up in Travis but not when I run yarn test.
Ex.
On this line, I added a comment to hide the jslint errors and warnings. yarn test passes for me, but when it Travis thinks it is no bueno.
https://github.com/jwang1919/popcode/blob/jest/src/logic/__tests__/linkGithubIdentityTest.js#L7

…es to .eslintrc

jwang1919 · 2019-05-08T02:45:42Z

@outoftime anyway yeah I need help, Major Payne is kicking my ass.

* master: Archive projects (popcodeorg#1485)

As expected, `eslint-plugin-import` released a fix that corrects the behavior with internal modules that use module lookup aliases. So now the modules aliased under `@factory` are correctly recognized as internal.

outoftime · 2019-05-09T23:55:03Z

@jwang1919 got u

outoftime

@jwang1919 all looks good aside from moving that one factory! (and the one test desc)

outoftime · 2019-05-09T23:57:07Z

__factories__/clients/firebase.js

+
+export const githubProfile = new Factory().attrs({
+  login: 'popcoder',
+});


Sorry, to be clear—this looks good (very complete!) but it belongs in a __factories__/clients/github.js module because it is synthesizing GitHub client data, not Firebase client data.

outoftime · 2019-05-10T00:02:43Z

src/logic/__tests__/linkGithubIdentityTest.js

+    expect(user).toEqual(mockUser);
+  });
+
+  test('credential already in use in experimental mode', async() => {


You can drop the “in experimental mode” from this, that’s an outdated description in the old test.

…djusted test description.

outoftime · 2019-05-16T01:04:11Z

@jwang1919 looks like you might have forgotten to commit the new factory?

jwang1919 · 2019-05-17T01:06:29Z

Oops! My bad.

outoftime and others added 22 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

Fix incorrect merge with .eslintrc

41914c3

Add jest override to .eslintrc

0f4523b

Lint fixing

ca43651

Start redux-logic transition for linkGithubIdentity code

89081bd

Completed redux-logic transition for linkGithubIdentity code

cc93549

Merge remote-tracking branch 'upstream/master' into jest

8db03c2

# Conflicts: # src/logic/index.js # src/sagas/user.js # test/unit/sagas/user.js

outoftime requested changes Mar 25, 2019

View reviewed changes

jwang1919 added 2 commits March 25, 2019 21:43

Fix easy nit-pits. Will work on Rosie and factory approach later.

f0302a7

Fix eslint warnings triggered by Travis

d3db32e

Add support for factories

86abf45

Cherry-picked from CodeMirror branch

Attempt to use Rosie into jest testing

2de5d78

outoftime reviewed Apr 18, 2019

View reviewed changes

jwang1919 added 5 commits April 22, 2019 10:10

Label all factory objects with a suffix of "Factory". Added mock user…

feb77a2

… object.

Merge branch 'master' into jest

dfd9bd4

Fix eslint failing in Travis

bfa3636

Merge remote-tracking branch 'origin/jest' into jest

c67aa17

2nd attempt at fixing eslint failing in Travis

1d9f486

outoftime reviewed May 8, 2019

View reviewed changes

Added more realistic mock data for githubProfileFactory. Revert chang…

58d7cdd

…es to .eslintrc

outoftime added 2 commits May 9, 2019 19:41

Merge branch 'master' into jest

19fa7e5

* master: Archive projects (popcodeorg#1485)

Upgrade eslint-plugin-import, fix import order

86d033e

As expected, `eslint-plugin-import` released a fix that corrects the behavior with internal modules that use module lookup aliases. So now the modules aliased under `@factory` are correctly recognized as internal.

Merge branch 'master' into jest

02fedd8

outoftime requested changes May 10, 2019

View reviewed changes

Created new client factory github.js and moved relevant code there. A…

68123c1

…djusted test description.

outoftime requested changes

abb5d5d

outoftime approved these changes May 18, 2019

View reviewed changes

outoftime merged commit 60357cb into popcodeorg:master May 18, 2019

outoftime mentioned this pull request Nov 1, 2019

Migrate from redux-saga to redux-logic #1934

Closed

25 tasks

Conversation

jwang1919 commented Mar 25, 2019

Uh oh!

outoftime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jwang1919 commented Apr 4, 2019

Uh oh!

outoftime commented Apr 4, 2019

Uh oh!

outoftime commented Apr 8, 2019

Uh oh!

jwang1919 commented Apr 17, 2019

Uh oh!

outoftime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

outoftime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwang1919 commented May 8, 2019

Uh oh!

jwang1919 commented May 8, 2019

Uh oh!

outoftime commented May 9, 2019

Uh oh!

outoftime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

outoftime commented May 16, 2019

Uh oh!

jwang1919 commented May 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!