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

tests: run most smoke tests on devtools #13456

Merged
merged 37 commits into from
Jan 5, 2022
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
b1bf3ff
tests: run smokehouse on devtools
connorjclark Dec 2, 2021
036c7f6
tests: fix devtools build command
connorjclark Dec 2, 2021
119f17a
Merge branch 'dt-test-fix-roll' into smoke-devtools
connorjclark Dec 2, 2021
73735d4
Merge remote-tracking branch 'origin/master' into smoke-devtools
connorjclark Dec 3, 2021
29d5b9e
fix path
connorjclark Dec 3, 2021
397eff0
update
connorjclark Dec 4, 2021
71cf0d0
custom devtools path
connorjclark Dec 4, 2021
f2bd19a
update
connorjclark Dec 15, 2021
c947f8d
observeTargets
connorjclark Dec 15, 2021
9a47829
tweak
connorjclark Dec 15, 2021
9022261
testing
connorjclark Dec 15, 2021
bbe7de5
ugh
connorjclark Dec 16, 2021
7d1ab09
Merge remote-tracking branch 'origin/master' into smoke-devtools
connorjclark Dec 20, 2021
98f36e4
more logging
connorjclark Dec 20, 2021
d371fd1
test
connorjclark Dec 30, 2021
30a3da3
fix path cuz mac
connorjclark Dec 30, 2021
8136727
support mac tot
connorjclark Dec 30, 2021
0c39ed1
tweak
connorjclark Dec 30, 2021
4a1ea69
fix path
connorjclark Dec 30, 2021
ae5258a
no xvfb
connorjclark Dec 30, 2021
c01b920
for xmas i want a local gha runner
connorjclark Dec 30, 2021
c701b3a
trying run-devtools
connorjclark Dec 30, 2021
747eab1
mkdir
connorjclark Dec 30, 2021
35c77a6
logggg
connorjclark Dec 30, 2021
eba8d0b
targetManager
connorjclark Dec 30, 2021
97542c1
maybe ok now
connorjclark Dec 30, 2021
4367c71
logs
connorjclark Dec 31, 2021
5e4360c
check for headless
connorjclark Dec 31, 2021
b26652c
log
connorjclark Dec 31, 2021
9f17ee2
fix frontend path
connorjclark Dec 31, 2021
9ccb3f5
remove bad apples
connorjclark Dec 31, 2021
32546f7
ascii
connorjclark Jan 4, 2022
fd6937c
comment
connorjclark Jan 4, 2022
b79270d
add beforeAll to smoke runner options. build devtools locally for dev…
connorjclark Jan 4, 2022
b552400
update
connorjclark Jan 5, 2022
b056663
pr
connorjclark Jan 5, 2022
12a060c
ts
connorjclark Jan 5, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/workflows/devtools.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,19 @@ jobs:
with:
name: results
path: ${{ github.workspace }}/lighthouse/.tmp/layout-test-results

# Run smoke tests via DevTools
Copy link
Member

Choose a reason for hiding this comment

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

Better in a new job?

Copy link
Collaborator Author

@connorjclark connorjclark Dec 3, 2021

Choose a reason for hiding this comment

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

How to avoid redoing all the work for building devtools?

Copy link
Member

@brendankenny brendankenny Jan 5, 2022

Choose a reason for hiding this comment

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

How to avoid redoing all the work for building devtools?

Yeah it's not pretty. We could have dependent jobs (or cache the build, but dependent jobs seems workable)? I'm not sure of the best approach, but if the devtools CI job is normally going to take 30+ minutes we'll definitely want to split it up.

edit: if an already-built devtools was available, sharding would work as well

- name: Define ToT chrome path
run: echo "CHROME_PATH=/Users/runner/chrome-mac-tot/Chromium.app/Contents/MacOS/Chromium" >> $GITHUB_ENV
- name: Install Chrome ToT
working-directory: /Users/runner
run: bash $GITHUB_WORKSPACE/lighthouse/lighthouse-core/scripts/download-chrome.sh && mv chrome-mac chrome-mac-tot
- run: mkdir latest-run
working-directory: ${{ github.workspace }}/lighthouse
- name: yarn smoke --runner devtools
# TODO: run on all tests.
# - Current DevTools hangs on any page with a service worker.
# https://github.com/GoogleChrome/lighthouse/issues/13396
# - Various other issues that needed investigation.
Copy link
Member

Choose a reason for hiding this comment

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

Can you create an issue to track this second bullet?

