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

Configure testing and add some sample tests #176

Merged
merged 6 commits into from
Jun 23, 2018

Conversation

mynameistechno
Copy link
Contributor

Note:

  • I had a few issues getting jest + enzyme to work with the current versions of our dependecies. I ended up upgrading all of them - minor version only though! Except for react-scripts, which I needed the next major version, per one of the react maintainers: Mac OS test watcher failing facebook/create-react-app#4215 (comment) If needed, I can go back and upgrade only what is needed, but the site seemed fine.
  • After upgrading I ran into another issue with how we were using reducers. The use of the new keyword in conjunction with the reducer functions was causing a Not a constructor errors by webpack in the dev server. It looks the reducers already return new objects using the spread operator, so I removed the new keyword as it didn't seem necessary, but @jeffmcaffer you may want to take a look. See commit 3bbafb2

Mark Matyas added 6 commits June 22, 2018 09:53
Signed-off-by: Mark Matyas <[email protected]>

Signed-off-by: Mark Matyas <[email protected]>
I needed to upgrade to get enzyme to work.

Signed-off-by: Mark Matyas <[email protected]>
This was causing "not a constructor" errors in webpack after minor
upgrades. Need @jeffmcaffer to review.

Signed-off-by: Mark Matyas <[email protected]>
Signed-off-by: Mark Matyas <[email protected]>
Signed-off-by: Mark Matyas <[email protected]>
@jeffmcaffer
Copy link
Member

@mynameistechno this is awesome! Thanks. The changes look good all up. I'll comment on your comments here.

  • The version updates are good. The only thing to watch out for is react-bootstrap-typeahead. >=3.0.0 will break the harvest page. We are using an undocumented method that has been removed. See Setting the input value  ericgio/react-bootstrap-typeahead#266 (comment) if you care for the details. We can change our UI there but for right now lets stick to 2.6.0.
  • the use of new actually is sorta weird. We should not be doing that anyway AFAICT. Curious that we are. Perhaps at one point that was the requirement and then the code never got changed. Good catch.
  • as best I can tell I have enabled travis on the website repo. It seemed like it already was so ...

(BTW, somewhat unrelated but we now have contribution PR validation checks in the ClearlyDefined service so I'm going to turn off the Travis builds for PRs in the curated repos. They never got updated and are failing now.

I'll poke at this PR locally and then merge.

@jeffmcaffer jeffmcaffer merged commit 868ac5c into clearlydefined:master Jun 23, 2018
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.

2 participants