Fix test runner (npm test) #222

mynameistechno · 2018-07-20T20:55:22Z

This fixes npm test. We need to use react-scripts 2+ to be able to use jest + enzyme, per
facebook/create-react-app#4215 (comment). They won't be fixing it for the react-scripts 1.x branch.

mynameistechno · 2018-07-20T21:00:46Z

FYI I tested npm test and npm run-script build. No problems with the react-scripts upgrade 👍

jeffmcaffer · 2018-07-25T04:59:21Z

@mynameistechno are you comfortable doing a major version change for react-scripts? and to a non-released version? I looked through the changes in the package-lock file and there appear to be some pretty extensive changes as a result of the update. Not against this but would like to be sure we are not taking on some additional risks. For example, the creators note of the alpha builds warning: they're somewhat unstable in the cited issue.

They also note that there are some other workarounds. Can we use some of them? Is there a different way to run the tests?

mynameistechno · 2018-07-26T20:56:13Z

@jeffmcaffer we've been using it with a couple projects no problem (so far). That unstable comment was from April though. I think they are waiting for Babel 7 to go stable to release 2.0 at the same time. If you can wait a few days I can try something else that I think may work.

gianpaj · 2018-08-13T15:04:56Z

What was the exact issue?
If the tests were not passing, at the moment they do.

2a4b4c2 (last commit on master)

 PASS  src/components/__tests__/App.test.js (6.866s)
 PASS  src/components/__tests__/PageInspect.test.js (6.883s)

Test Suites: 2 passed, 2 total
Tests:       12 passed, 12 total
Snapshots:   0 total
Time:        7.896s
Ran all test suites.

mynameistechno · 2018-08-14T23:42:22Z

This is what I get when on master (running on mac os):

~/p/c/website ❯❯❯ npm test                                                                                                                                              
> website@0.1.0 test /Users/mmatyas/projects/clearlydefined/website
> react-scripts test --env=jsdom

2018-08-14 16:34 node[33942] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2018-08-14 16:34 node[33942] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2018-08-14 16:34 node[33942] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2018-08-14 16:34 node[33942] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: Error watching file for changes: EMFILE
    at _errnoException (util.js:1024:11)
    at FSEvent.FSWatcher._handle.onchange (fs.js:1351:9)
npm ERR! Test failed.  See above for more details.

facebook/create-react-app#4215 (comment)

Appears to be related to the watch functionality. As gaeron mentions, upgrading to react-scripts (which upgrades jest) fixes the issue. Am I the only seeing the error? You guys using linux/windows?

We need to use `react-scripts` 2+ to be able to use jest + enzyme, per facebook/create-react-app#4215 (comment) They won't be fixing it for `react-scripts` 1.x branch. Signed-off-by: Mark Matyas <mmatyas@codeaurora.org>

jeffmcaffer · 2018-08-14T23:59:16Z

@dabutvin can you try a run of npm test in the website repo on you Mac and see if you can repo @mynameistechno 's problem?

dabutvin · 2018-08-15T02:11:03Z

yep. here's my output

$ npm test

> website@0.1.0 test /Users/dan/codes/clearlydefined/website
> react-scripts test --env=jsdom

2018-08-14 19:10 node[4629] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2018-08-14 19:10 node[4629] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: Error watching file for changes: EMFILE
    at _errnoException (util.js:992:11)
    at FSEvent.FSWatcher._handle.onchange (fs.js:1359:9)
npm ERR! Test failed.  See above for more details.

jeffmcaffer · 2018-08-15T02:51:16Z

well "good" I guess. At least it is reproducible. So we need to ensure that Mac-based devs can work on the website. I'm just not sure how to really validate that the upgrade to the alpha react-scripts is the right thing. There could be so many subtle side effects and it is still alpha. Sucks not having a decent test suite.

mynameistechno · 2018-08-15T05:12:41Z

FWIW I've been using the alpha version of react-scripts internally for a few weeks and the test and build commands have been working fine so far.

