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

Setup for Circle 2 #4149

Merged
merged 14 commits into from
Aug 8, 2017
Merged

Setup for Circle 2 #4149

merged 14 commits into from
Aug 8, 2017

Conversation

thymikee
Copy link
Collaborator

Summary

This is experimental PR to switch to Circle 2.
The main advantage of v2 is "Workflows" feature, enabling easier setup for parallel builds.

There's a lot of repetitive code, but I'm not sure if we can do anything about it.

Test plan

CI pass

@aaronabramov
Copy link
Contributor

seems like it renamed the directory from jest to project
screen shot 2017-07-27 at 4 12 03 pm

and the other build is missing chrome for browser build

@aaronabramov
Copy link
Contributor

you can probably share some of the values using

steps: &steps
  - checkout
  - ...

test-node-6:
  <<: *steps

@thymikee
Copy link
Collaborator Author

Yup, we need a docker container with chrome.
I expected Node 4 to fail, because we build files on higher Node versions.

@aaronabramov
Copy link
Contributor

i love that we have a separate build for each version!

docker:
- image: circleci/node:8.1.4
steps:
<<: *checkout
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i don't think you can merge sequences.

@thymikee
Copy link
Collaborator Author

ok, got to the point where only node 4 fails.
What should we do about that, rewrite our scripts to be node-4 compatible?

@aaronabramov
Copy link
Contributor

sent out #4153 to fix the build script on node@4!
@cpojer can we kill travis at this point?

@cpojer
Copy link
Member

cpojer commented Jul 28, 2017

No, it's important that we verify Jest runs on travis. One of the primary use cases of Jest is to run on CI, and Travis is one of the most popular CI systems.

@thymikee
Copy link
Collaborator Author

At least we'll have faster Circle feedback ;)

@aaronabramov
Copy link
Contributor

@cpojer that makes sense! i'll try to get travis fixed in master then.
Can we not wait for travis in PRs though? i think we'll still get a lot of signal from travis master builds (when they're fixed)

@thymikee
Copy link
Collaborator Author

@aaronabramov Node 4 fails, because local packages are not linked (because they're installed with npm)

@aaronabramov
Copy link
Contributor

oh. so that's a bigger issue..
why is it installed with npm? is it yarn's fallback on old node versions?
can we use nvm on node 4 build, switch to node@8, run yarn and then switch back to node@4 and test?

@thymikee
Copy link
Collaborator Author

This was intentional, I'd like to know for sure if Jest can be safely installed and used on Node 4, because npm 2 produces different node_modules structure, to prevent bugs like we had when releasing jest 20

@thymikee
Copy link
Collaborator Author

Looks like we have eslint plugin import installed, I just need to add no-extraneous-dependencies rule so we prevent bugs caused by using transitive deps

@codecov-io
Copy link

codecov-io commented Jul 29, 2017

Codecov Report

Merging #4149 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4149   +/-   ##
=======================================
  Coverage   60.55%   60.55%           
=======================================
  Files         196      196           
  Lines        6777     6777           
  Branches        6        6           
=======================================
  Hits         4104     4104           
  Misses       2670     2670           
  Partials        3        3
Impacted Files Coverage Δ
...ircus/src/legacy_code_todo_rewrite/jest_adapter.js 0% <ø> (ø) ⬆️
packages/jest-jasmine2/src/index.js 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b2b647...fd68258. Read the comment docs.

@aaronabramov
Copy link
Contributor

@cpojer are we good to merge it?

@cpojer cpojer merged commit 4523826 into jestjs:master Aug 8, 2017
@cpojer
Copy link
Member

cpojer commented Aug 8, 2017

Looks good to me. Can we make it so the website is only deployed when the tests run successfully at least in one version of node?

@thymikee
Copy link
Collaborator Author

thymikee commented Aug 8, 2017

Should be possible. I'll play around with that someday


- &yarn-install
run: |
sudo npm i -g yarn@^0.27.5
Copy link
Contributor

Choose a reason for hiding this comment

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

The Node.js Docker image already has Yarn installed, so you shouldn't need to do this. Also, we discourage installing Yarn via npm 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it a super old version of yarn though?
i remember i had to manually install it on travis

Copy link
Contributor

Choose a reason for hiding this comment

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

Travis doesn't use the Node.js Docker image. I think the Node.js image has the latest Yarn.

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 docker image was using yarn 0.24, that's why I had to install it manually. Will check if that was updated

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do want to upgrade Yarn, we should do that in a Docker image that's layered on top of another one 😃

We should at least be installing Yarn using the Debian package if the image is Debian, based, too :D

Feel free to reach out to me if you need help with it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would appreciate some help here, I'm not really experienced with Docker images 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

There's docs on CircleCI's site: https://circleci.com/docs/2.0/custom-images/

I was going to submit a PR for this, but then I realised it's actually using a bunch of Docker images for various Node.js versions 😛 Initially I was going to do something like this:

# Dockerfile for building and testing Jest

FROM markhobson/node-chrome
MAINTAINER Daniel Lo Nigro <[email protected]>

# Yarn repo
ADD https://dl.yarnpkg.com/debian/pubkey.gpg /tmp/yarn-pubkey.gpg
RUN apt-key add /tmp/yarn-pubkey.gpg && rm /tmp/yarn-pubkey.gpg
RUN echo "deb http://dl.yarnpkg.com/debian/ stable main" > /etc/apt/sources.list.d/yarn.list

# Crowdin repo
ADD https://artifacts.crowdin.com/repo/GPG-KEY-crowdin /tmp/crowdin-pubkey.gpg
RUN apt-key add /tmp/crowdin-pubkey.gpg && rm /tmp/crowdin-pubkey.gpg
RUN echo "deb https://artifacts.crowdin.com/repo/deb/ /" > /etc/apt/sources.list.d/crowdin.list

RUN apt-get -y update && \
  apt-get install -y --no-install-recommends \
    yarn \
    default-jre \
    crowdin \
  && \
  apt-get clean && \
  rm -rf /var/lib/apt/lists/*

# crowdin install start
sudo apt-get install default-jre
wget https://artifacts.crowdin.com/repo/deb/crowdin.deb -O crowdin.deb
sudo dpkg -i crowdin.deb
Copy link
Contributor

@Daniel15 Daniel15 Aug 11, 2017

Choose a reason for hiding this comment

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

Ideally all required software should be installed in a Docker image so that you don't incur the cost every single time the build runs (instead it's just a once-off cost whenever you build the Docker container). That's one of the main advantages of CircleCI 2.0 :) I might submit a PR to do that, if I get a chance.

Basically, there should be some Jest dev Docker image that extends whichever other Docker images you're using, and has this stuff installed.

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Setup for Circle 2

* Try to reuse config

* Fix aliases and add browser build

* Adjust config

* Fix save-cache alias

* Add website/node_modules to alias cache

* Change browser docker

* Add working_directory

* add yarn-install-no-sudo

* Remove environment

* Actually skip react-native example on Node 4

* Declare transitive deps in corresponding package.json

* Install node 4 deps with yarn

* Setup import/no-extraneous-dependencies rule
@thymikee thymikee deleted the circle-2 branch March 16, 2019 10:55
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants