tests(viewer): don't override puppeteer's chromium #9877

connorjclark · 2019-10-23T06:33:56Z

This test is currently failing in master.

The viewer's puppeteer test doesn't work with Chrome 78, and neither does the latest puppeteer (1.20.0).

Puppeteer ships with a specific Chromium revision, and is only meant to work with that, so we shouldn't override it.

This test is currently failing in master. The viewer's puppeteer test doesn't work with Chrome 78, and neither does the latest puppeteer (1.20.0). Puppeteer ships with a specific Chromium revision, and is only meant to work with that, so we shouldn't override it.

…ouse into connorjclark-patch-1

patrickhulce · 2019-10-23T15:36:49Z

that didn't seem to do the trick yet

viewer tests failed with

Chromium revision is not downloaded. Run "npm install" or "yarn install"

connorjclark · 2019-10-23T15:59:42Z

MaybeTravis doesn't clear env variables between runs?

brendankenny · 2019-10-23T16:05:21Z

Puppeteer ships with a specific Chromium revision, and is only meant to work with that, so we shouldn't override it.

this was partly to avoid redownloading Chromium every time (it may have been skipping node_modules/ caching since pptr downloads it itself on install?) and making the travis runs even slower. Ideally someone in the world has figured that out since #4640?

Otherwise, is there something specific that broke with m78 and is it something we can get fixed upstream? pptr is going to have to update its version of Chromium at some point...

connorjclark · 2019-10-23T17:04:54Z

Otherwise, is there something specific that broke with m78 and is it something we can get fixed upstream? pptr is going to have to update its version of Chromium at some point...

@mathiasbynens might know. We could bisect it.

connorjclark · 2019-10-23T21:20:13Z

tools/bisect-builds.py --use-local-cache -a linux64 -g 674921 -b 693954 --not-interactive -c 'bash -c "cd ~/code/lighthouse && CHROME_PATH=%p yarn jest viewer -t PSI"' --verify-range

Can't get the bad revision (693954 is current stable) to fail, even though using the stable channel binary directly does fail. Weird.

This reverts commit ca383f5.

connorjclark · 2019-10-23T23:56:31Z

For travis, had to unset the puppeteer env variable. I guess it stuck around from previous runs.

patrickhulce

love that green build :)

a major bummer to our build times but a necessary evil to be able to keep merging I suppose

patrickhulce · 2019-10-24T01:10:03Z

.travis.yml

- # with the latest puppeteer api so we probably need to run on chromimum
- # @see https://github.com/GoogleChrome/lighthouse/pull/4640/files#r171425004
- - export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=1
+ - unset PUPPETEER_SKIP_CHROMIUM_DOWNLOAD


I'm assuming can we remove this at some point?

yeah, but can't be sure at what point. unless someone wants to merge master into ever outstanding PR branch after this lands.

It sure is unexpected that environment variables seem to persist.

we might need to just clear the caches. If we keep this we should also leave a comment similar to the removed one for why this is being unset :)

Clearing cache didn't help, I tried that first.

environment variables seem to persist

I just don't see how this makes sense. It seems like people would be constantly running into it.

Maybe b/c we should have use env: instead of a manual export?

connorjclark · 2019-10-24T05:45:51Z

a major bummer to our build times but a necessary evil to be able to keep merging I suppose

We could consider removing the other Chrome install, and just use Puppeteer's instead. We would have to manage updating Puppeteer on a regular basis. note: I don't love this idea.

mathiasbynens · 2019-10-24T06:37:38Z

Can't get the bad revision (693954 is current stable) to fail, even though using the stable channel binary directly does fail. Weird.

This sounds super weird indeed... I wonder what we're missing?

We could consider removing the other Chrome install, and just use Puppeteer's instead. We would have to manage updating Puppeteer on a regular basis. note: I don't love this idea.

FWIW, we're committed to cutting a new Puppeteer release for every Chrome milestone (at the very least). Managing the major Chrome versions through whatever the latest Puppeteer requires seems like the way to go, imho.

connorjclark · 2019-10-24T18:23:24Z

One unexpected thing is that https://omahaproxy.appspot.com/ gives me 78.0.3904.70 as the stable version and 693954 as its base revision, but when the bisect script downloads 693954 it's actually version 78.0.3904.0.

tools/bisect-builds.py --use-local-cache -a mac -g 674921 -b 693954 --not-interactive -c '%p --version' --verify-range

brendankenny

does it work with the new 2.0.0 pptr release?

brendankenny · 2019-10-24T21:17:21Z

.travis.yml

