-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fix test runner (npm test) #222
Fix test runner (npm test) #222
Conversation
FYI I tested |
@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? |
@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. |
What was the exact issue? 2a4b4c2 (last commit on master)
|
This is what I get when on master (running on mac os):
facebook/create-react-app#4215 (comment) Appears to be related to the |
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 <[email protected]>
8188523
to
bb55f40
Compare
@dabutvin can you try a run of |
yep. here's my output
|
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. |
FWIW I've been using the alpha version of There is another work around that is not ideal, but might do until Several people reported that installing |
Could it be because $ grep jest package.json
"jest-enzyme": "^6.0.2",
$ npm list --depth=0 |grep jest
├── UNMET PEER DEPENDENCY jest@>=22.0.0
├── [email protected]
npm ERR! peer dep missing: jest@>=22.0.0, required by [email protected]
npm ERR! peer dep missing: jest@>=22.0.0, required by [email protected]
npm ERR! peer dep missing: ajv@^6.0.0, required by [email protected] 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 <[email protected]>
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.
|
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 $ 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
I have the latest (released in Aug 2017): $ brew info watchman
watchman: stable 4.9.0 (bottled), HEAD |
Due to the fact that we used
The issue in only present in OS X (I think Sierra only?)
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 At this point the easiest thing to do is for me to just add a note to the |
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? |
Sounds good. |
Added a note to clearlydefined/clearlydefined#80 to update the doc fro this. closing this PR |
This fixes
npm test
. We need to usereact-scripts
2+ to be able to use jest + enzyme, perfacebook/create-react-app#4215 (comment). They won't be fixing it for the
react-scripts
1.x branch.