Skip to content
This repository was archived by the owner on Jan 26, 2019. It is now read-only.

Fix travis build #302

Merged
merged 2 commits into from
Apr 27, 2018
Merged

Fix travis build #302

merged 2 commits into from
Apr 27, 2018

Conversation

DorianGrey
Copy link
Collaborator

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-scripts 2.0 from what is required in 1.x.
Just testing this for the moment and see how far I may get ...

@DorianGrey DorianGrey force-pushed the travis-build-fix-attempt branch 5 times, most recently from 2c624b9 to e9d87b1 Compare April 13, 2018 09:37
@DorianGrey DorianGrey force-pushed the travis-build-fix-attempt branch from e9d87b1 to 544ae2b Compare April 13, 2018 09:39
@DorianGrey
Copy link
Collaborator Author

@wmonk , things are getting green - yay!
A second look would be good, to take care I did not pick up something unintended. Stuff was picked from CRA's master branch that should not contain things planned for their 2.0 exclusively.

@DorianGrey DorianGrey requested a review from wmonk April 13, 2018 10:11
wmonk
wmonk suggested changes Apr 13, 2018
View reviewed changes
Copy link
Owner

@wmonk wmonk left a 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.

tasks/e2e-installs.sh
cd "$root_path"/packages/create-react-app
cli_path=$PWD/`npm pack`
cd "$temp_app_path"
npx create-react-app --scripts-version=1.0.17 test-app-version-number
Copy link
Owner

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 install react-scripts right?

Copy link
Collaborator Author

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.

tasks/e2e-installs.sh
# ******************************************************************************

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
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above

tasks/e2e-installs.sh
checkDependencies

# ******************************************************************************
# Test --scripts-version with a tarball url
# ******************************************************************************

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
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above

tasks/e2e-installs.sh

# Check corresponding scripts version is installed.
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
Copy link
Owner

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
@@ -284,40 +212,39 @@ function verify_module_scope {
echo "{}" >> sample.json

# 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
Copy link
Owner

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 of react-scripts

Copy link
Collaborator Author

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
Copy link
Owner

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 the scripts_path variable for --scripts-version.

Copy link
Collaborator Author

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
@@ -16,13 +12,17 @@ cache:
install: true
script:
- 'if [ $TEST_SUITE = "simple" ]; then tasks/e2e-simple.sh; fi'
- 'if [ $TEST_SUITE = "installs" ]; then tasks/e2e-installs.sh; fi'
Copy link
Owner

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 keep e2e-simple as the others test a lot of stuff from create-react-app we don't care about.

Copy link
Collaborator Author

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
Copy link
Owner

Choose a reason for hiding this comment

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

Can just keep simple here.

Copy link
Collaborator Author

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
Copy link
Owner

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 testing create-react-app as far as I know.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@DorianGrey
Copy link
Collaborator Author

Thanks for the review, I'm looking to fix these issue after my weekend vacation.

@wmonk wmonk mentioned this pull request Apr 13, 2018
Closed
@DorianGrey
Copy link
Collaborator Author

I've disabled the installs test for now, since this will require a lot of additional work to be done.
I'm still working on the other ones - esp. e2e-simple.sh was changed a lot, now using a local registry instead of local packing. Still experimenting which parts have to reverted to the previous strategy.

@DorianGrey DorianGrey force-pushed the travis-build-fix-attempt branch from 0a1f4ea to 169a401 Compare April 17, 2018 07:35
@wmonk
Copy link
Owner

wmonk commented Apr 17, 2018

Awesome work @DorianGrey - is this ready to be merged?

@DorianGrey
Copy link
Collaborator Author

Suppose, yes.

We might have to deal with e2e-installs.sh, but I don't think it's that important for now. I've just disabled the test suite and left a particular comment that additional work has to be done before activating this again.

@DorianGrey DorianGrey changed the title [W.I.P.] Attempt to fix travis build Fix travis build Apr 27, 2018
@DorianGrey DorianGrey merged commit 97d5561 into wmonk:master Apr 27, 2018
sebald pushed a commit to sebald/create-react-app-typescript that referenced this pull request May 8, 2018
Merge branch 'master' into allow-moduleNameMapper-override
25d06bb 
* master:
  Stick @types/node to 9.6.7 to avoid problems with 10.0.0 (wmonk#315)
  Fix travis build (wmonk#302)
  v2.15.1
  Update README For 2.15.1
  fix(modules): remove duplicate mjs from jest config
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@wmonk wmonk wmonk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DorianGrey @wmonk