There is another work around that is not ideal, but might do until react-scripts 2 is out of alpha. The react-scripts repo has added a note on this issue in their readme with a suggestion: https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#npm-test-hangs-on-macos-sierra

Several people reported that installing watchman using brew , i.e. brew install watchman fixed it for them. I can confirm this works. So maybe we just add a note for mac users that links to the above section in our readme?

gianpaj · 2018-08-15T07:20:09Z

Could it be because jest is not a dependency?

$ grep jest package.json
    "jest-enzyme": "^6.0.2",


$ npm list --depth=0 |grep jest
├── UNMET PEER DEPENDENCY jest@>=22.0.0
├── jest-enzyme@6.0.2
npm ERR! peer dep missing: jest@>=22.0.0, required by jest-enzyme@6.0.2
npm ERR! peer dep missing: jest@>=22.0.0, required by jest-environment-enzyme@6.0.2
npm ERR! peer dep missing: ajv@^6.0.0, required by ajv-keywords@3.1.0

This is on a macOS 10.13.6 (High Sierra)

$ rm -rf node_modules
$ git show
commit 2a4b4c22928f376ea591e2fceeb135ec5d433e11 (HEAD -> master, origin/master, origin/HEAD)
Merge: 388e23b ef9a5c0
Author: Jeff McAffer <jeff.mcaffer@microsoft.com>
Date:   Sat Aug 11 07:31:03 2018 -0700

    Merge pull request #241 from WebYourMind/webyourmind/full-details-view

    Full Details View - Read Only
$ npm install
...
added 1751 packages from 934 contributors and audited 16873 packages in 28.446s

$ npm test
No tests found related to files changed since last commit.
Press `a` to run all tests, or run Jest with `--watchAll`.

Watch Usage
 › Press a to run all tests.
 › Press p to filter by a filename regex pattern.
 › Press t to filter by a test name regex pattern.
 › Press q to quit watch mode.
 › Press Enter to trigger a test run.

# I pressed 'a'

 PASS  src/components/__tests__/App.test.js
 PASS  src/components/__tests__/PageInspect.test.js

Test Suites: 2 passed, 2 total
Tests:       12 passed, 12 total
Snapshots:   0 total
Time:        4.35s, estimated 7s
Ran all test suites.

$ node --version
v8.11.1
$ npm --version
6.3.0
$ npm test -- --version

> website@0.1.0 test /Users/gianfranco/clearlydefined/websiteCD
> react-scripts test --env=jsdom "--version"

v20.0.4

gianpaj · 2018-08-15T11:05:25Z

I tried on a Docker container (Node 10.x) to test if the tests worked on my laptop environment because I had some special version of watchman or something else:

$ docker pull node
...
Status: Downloaded newer image for node:latest
$ docker run -it node bash
@4c383e1c819e:/#

# (now we are inside the docker container)

$ git clone https://github.com/clearlydefined/website.git
$ cd website/
$ npm install
$ npm test
# all fine and dandy 🙂

$ exit
# (back on your host)

# to remove the container
$ docker rm __the_docker_container__

@mynameistechno have you tried to (re)install watchman?

I have the latest (released in Aug 2017):

$ brew info watchman
watchman: stable 4.9.0 (bottled), HEAD

https://facebook.github.io/watchman/docs/release-notes.html

mynameistechno · 2018-08-15T20:43:10Z

Could it be because jest is not a dependency?

Due to the fact that we used create-react-app, react-scripts manages jest as it's own dependency. To get around this we'd have to "eject" react-scripts, but then we might have other concerns to worry about, like having to manage the webpack config ourselves, and we'd lose out on any benefits react-scripts 2.0 will get us for free. I guess we might be able to use npm shrinkwrap to force jest to be a certain version.

I tried on a Docker container (Node 10.x) to test if the tests worked on my laptop environment because I had some special version of watchman or something else:

The issue in only present in OS X (I think Sierra only?)

@mynameistechno have you tried to (re)install watchman?

