Internal: Convert ghostjs to puppeteer #182
Conversation
|
You should check out #158! |
christianvuerings
force-pushed
the
puppeteer
branch
from
5c68e6f
to
4a449b9
Compare
christianvuerings
changed the title
|
Deploy preview for gestalt ready! Built with commit b90fbd1 |
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
=======================================
Coverage 61.03% 61.03%
=======================================
Files 40 40
Lines 770 770
Branches 178 178
=======================================
Hits 470 470
Misses 207 207
Partials 93 93Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Rather than scoping by filename *.integration.js - can you just use the regular test regexp and use the @Heredoc at the top of the test?
Dockerfile
Outdated
| && apt-get update \ | ||
| && apt-get install -y google-chrome-unstable --no-install-recommends \ | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| && npm i puppeteer@1.4.0 |
There was a problem hiding this comment.
This is a dependency of the tests - any reason why you're doing this here?
There was a problem hiding this comment.
@chrislloyd was a trail / error of combining multiple docker commands. Was able to get rid of it.
Dockerfile
Outdated
| COPY yarn.lock package.json ./ | ||
|
|
||
| RUN mkdir -p docs test packages/gestalt | ||
| COPY packages/gestalt/package.json ./packages/gestalt/ | ||
| COPY docs/package.json ./docs/ | ||
| COPY test/package.json ./test/ | ||
|
|
||
| RUN apt-get update \ |
There was a problem hiding this comment.
I think you want this RUN command to be high so it's not invalidated by changes to the package.json etc.
There was a problem hiding this comment.
@chrislloyd indeed - moved this one (and the other RUN command) back up.
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
This should probably be baked into the package.json - you can always run jest with only a particular project if you want to narrow the scope. jest --project integration
There was a problem hiding this comment.
@chrislloyd indeed, that should be the case if we make puppeteer a jest project. It seems like we're running into a couple of issues though if we're going for that approach:
{
"displayName": "puppeteer",
"preset": "jest-puppeteer",
"testMatch": ["<rootDir>**/__integration__/**/*.integration.js"]
}
scripts/jestIntegration.config.json
Outdated
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "name": "integration", | |||
There was a problem hiding this comment.
It should probably be named puppeteer to match node & jsdom (name the tool rather than the type of test).
| @@ -0,0 +1,14 @@ | |||
| module.exports = { | |||
| launch: { | |||
There was a problem hiding this comment.
This config also takes a server option that you can use to have Jest run the dev server. That might mean you can remove run_integration_tests.sh
There was a problem hiding this comment.
@chrislloyd I tried that before but it seems to not wait for the server to be launched, even though I pass in the port option:
server: {
command:
'(cd packages/gestalt && yarn prepublish) && (cd test && yarn start)',
port: 3001,
},
Seems like this might be a task better tackled in #162 - wdyt?
There was a problem hiding this comment.
./run_integration_tests has a timeout for the server to start - perhaps you could just use that script without the bit which actually runs the tests?
test/package.json
Outdated
| "start:webpack-server": "yarn webpack-dev-server --hot --inline --progress --colors", | ||
| "start:node-server": "BABEL_DISABLE_CACHE=1 NODE_ENV=testing yarn babel-node server.js" | ||
| }, | ||
| "devDependencies": {} |
There was a problem hiding this comment.
@chrislloyd it was not, removed
|
The other reason - we lost parallelism with this. If we can at least consolidate around 1 command to run all the tests then we can add parallelism to that one place and chunk both unit and integration tests across machines. |
Could you explain this a bit more?
Indeed, I removed it within the integration tests since it no longer seemed necessary. Before, the combined integration tests were taking 5+ mins, now they only take 1m 20s https://buildkite.com/pinterest/gestalt/builds/4241 |
|
@christianvuerings https://facebook.github.io/jest/docs/en/configuration.html#testenvironment-string 1:20 is still too long! As we add more we'll still have to parallelize the tests - it would be great if we could get that parallelism for free also with regular Jest unit tests. |
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "globals": { | |||
There was a problem hiding this comment.
@christianvuerings i think this can be avoided, its baked into eslint now, you just need to add .integration.js to the overrides for jest env
There was a problem hiding this comment.
Good catch - I'll do this in a follow up PR. I'd like to get this merged ASAP.
There was a problem hiding this comment.
@chrislloyd @christianvuerings check out #193 which updates based on the instructions here
No description provided.