- # with the latest puppeteer api so we probably need to run on chromimum
- # @see https://github.com/GoogleChrome/lighthouse/pull/4640/files#r171425004
- - export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=1
+ - unset PUPPETEER_SKIP_CHROMIUM_DOWNLOAD


we might need to just clear the caches. If we keep this we should also leave a comment similar to the removed one for why this is being unset :)

connorjclark · 2019-10-24T21:22:20Z

does it work with the new 2.0.0 pptr release?

I don't think it will, since the issue is with Chromium and not Puppeteer. Let's see: #9886

brendankenny

well, 🤷. At least it'll work now :)

LGTM

connorjclark added 2 commits October 18, 2019 14:41

misc: add chrome version field to bug report template

164a42a

connorjclark requested a review from brendankenny as a code owner October 23, 2019 06:33

googlebot added the cla: yes label Oct 23, 2019

connorjclark added 2 commits October 23, 2019 00:27

Merge branch 'connorjclark-patch-1' of github.com:GoogleChrome/lighth…

2fb7c28

…ouse into connorjclark-patch-1

download pup chr

5b65636

vercel bot deployed to staging October 23, 2019 07:28 View deployment

tmp change

ca383f5

vercel bot deployed to staging October 23, 2019 21:37 View deployment

Revert "tmp change"

b69c27a

This reverts commit ca383f5.

vercel bot deployed to staging October 23, 2019 21:39 View deployment

test

570f564

vercel bot deployed to staging October 23, 2019 23:24 View deployment

skips

9080bbe

vercel bot deployed to staging October 23, 2019 23:27 View deployment

unset

97f662c

vercel bot deployed to staging October 23, 2019 23:34 View deployment

fix

4819d9d

vercel bot deployed to staging October 23, 2019 23:56 View deployment

patrickhulce approved these changes Oct 24, 2019

View reviewed changes

brendankenny reviewed Oct 24, 2019

View reviewed changes

connorjclark mentioned this pull request Oct 24, 2019

TEST: puppeteer 2 ci #9886

Closed

comment

dde3791

vercel bot deployed to staging October 24, 2019 21:24 View deployment

brendankenny approved these changes Oct 24, 2019

View reviewed changes

connorjclark merged commit 4e11bd2 into master Oct 24, 2019

connorjclark deleted the connorjclark-patch-1 branch October 24, 2019 21:52

paulirish pushed a commit that referenced this pull request Nov 6, 2019

tests(viewer): don't override puppeteer's chromium (#9877)

41dd404

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(viewer): don't override puppeteer's chromium #9877

tests(viewer): don't override puppeteer's chromium #9877

connorjclark commented Oct 23, 2019

patrickhulce commented Oct 23, 2019

connorjclark commented Oct 23, 2019

brendankenny commented Oct 23, 2019

connorjclark commented Oct 23, 2019

connorjclark commented Oct 23, 2019

connorjclark commented Oct 23, 2019

patrickhulce left a comment

patrickhulce Oct 24, 2019

connorjclark Oct 24, 2019

brendankenny Oct 24, 2019 •

edited

Loading

connorjclark Oct 24, 2019

brendankenny Oct 24, 2019

connorjclark Oct 24, 2019

connorjclark commented Oct 24, 2019

mathiasbynens commented Oct 24, 2019

connorjclark commented Oct 24, 2019

brendankenny left a comment

brendankenny Oct 24, 2019 •

edited

Loading

connorjclark commented Oct 24, 2019

brendankenny left a comment

tests(viewer): don't override puppeteer's chromium #9877

tests(viewer): don't override puppeteer's chromium #9877

Conversation

connorjclark commented Oct 23, 2019

patrickhulce commented Oct 23, 2019

connorjclark commented Oct 23, 2019

brendankenny commented Oct 23, 2019

connorjclark commented Oct 23, 2019

connorjclark commented Oct 23, 2019

connorjclark commented Oct 23, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Oct 24, 2019

Choose a reason for hiding this comment

connorjclark Oct 24, 2019

Choose a reason for hiding this comment

brendankenny Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

connorjclark Oct 24, 2019

Choose a reason for hiding this comment

brendankenny Oct 24, 2019

Choose a reason for hiding this comment

connorjclark Oct 24, 2019

Choose a reason for hiding this comment

connorjclark commented Oct 24, 2019

mathiasbynens commented Oct 24, 2019

connorjclark commented Oct 24, 2019

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

connorjclark commented Oct 24, 2019

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Oct 24, 2019 •

edited

Loading

brendankenny Oct 24, 2019 •

edited

Loading