Yes installing watchman fixes it, but this is more of a hack, and less than ideal as the real issue appears to be a bug in the version of Jest that react-scripts v1 is pinned to. They won't upgrade jest in v1 due to a change that breaks BC, so they recommend upgrading to react-scripts v2 (which is in alpha), or try one of the workarounds, like installing watchman.

At this point the easiest thing to do is for me to just add a note to the website's readme about this issue on mac os, and point them https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#npm-test-hangs-on-macos-sierra, which recommends installing watchman. Once react-scripts 2.0 ships we can remove the note. @jeffmcaffer thoughts?

jeffmcaffer · 2018-08-21T06:32:06Z

At this point I'd rather document the issue and workaround than take all the change and then the risk associated with being on an alpha of a foundational piece. I think in a few weeks we should regroup and consolidate a bunch of refactorings and version upgrades. This should be one of the things considered.

Do you want to just change this PR to add the doc in the README? We should also add some notes in https://github.com/clearlydefined/clearlydefined where we talk about the easy setup (https://docs.clearlydefined.io/contributing-code). Perhaps just a pointer there to the canonical guidance in the README here?

mynameistechno · 2018-08-31T17:09:16Z

Do you want to just change this PR to add the doc in the README? We should also add some notes in https://github.com/clearlydefined/clearlydefined where we talk about the easy setup (https://docs.clearlydefined.io/contributing-code). Perhaps just a pointer there to the canonical guidance in the README here?

Sounds good.

jeffmcaffer · 2018-09-02T20:03:59Z

Added a note to clearlydefined/clearlydefined#80 to update the doc fro this.

closing this PR

Fix test runner

bb55f40

We need to use `react-scripts` 2+ to be able to use jest + enzyme, per facebook/create-react-app#4215 (comment) They won't be fixing it for `react-scripts` 1.x branch. Signed-off-by: Mark Matyas <mmatyas@codeaurora.org>

mynameistechno force-pushed the fix-test-runner branch from 8188523 to bb55f40 Compare August 14, 2018 23:49

jeffmcaffer mentioned this pull request Sep 2, 2018

Update documentation on how to manage development environments for website project clearlydefined/clearlydefined#80

Closed

jeffmcaffer closed this Sep 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test runner (npm test) #222

Fix test runner (npm test) #222

mynameistechno commented Jul 20, 2018

mynameistechno commented Jul 20, 2018

jeffmcaffer commented Jul 25, 2018

mynameistechno commented Jul 26, 2018

gianpaj commented Aug 13, 2018

mynameistechno commented Aug 14, 2018

jeffmcaffer commented Aug 14, 2018

dabutvin commented Aug 15, 2018

jeffmcaffer commented Aug 15, 2018

mynameistechno commented Aug 15, 2018

gianpaj commented Aug 15, 2018 •

edited

Loading

gianpaj commented Aug 15, 2018 •

edited

Loading

mynameistechno commented Aug 15, 2018 •

edited

Loading

jeffmcaffer commented Aug 21, 2018

mynameistechno commented Aug 31, 2018

jeffmcaffer commented Sep 2, 2018

Fix test runner (npm test) #222

Fix test runner (npm test) #222

Conversation

mynameistechno commented Jul 20, 2018

mynameistechno commented Jul 20, 2018

jeffmcaffer commented Jul 25, 2018

mynameistechno commented Jul 26, 2018

gianpaj commented Aug 13, 2018

mynameistechno commented Aug 14, 2018

jeffmcaffer commented Aug 14, 2018

dabutvin commented Aug 15, 2018

jeffmcaffer commented Aug 15, 2018

mynameistechno commented Aug 15, 2018

gianpaj commented Aug 15, 2018 • edited Loading

gianpaj commented Aug 15, 2018 • edited Loading

mynameistechno commented Aug 15, 2018 • edited Loading

jeffmcaffer commented Aug 21, 2018

mynameistechno commented Aug 31, 2018

jeffmcaffer commented Sep 2, 2018

gianpaj commented Aug 15, 2018 •

edited

Loading

gianpaj commented Aug 15, 2018 •

edited

Loading

mynameistechno commented Aug 15, 2018 •

edited

Loading