run: yarn smoke --runner devtools --invert-match a11y byte-efficiency byte-gzip dbw errors-expired-ssl errors-infinite-loop lantern-idle-callback-short legacy-javascript metrics-tricky-tti metrics-tricky-tti-late-fcp offline-ready offline-sw-broken offline-sw-slow oopif-requests perf-budgets perf-diagnostics-third-party perf-fonts perf-frame-metrics perf-preload perf-trace-elements pwa redirects-client-paint-server redirects-history-push-state redirects-multiple-server redirects-single-client redirects-single-server screenshot seo-passing seo-tap-targets
working-directory: ${{ github.workspace }}/lighthouse
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ yarn-error.log

/chrome-linux/
/chrome-win32/
/chrome-mac/
/chrome.zip

*__pycache__
Expand Down
2 changes: 1 addition & 1 deletion clients/devtools-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ globalThis.Buffer = require('buffer').Buffer;
* If `lighthouse-plugin-publisher-ads` is in the list of
* `categoryIDs` the plugin will also be run.
* Counterpart to the CDT code that sets flags.
* @see https://cs.chromium.org/chromium/src/third_party/devtools-frontend/src/front_end/lighthouse/LighthouseController.js?type=cs&q=%22const+RuntimeSettings%22+f:lighthouse+-f:out&g=0&l=250
* @see https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/lighthouse/LighthouseController.ts;l=280
* @param {Array<string>} categoryIDs
* @param {string} device
* @return {LH.Config.Json}
Expand Down
6 changes: 4 additions & 2 deletions lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const coreTestDefnsPath =
const runnerPaths = {
cli: '../lighthouse-runners/cli.js',
bundle: '../lighthouse-runners/bundle.js',
devtools: '../lighthouse-runners/devtools.js',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
};

/**
Expand Down Expand Up @@ -151,7 +152,7 @@ async function begin() {
},
'runner': {
default: 'cli',
choices: ['cli', 'bundle'],
choices: ['cli', 'bundle', 'devtools'],
describe: 'The method of running Lighthouse',
},
'tests-path': {
Expand Down Expand Up @@ -183,7 +184,7 @@ async function begin() {
if (argv.runner === 'bundle') {
console.log('\n✨ Be sure to have recently run this: yarn build-all');
}
const {runLighthouse} = await import(runnerPath);
const {runLighthouse, beforeAll} = await import(runnerPath);
Copy link
Member

Choose a reason for hiding this comment

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

can remove beforeAll (here and in types/smokehouse.d.ts)


// Find test definition file and filter by requestedTestIds.
let testDefnPath = argv.testsPath || coreTestDefnsPath;
Expand Down Expand Up @@ -216,6 +217,7 @@ async function begin() {
isDebug: argv.debug,
useFraggleRock: argv.fraggleRock,
lighthouseRunner: runLighthouse,
beforeAll,
takeNetworkRequestUrls,
};

Expand Down
98 changes: 98 additions & 0 deletions lighthouse-cli/test/smokehouse/lighthouse-runners/devtools.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @fileoverview A runner that launches Chrome and executes Lighthouse via DevTools.
*/

import fs from 'fs';
import os from 'os';
import {spawn} from 'child_process';

import {LH_ROOT} from '../../../../root.js';

const devtoolsDir =
process.env.DEVTOOLS_PATH || `${LH_ROOT}/.tmp/chromium-web-tests/devtools/devtools-frontend`;

/**
* @param {string} command
* @param {string[]} args
*/
async function spawnAndLog(command, args) {
let log = '';

/** @type {Promise<void>} */
const promise = new Promise((resolve, reject) => {
const spawnHandle = spawn(command, args);
spawnHandle.on('close', code => {
if (code === 0) resolve();
else reject(new Error(`Command exited with code ${code}`));
});
spawnHandle.on('error', reject);
spawnHandle.stdout.on('data', data => {
console.log(data.toString());
Copy link
Member

Choose a reason for hiding this comment

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

are these lines intended to stay or for debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, otherwise the cases where things hang or error aren't printed (the runner interface only allows returning successful logs). and it's nicer to see the output stream in.

Copy link
Member

@brendankenny brendankenny Jan 5, 2022

Choose a reason for hiding this comment

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

just checking that it's intentional. The tradeoff is that concurrent runs will be streaming in stuff all interleaved with each other and hard to tell the source of each line, but that can still be helpful, especially when things are new and need to check that it's actually running, etc

log += `STDOUT: ${data.toString()}`;
});
spawnHandle.stderr.on('data', data => {
console.log(data.toString());
log += `STDERR: ${data.toString()}`;
});
});
await promise;

return log;
}

/** @type {Promise<void>} */
let beforeAllPromise;
Copy link
Member

Choose a reason for hiding this comment

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

maybe

Suggested change
let beforeAllPromise;
let buildDevtoolsPromise;

or something like that?

async function beforeAll() {
Copy link
Member

Choose a reason for hiding this comment

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

can you add a jsdoc like, "Build the lighthouse DevTools bundle and fetch and build the DevTools frontend using it" or whatever?

if (process.env.CI) return;

process.env.DEVTOOLS_PATH = devtoolsDir;
await spawnAndLog('bash', ['lighthouse-core/test/chromium-web-tests/download-devtools.sh']);
await spawnAndLog('bash', ['lighthouse-core/test/chromium-web-tests/roll-devtools.sh']);
}

/**
* Launch Chrome and do a full Lighthouse run via DevTools.
Copy link
Member

Choose a reason for hiding this comment

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

I'm having trouble coming up with a non-awkward way to say this, but seems worth clarifying it's the current/local Lighthouse devtools bundle run in the latest DevTools frontend
vs
latest DevTools frontend with whatever Lighthouse version was last rolled into Chromium.

* @param {string} url
* @param {LH.Config.Json=} configJson
* @param {{isDebug?: boolean}=} testRunnerOptions
* @return {Promise<{lhr: LH.Result, artifacts: LH.Artifacts, log: string}>}
*/
async function runLighthouse(url, configJson, testRunnerOptions = {}) {
if (!beforeAllPromise) beforeAllPromise = beforeAll();
await beforeAllPromise;

const outputDir = fs.mkdtempSync(os.tmpdir() + '/lh-smoke-cdt-runner-');
Copy link
Member

Choose a reason for hiding this comment

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

Does this create multiple temp directories if the smokes are running concurrently?

Copy link
Member

@brendankenny brendankenny Jan 5, 2022

Choose a reason for hiding this comment

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

yeah, fs.mkdtempSync handles that

const args = [
'run-devtools',
url,
`--custom-devtools-frontend=file://${devtoolsDir}/out/Default/gen/front_end`,
'--output-dir', outputDir,
Copy link
Member

Choose a reason for hiding this comment

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

We could use the custom DT frontend flag to test how PR changes affect DT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I keep making this mistake, thinking that is the default behavior. It is for open-devtools! I want to followup with a change that does that.

Will add the arg here for now.

];
if (configJson) {
args.push('--config', JSON.stringify(configJson));
}

const log = await spawnAndLog('yarn', args);
const lhr = JSON.parse(fs.readFileSync(`${outputDir}/lhr-0.json`, 'utf-8'));
const artifacts = JSON.parse(fs.readFileSync(`${outputDir}/artifacts-0.json`, 'utf-8'));

if (testRunnerOptions.isDebug) {
console.log(`${url} results saved at ${outputDir}`);
} else {
fs.rmSync(outputDir, {recursive: true, force: true});
}

return {lhr, artifacts, log};
}

export {
beforeAll,
runLighthouse,
};
18 changes: 10 additions & 8 deletions lighthouse-cli/test/smokehouse/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,15 @@ Smokehouse Frontends Lighthouse Runners
| | | | | <lhr | +--------------+
+------------+ | +-------+-------+ | | |
| ^ +-->+ bundle.js |
+------------+ | | | |
| | | | +--------------+
| lib.js +----+ v
| | +--------+--------+
+------------+ | |
| report/assert |
| |
+-----------------+
+------------+ | | | | |
| | | | | +--------------+
| lib.js +----+ v |
| | +--------+--------+ |
+------------+ | | | +--------------+
| report/assert | | | |
| | +-->+ devtools.js |
+-----------------+ | |
+--------------+
```

### Smokehouse frontends
Expand All @@ -142,6 +143,7 @@ Smokehouse Frontends Lighthouse Runners

- `lighthouse-runners/cli.js` - the original test runner, exercising the Lighthouse CLI from command-line argument parsing to the results written to disk on completion.
- `lighthouse-runners/bundle.js` - a smoke test runner that operates on an already-bundled version of Lighthouse for end-to-end testing of that version.
- `lighthouse-runners/devtools.js` - a smoke test runner that operates on Lighthouse running from inside DevTools.

## Custom smoke tests (for plugins et al.)

Expand Down
17 changes: 15 additions & 2 deletions lighthouse-core/scripts/download-chrome.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,23 @@

set -euo pipefail

if [ "$OSTYPE" == "msys" ]; then
unameOut="$(uname -s)"
case "${unameOut}" in
Linux*) machine=Linux;;
Darwin*) machine=Mac;;
MINGW*) machine=MinGw;;
*) machine="UNKNOWN:${unameOut}"
esac

if [ "$machine" == "MinGw" ]; then
url="https://download-chromium.appspot.com/dl/Win?type=snapshots"
else
elif [ "$machine" == "Linux" ]; then
url="https://download-chromium.appspot.com/dl/Linux_x64?type=snapshots"
elif [ "$machine" == "Mac" ]; then
url="https://download-chromium.appspot.com/dl/Mac?type=snapshots"
else
echo "unsupported platform"
exit 1
fi

if [ -e "$CHROME_PATH" ]; then
Expand Down
Loading