- Notifications
You must be signed in to change notification settings - Fork488
Fix travis build#302
Fix travis build#302
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2c624b9 toe9d87b1Comparee9d87b1 to544ae2bCompareDorianGrey commentedApr 13, 2018
@wmonk , things are getting green - yay! |
wmonk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Awesome work@DorianGrey 🎉
I have some comments that need to be addressed to get this fully finished, but they shouldn't be very big.
| cd"$root_path"/packages/create-react-app | ||
| cli_path=$PWD/`npm pack` | ||
| cd"$temp_app_path" | ||
| npxcreate-react-app --scripts-version=1.0.17 test-app-version-number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think--scripts-version here will installreact-scripts right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This test suite is disabled for now.
| cd"$temp_app_path" | ||
| create_react_app --scripts-version=0.4.0 test-app-version-number | ||
| cd test-app-version-number | ||
| npx create-react-app --use-npm --scripts-version=1.0.17 test-use-npm-flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as above
| cd"$temp_app_path" | ||
| create_react_app --scripts-version=https://registry.npmjs.org/react-scripts/-/react-scripts-0.4.0.tgz test-app-tarball-url | ||
| npx create-react-app --scripts-version=https://registry.npmjs.org/react-scripts/-/react-scripts-1.0.17.tgz test-app-tarball-url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as above
| exists node_modules/react-scripts | ||
| grep'"version": "0.4.0"' node_modules/react-scripts/package.json | ||
| [!-e"yarn.lock" ]&&echo"yarn.lock correctly does not exist" | ||
| grep'"version": "1.0.17"' node_modules/react-scripts/package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Our version is different now
tasks/e2e-simple.sh Outdated
| # Save App.js, we're going to modify it | ||
| cp src/App.tsx src/App.tsx.bak | ||
| cp src/App.js src/App.js.bak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the bit that shows we're installing the wrong version ofreact-scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
tasks/e2e-simple.sh Outdated
| # Install the app in a temporary location | ||
| cd$temp_app_path | ||
| create_react_app --scripts-version="$scripts_path" test-app | ||
| npx create-react-app test-app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the bit that needs to use thescripts_path variable for--scripts-version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done. Due to the new strategy with a local registry, it seems that we only need to provide--scripts-version=react-scripts-ts here.
.travis.yml Outdated
| install:true | ||
| script: | ||
| -'if [ $TEST_SUITE = "simple" ]; then tasks/e2e-simple.sh; fi' | ||
| -'if [ $TEST_SUITE = "installs" ]; then tasks/e2e-installs.sh; fi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can just keepe2e-simple as the others test a lot of stuff fromcreate-react-app we don't care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.install is disabled for now.
.travis.yml Outdated
| # TODO: reenable after we ship 1.0. | ||
| # - node_js: 6 | ||
| # env: USE_YARN=yes TEST_SUITE=simple | ||
| -TEST_SUITE=installs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can just keepsimple here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.install is disabled for now.
.travis.yml Outdated
| -TEST_SUITE=kitchensink | ||
| matrix: | ||
| include: | ||
| -node_js:0.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we want this line, more for testingcreate-react-app as far as I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
DorianGrey commentedApr 13, 2018
Thanks for the review, I'm looking to fix these issue after my weekend vacation. |
DorianGrey commentedApr 16, 2018
I've disabled the |
0a1f4ea to169a401Comparewmonk commentedApr 17, 2018
Awesome work@DorianGrey - is this ready to be merged? |
DorianGrey commentedApr 17, 2018
Suppose, yes. We might have to deal with |
A "work in progress" for now - seems that not all changes made it into the merge with 1.1.1 because we to separate the stuff that should go into
react-scripts2.0 from what is required in 1.x.Just testing this for the moment and see how far I may get ...