Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codeowners file for automatic reviews and clean up some configs #139

Merged
merged 9 commits into from
Feb 7, 2019

Conversation

jwu910
Copy link
Contributor

@jwu910 jwu910 commented Feb 6, 2019

Edit:

Summary of the scope of work for this PR.

Initial scope of work was to just include a CODEOWNERS file to allow for automatic review assignments. Discovered issues with concurrent use of jest/react-scripts test to run separate tests.

Scope evolved to removing the use of react-scripts test so we could continue to use our project specific test structure

rainrivas
rainrivas previously approved these changes Feb 7, 2019
Copy link
Contributor

@rainrivas rainrivas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gucci 💯

I don't get why seedDB is suddenly broken with CI... any thoughts on this one? Aside from that we golden

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

No clue, last commits that made it to development were in september.

Can i just add the seedDB file to the eslint ignore?

@rainrivas
Copy link
Contributor

@jwu910 yeah, lets do that in this PR

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

jestjs/jest#5119 (comment) Looks like the failing tests are a conflict of dependencies.

react-scripts node_modules output includes jest so we shouldn't include it as a dev dependency. I was able to reproduce the error locally, remove jest, and test this solution.

However I do see some snapshot updates though I haven't touched anything.

@jwu910 jwu910 changed the title Add codeowners file for automatic reviews Add codeowners file for automatic reviews and clean up some configs Feb 7, 2019
@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

Lol nvm, we have some other issues we need to figure out. Something changed somewhere, causing react-scripts to break if jest is a devDep becuase its already included, but we have our api tests set up with jest and not react-scripts.

Thoughts?

@rainrivas
Copy link
Contributor

@jwu910 there was a reason why we set up the scripts with jest separately from react-scripts but I don't recall it off the top of my head. Is it somewhere in the commit logs or issue list I wonder...

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

@rainrivas Yeah I'm looking through everything a little bit more now. I think we'll have to reassess at some point how we're managing these test scripts. It's a little much to require jest on top of react-scripts haha.

No clue why its breaking now. but react-scripts uses jest 22.4.4 -- which seeeems to be the same version as we have installed with last nights discussion about ^22.2.2 => 22.4.4

@rainrivas
Copy link
Contributor

@jwu910 I found the initial addition of jest in the PR #106
I added jest despite it being in react-scripts because we are depending on it to run our scripts. I believe i did this because it's good convention and practice to explicitly include things you need.

@rainrivas
Copy link
Contributor

@jwu910 I found the initial addition of jest in the PR #106
I added jest despite it being in react-scripts because we are depending on it to run our scripts. I believe i did this because it's good convention and practice to explicitly include things you need.

However I think things might have changed since we did this a year ago....
Lets read this doc from Jest team on how jest and react should be setup -- https://jestjs.io/docs/en/tutorial-react#setup-with-create-react-app

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

@rainrivas I'm still all for ejecting react-scripts. or just ditching it and setting up the build files ourselves. We would def have to check our jest setup again. Something to consider. Jest looks like its at latest v24 now--our setup is 2 major version behind :p

I'll skim through and see if there's anything we can do w/o having to eject. Maybe just explicitly use jest instead of react-scripts test

@rainrivas
Copy link
Contributor

@jwu910 Maybe this is why we decided to install our own Jest?

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

Correction: Commit message should have said "remove use of react-scripts test"

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

Force pushed to rename commit so it isn't misleading

@jwu910 jwu910 requested a review from rainrivas February 7, 2019 19:43
Copy link
Contributor

@rainrivas rainrivas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a babelrc now? Doesn't react have a built in config of sorts? If not, why were we missing it for so long?

@jwu910
Copy link
Contributor Author

jwu910 commented Feb 7, 2019

@rainrivas the changes with jest, -- now that we're testing our react/jsx code with it and not just the es6 tests we had for APIs, jest needs environment information on how to read the tests

@jwu910 jwu910 merged commit 118dab8 into development Feb 7, 2019
@jwu910 jwu910 deleted the AAF-0 branch February 7, 2019 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants