extension(tests): Add extension smoketest #4640

wardpeet · 2018-02-27T21:09:44Z

This PR will run the extension in chrome and checks if the report has all necessary fields listed.

the manifest change is needed because puppeteer can't detect the active tab. We need to add tabs to the permission list, we could temporary add it for each run so we don't have to update our manifest.

Also a caveat, we need to run puppeteer in non headless mode to let the extensions work

wardpeet · 2018-02-27T21:21:15Z

Fixes #4479

paulirish

really nice job. clean impl and super jazzed you got this running on travis no problem. :)

so so good.

mostly style feedback, but the approach is aces. ♠️

paulirish · 2018-03-01T00:02:02Z

lighthouse-extension/test/extension.js

+ return browser.pages().then(pages => {
+ const page = pages.find(page => page.url().includes('blob:chrome-extension://'));
+ const assertAudits = (category, expected, selector) =>
+ page.$eval(`#${category}`,


trying this out to see if it's more clear:

function assertAuditElements(category, expected, selector) { page.$evaluate(({category, selector}) => document.querySelector(`#${category}`).parentNode.querySelectorAll(selector).length, {category, selector} ).then(elementCount => { assert.equal(expected, elementCount); }); }

also, let's use a keyed object for the method signature?

assertAudits({ category: 'performance', count: config.categories.performance.audits.length, selector: '.lh-audit,.lh-timeline-metric,.lh-perf-hint,.lh-filmstrip' });

paulirish · 2018-03-01T00:03:49Z

lighthouse-extension/test/extension.js

+ `--disable-extensions-except=${lighthouseExtensionPath}`,
+ `--load-extension=${lighthouseExtensionPath}`,
+ ],
+}).then(browser => {


Let's do async/await for this file. I'm sorry we didn't sort that out beforehand.

But IMO that'll make this guy a lot more readable.

what about node 6 which we run our PR checks on? it doesn't support async/await right?

we're about to drop node 6 support. :) we'll land this patch in the 3.0 side of things.

paulirish · 2018-03-01T00:08:35Z

lighthouse-extension/app/manifest.json

@@ -18,6 +18,7 @@
 },
 "permissions": [
 "activeTab",
+ "tabs",


i remember you telling me, but remind me again why we now need this permission?

let's definitely add this on demand as you suggested @wardpeet, grabbing a new permission might still disable the extension on update?

I don't think we need it. In https://stackoverflow.com/questions/48089670/detect-and-test-chrome-extension-using-puppeteer, I needed it to get the title of tab. We're not doing anything special with the tabs api here.

The reason why I need the extension is the following function

const queryOpts = { active: true, currentWindow: true, }; chrome.tabs.query(queryOpts, (tabs => {

without it I couldn't get the url of the tab

Yessssss. I repro'd this.
Basically I believe activeTab isn't enough because of however we're bypassing the popup and such. It's hitting some corner case where activeTab just isn't enough. +1 to dynamically adding tabs permission just for this case.

paulirish · 2018-03-01T00:08:56Z

.travis.yml

 - yarn bundlesize
 - yarn lint
 - yarn unit
 - yarn type-check
 - yarn closure
 - yarn smoke
 - yarn smokehouse
+ - node lighthouse-extension/test/extension.js


let's add an npm scripts entry to the extensions package.json so it's easier to run on demand

cd lighthouse-extension; yarn test:e2e

ebidel

Pretty sweet!

ebidel · 2018-02-28T23:51:39Z

lighthouse-extension/app/manifest.json

@@ -18,6 +18,7 @@
 },
 "permissions": [
 "activeTab",
+ "tabs",


Won't this change the permission model and reprompt users to accept the changes. We saw a lot of uninstalls last time we changed the crx permissions.

I could change the config just during the test so we don't need to change it for installations?

ebidel · 2018-02-28T23:52:28Z

.travis.yml

 - yarn
 # travis can't handle the parallel install (without caches)
 - yarn run install-all:task:windows
 before_script:
 - export DISPLAY=:99.0
- - export CHROME_PATH="$(pwd)/chrome-linux/chrome"
+ - export CHROME_PATH="$(which google-chrome-stable)"


if you skip the pptr chromium download, is this at least chrome 66. PPTR may need that version to work properly.

I just thought it was a bit stupid to install it twice and we moved to chrome stable to test the lowest version we support. I could enable chromium again.

well, it does appear to work fine with chrome stable.
I'm also fine with using the bundled Chromium, but yes it does seem wasteful to d/l each time.

If it works fine with chrome stable as of now, then moving forward (assuming we infrequently bump the pptr version), it shouldn't regress. So I'm +1 to using the stable.

ebidel · 2018-02-28T23:54:09Z

lighthouse-extension/test/extension.js

+
+puppeteer.launch({
+ headless: false,
+ executablePath: process.env.CHROME_PATH ? process.env.CHROME_PATH : undefined,


can just be: executablePath: process.env.CHROME_PATH,

ebidel · 2018-02-28T23:55:59Z

lighthouse-extension/test/extension.js

+ ],
+}).then(browser => {
+ browser.newPage()
+ .then(page => {


nit .then(page => page.goto('https://www.paulirish.com', {waitUntil: 'networkidle2'})

ebidel · 2018-02-28T23:56:59Z

lighthouse-extension/test/extension.js

+ return page.goto('https://www.paulirish.com', {waitUntil: 'networkidle2'});
+ })
+ .then(() => browser.targets())
+ .then(targets => {


Man ...it's been a while since I haven't seen await/async. pptr code look especially painful without it :)

ebidel · 2018-02-28T23:57:19Z

lighthouse-extension/test/extension.js

+ .then(() => browser.targets())
+ .then(targets => {
+ const extensionTarget = targets.find(target => {
+ return target._targetInfo.title === 'Lighthouse';


nit: code be 1 line

IMO should also check _targetInfo.type === 'background_page'
not a one-liner any more. sorryyy

ebidel · 2018-03-01T00:02:55Z

lighthouse-extension/test/extension.js

+ });
+
+ if (extensionTarget) {
+ extensionTarget.createCDPSession()


Any way to use puppeteer's evaluate method(s) here

https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#executioncontextevaluatepagefunction-args maybe?

await ctx.evaluate(() => { return runLighthouseInExtension({restoreCleanState: true}, ['performance', 'pwa', 'accessibility', 'best-practices', 'seo']); });

Whatever that looks like. Not sure if you can get the crx context, other than what you're doing using an private api.

sounds neat, will try :)

yup +1 to using this rather than dropping to the protocol. good call.

@ebidel not sure how I can get that execution context from the Target class. I tried .page().mainFrame().executionContext() but page() is of course null.

Any ideas? 😛

ebidel · 2018-03-01T00:05:14Z

lighthouse-extension/test/extension.js

+ return browser.pages().then(pages => {
+ const page = pages.find(page => page.url().includes('blob:chrome-extension://'));
+ const assertAudits = (category, expected, selector) =>
+ page.$eval(`#${category}`,


Having a hard time parsing this. Maybe just use function. And I think selector can just be passed as a string, not in in an obj:

const assertNumAuditsEqual = function(el, selector) { return page.$eval(`#${category}`, (el, selector) => el.parentNode.querySelectorAll(selector).length, selector)); }

ebidel · 2018-03-01T00:11:50Z

lighthouse-extension/test/extension.js

+ '.lh-audit,.lh-timeline-metric,.lh-perf-hint,.lh-filmstrip'
+ );
+ })
+ .then(


the formatting of these then's should be the same. I may also break this testing section into a separate function if you can.

just left comments.

paulirish · 2018-03-01T21:09:54Z

lighthouse-extension/test/extension.js

+ .then(() => browser.targets())
+ .then(targets => {
+ const extensionTarget = targets.find(target => {
+ return target._targetInfo.title === 'Lighthouse';


IMO should also check _targetInfo.type === 'background_page'
not a one-liner any more. sorryyy

paulirish · 2018-03-01T21:10:31Z

lighthouse-extension/test/extension.js

+ return target._targetInfo.title === 'Lighthouse';
+ });
+
+ if (extensionTarget) {


i know the async/await will change this completely, but IMO you should early exit on the ! case here instead.

paulirish · 2018-03-01T21:14:07Z

lighthouse-extension/test/extension.js

+ return client;
+ })
+ .then(client => {
+ return client.send(


this can return obj.exceptionDetails which means the evaluation failed. if we get that, we need to log the error at least.

paulirish · 2018-03-01T22:23:27Z

lighthouse-extension/app/manifest.json

@@ -18,6 +18,7 @@
 },
 "permissions": [
 "activeTab",
+ "tabs",


Yessssss. I repro'd this.
Basically I believe activeTab isn't enough because of however we're bypassing the popup and such. It's hitting some corner case where activeTab just isn't enough. +1 to dynamically adding tabs permission just for this case.

wardpeet · 2018-03-06T08:12:41Z

.eslintrc.js

@@ -68,7 +68,7 @@ module.exports = {
 'arrow-parens': 0,
 },
 parserOptions: {
- ecmaVersion: 6,
+ ecmaVersion: 2017,


this is needed for eslint async/await

paulirish · 2018-03-07T03:09:39Z

lighthouse-extension/package-lock.json

@@ -0,0 +1,5979 @@
+{


we're still yarn folk so we should be updating the yarn lock and not adding this, right?
or did you already point out why we need this and i'm forgetting?

no no, it's my mistake . It shouldn't be here. I use yarn in WSL but npm on powershell because i'm to lazy to install yarn on windows as I only need it to run puppeteer (i'll probably create a bug for that in the puppeteer repository).

fwiw npm install -g yarn is what we do on appveyor. probably not too bad.

also i think pptr is moving to package-lock soon.

paulirish · 2018-03-07T03:10:22Z

lighthouse-extension/test/extension.js

+ _targetInfo.type === 'background_page';
+ });
+
+ if (extensionTarget) {


early exit instead

paulirish · 2018-03-07T03:12:14Z

lighthouse-extension/test/extension.js

+const config = require(path.resolve(__dirname, '../../lighthouse-core/config/default.js'));
+const lighthouseCategories = Object.keys(config.categories);
+
+const manifestLocation = path.join(lighthouseExtensionPath, 'manifest.json');


can you pull this stuff into a little named function? might as well create another for putting the orig back.

actually if we use mocha to run this test, then we can use before/after methods for these things.

seems fairly reasonable to use mocha at this point

any idea how I should name this test? describe and it text😛.

describe('lighthouse chrome extension', ...
it('completes an end-to-end run', ...

paulirish · 2018-03-07T03:14:17Z

lighthouse-extension/test/extension.js

+ extensionPage = (await browser.pages())
+ .find(page => page.url().includes('blob:chrome-extension://'));
+
+ const categories = await extensionPage.$$(`#${lighthouseCategories.join(',#')}`);


i'd pull 80-94 into a separate function, so it's clear that this is the test assertions against the report page

brendankenny · 2018-03-08T21:12:34Z

lighthouse-extension/package.json

@@ -22,6 +23,7 @@
 "gulp-util": "^3.0.7",
 "gulp-zip": "^3.2.0",
 "package-json-versionify": "^1.0.4",
+ "puppeteer": "^1.1.1",


what do folks think about moving puppeteer up to a root-level dev dependency? As I was testing viewer yesterday I was thinking it would be great to have a similar (but should be much simpler) e2e test for viewer, and it seems a bit silly to have a copy here and in viewer when yarn install-all would install both anyway...

Great idea. Viewer UI testing is the perfect use case for 🤹‍♀️

googlebot · 2018-03-10T03:01:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

wardpeet · 2018-03-12T06:17:04Z

@paulirish yarn compile-devtools is failing. How can I fix that?

paulirish · 2018-03-20T16:44:30Z

Can you add in an assertion that the report doesn't include the text "Audit error"? Maybe grab all .lh-error and look within their textContent?

If we have this we would have caught #4794

paulirish · 2018-03-20T16:56:48Z

lighthouse-extension/test/extension-test.js

+ });
+
+ it('should contain a filmstrip', async () => {
+ const filmstrip = await extensionPage.$('lh-filmstrip');


missing a dot in the selector here?

i think this sorts it out:

it('should contain a filmstrip', async () => { const filmstrip = await extensionPage.$('.lh-filmstrip'); assert.equal(true, !!filmstrip, `filmstrip is not available`); });

paulirish · 2018-03-20T16:57:14Z

lighthouse-extension/test/extension-test.js

+ it('should contain a filmstrip', async () => {
+ const filmstrip = await extensionPage.$('lh-filmstrip');
+
+ assert.equal(null, filmstrip,


wait a second... null != page contains a filmstrip... :p

paulirish · 2018-03-20T16:57:46Z

lighthouse-extension/test/extension-test.js

+ const filmstrip = await extensionPage.$('lh-filmstrip');
+
+ assert.equal(null, filmstrip,
+ `filmstrip is not available`);


this can probably be 1 line

paulirish · 2018-03-20T18:24:47Z

lighthouse-extension/test/extension-test.js

+
+ it('should contain no failed audits', async () => {
+ const auditErrors = await extensionPage.$$('.lh-debug');
+ const failedAudits = auditErrors.filter(error =>


heh. I ran into a similar API misunderstanding. I was doing $$eval(selector, el => ...) but its actually elems instead of just el. Whoopsie daisy.

try something like this out:

it('should not have any audit errors', async () => { function getDebugStrings(elems) { return elems.map(el => ({ debugString: el.textContent, title: el.closest('.lh-audit').querySelector('.lh-score__title').textContent, })); } const auditErrors = await extensionPage.$$eval('.lh-debug', getDebugStrings); const errors = auditErrors.filter(item => item.debugString.includes('Audit error:')); assert.deepStrictEqual(errors, [], 'Audit errors found within the report'); });

gives a nice purdy error:

paulirish · 2018-03-20T23:24:33Z

🎉 this is epic, @wardpeet. thx for making this happen. :D

devtools-bot added the waiting4reviewer label Feb 27, 2018

wardpeet changed the title ~~Add extension smoketest~~ extension(tests): Add extension smoketest Feb 27, 2018

wardpeet requested review from paulirish and ebidel February 27, 2018 21:20

paulirish requested changes Mar 1, 2018

View reviewed changes

ebidel previously requested changes Mar 1, 2018

View reviewed changes

paulirish added waiting4committer and removed waiting4reviewer labels Mar 1, 2018

paulirish reviewed Mar 1, 2018

View reviewed changes

paulirish added the 3.0 label Mar 2, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Mar 6, 2018

wardpeet commented Mar 6, 2018

View reviewed changes

paulirish mentioned this pull request Mar 7, 2018

misc: drop support of Node 6 #4703

Merged

paulirish requested changes Mar 7, 2018

View reviewed changes

paulirish added waiting4committer and removed waiting4reviewer labels Mar 7, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Mar 7, 2018

brendankenny reviewed Mar 8, 2018

View reviewed changes

paulirish added waiting4committer and removed waiting4reviewer labels Mar 8, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Mar 8, 2018

wardpeet force-pushed the tests/lighthouse-extension branch from 6188f59 to b1a6fff Compare March 9, 2018 22:02

GoogleChrome deleted a comment from ebidel Mar 9, 2018

revert manifest

644df57

wardpeet force-pushed the tests/lighthouse-extension branch from 230c343 to 644df57 Compare March 20, 2018 05:51

Add audit error test

5f10d57

paulirish requested changes Mar 20, 2018

View reviewed changes

wardpeet and others added 2 commits March 20, 2018 19:39

Thanks paul!

0722566

Merge branch 'master' into tests/lighthouse-extension

c3ce3f9

paulirish approved these changes Mar 20, 2018

View reviewed changes

wardpeet and others added 9 commits March 20, 2018 19:48

Fix mocha unit test

ff432b0

Extra check for el.closest

a29d829

Format all the things!

d1b6baa

fix test

4f1e160

fix textcontent of null (thanks paul!)

c8ecaf9

const the selectors

6409d26

no tabs permission

198d47f

use dist extension.

e20717c

ignore timing info failures for now.

259441f

paulirish force-pushed the tests/lighthouse-extension branch from 8c71734 to 259441f Compare March 20, 2018 22:51

paulirish merged commit e90f9ac into master Mar 20, 2018

paulirish deleted the tests/lighthouse-extension branch March 20, 2018 23:24

paulirish pushed a commit that referenced this pull request Mar 22, 2018

extension(tests): add extension pptr smoketest (#4640)

b1b1b60

paulirish added the merged:2.x label Mar 22, 2018

paulirish mentioned this pull request Mar 26, 2018

Integration testing for the Chrome extension #4479

Closed

brendankenny mentioned this pull request Apr 24, 2018

tests(viewer): add pptr test for viewer #5025

Merged

paulirish removed the waiting4reviewer label Apr 30, 2018

paulirish removed the 3.0 label Dec 18, 2018

brendankenny mentioned this pull request Oct 23, 2019

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

Merged

smoralesd mentioned this pull request Nov 5, 2019

fix: adjust extension permissions microsoft/accessibility-insights-web#1613

Closed

3 tasks

rhelmer mentioned this pull request Oct 23, 2020

Run integration tests on CI mozilla-rally/rally-core-addon#55

Closed

extension(tests): Add extension smoketest #4640

extension(tests): Add extension smoketest #4640

Conversation

wardpeet commented Feb 27, 2018

wardpeet commented Feb 27, 2018

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Mar 10, 2018

wardpeet commented Mar 12, 2018

paulirish commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Mar 20, 2018