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(devtools): ensure Lighthouse starts in smoke tests #15459

Merged
merged 11 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/devtools.yml
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ jobs:
- run: mkdir latest-run
working-directory: ${{ github.workspace }}/lighthouse
- name: yarn smoke --runner devtools
run: xvfb-run --auto-servernum yarn smoke --runner devtools --shard=${{ matrix.smoke-test-shard }}/${{ strategy.job-total }} --jobs=3 --retries=2
run: xvfb-run --auto-servernum yarn smoke --runner devtools --shard=${{ matrix.smoke-test-shard }}/${{ strategy.job-total }} --jobs=3 --retries=2 --debug
working-directory: ${{ github.workspace }}/lighthouse

- name: Upload failures
Expand Down
4 changes: 3 additions & 1 deletion cli/test/smokehouse/lighthouse-runners/devtools.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,17 @@ async function setup() {
* CHROME_PATH determines which Chrome is used–otherwise the default is puppeteer's chrome binary.
* @param {string} url
* @param {LH.Config=} config
* @param {{isDebug?: boolean}=} options
* @return {Promise<{lhr: LH.Result, artifacts: LH.Artifacts, log: string}>}
*/
async function runLighthouse(url, config) {
async function runLighthouse(url, config, options) {
const chromeFlags = [
`--custom-devtools-frontend=file://${devtoolsDir}/out/LighthouseIntegration/gen/front_end`,
];
const {lhr, artifacts, logs} = await testUrlFromDevtools(url, {
config,
chromeFlags,
detailedError: options?.isDebug,
});

const log = logs.join('') + '\n';
Expand Down
12 changes: 9 additions & 3 deletions cli/test/smokehouse/smokehouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

/* eslint-disable no-console */

/** @typedef {import('./lib/child-process-error.js').ChildProcessError} ChildProcessError */

/**
* @typedef Run
* @property {string[] | undefined} networkRequests
Expand All @@ -39,6 +37,7 @@ import {runLighthouse as cliLighthouseRunner} from './lighthouse-runners/cli.js'
import {getAssertionReport} from './report-assert.js';
import {LocalConsole} from './lib/local-console.js';
import {ConcurrentMapper} from './lib/concurrent-mapper.js';
import {ChildProcessError} from './lib/child-process-error.js';

// The number of concurrent (`!runSerially`) tests to run if `jobs` isn't set.
const DEFAULT_CONCURRENT_RUNS = 5;
Expand Down Expand Up @@ -239,11 +238,18 @@ async function runSmokeTest(smokeTestDefn, testOptions) {
* @param {ChildProcessError|Error} err
*/
function logChildProcessError(localConsole, err) {
if ('stdout' in err && 'stderr' in err) {
if (err instanceof ChildProcessError) {
localConsole.adoptStdStrings(err);
}

localConsole.log(log.redify(err.stack || err.message));
if (err.cause) {
if (err.cause instanceof Error) {
localConsole.log(log.redify(`[cause] ${err.cause.stack || err.cause.message}`));
} else {
localConsole.log(log.redify(`[cause] ${err.cause}`));
}
}
}

/**
Expand Down
31 changes: 27 additions & 4 deletions core/scripts/pptr-run-devtools.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,18 @@ async function runLighthouse() {
);
});

const button = panel.contentElement.querySelector('button');
button.click();
// In CI clicking the start button just once is flaky and can cause a timeout.
// Therefore, keep clicking the button until we detect that the run started.
const intervalHandle = setInterval(() => {
const button = panel.contentElement.querySelector('button');
button.click();
}, 100);

addSniffer(
panel.__proto__,
'handleCompleteRun',
() => clearInterval(intervalHandle),
);

return resultPromise;
}
Expand Down Expand Up @@ -286,7 +296,7 @@ function dismissDialog(dialog) {

/**
* @param {string} url
* @param {{config?: LH.Config, chromeFlags?: string[]}} [options]
* @param {{config?: LH.Config, chromeFlags?: string[], detailedError?: boolean}} [options]
* @return {Promise<{lhr: LH.Result, artifacts: LH.Artifacts, logs: string[]}>}
*/
async function testUrlFromDevtools(url, options = {}) {
Expand All @@ -299,6 +309,9 @@ async function testUrlFromDevtools(url, options = {}) {
defaultViewport: null,
});

/** @type {puppeteer.CDPSession|undefined} */
let inspectorSession;

try {
if ((await browser.version()).startsWith('Headless')) {
throw new Error('You cannot use headless');
Expand All @@ -307,7 +320,7 @@ async function testUrlFromDevtools(url, options = {}) {
const page = (await browser.pages())[0];

const inspectorTarget = await browser.waitForTarget(t => t.url().includes('devtools'));
const inspectorSession = await inspectorTarget.createCDPSession();
inspectorSession = await inspectorTarget.createCDPSession();

/** @type {string[]} */
const logs = [];
Expand All @@ -328,6 +341,16 @@ async function testUrlFromDevtools(url, options = {}) {
const result = await evaluateInSession(inspectorSession, runLighthouse, [addSniffer]);

return {...result, logs};
} catch (err) {
if (options.detailedError && inspectorSession) {
Copy link
Collaborator

@connorjclark connorjclark Sep 18, 2023

Choose a reason for hiding this comment

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

why not always do this? debug mode is more about reducing output noise, but giving context on failure is always useful

Copy link
Member Author

Choose a reason for hiding this comment

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

The screenshot URL is very long compared to other output. This isn't really "noise" for the reason you mentioned but it is extra debug information. Since this is for errors and not expectation differences it makes more sense to always show the screenshot.

Copy link
Collaborator

@connorjclark connorjclark Sep 18, 2023

Choose a reason for hiding this comment

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

Could it save to disk in .tmp and just print the path? And then debug mode would print the contents too (for purposes of CI..) ... or we could upload a folder called .tmp/debug-info.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ehh I like having the screenshot easily accessible in CI logs. I don't want to have to download CI failure artifacts if I don't have to.

Plus, this file is organized to be called by a smoke runner but isn't aware of any smoke artifact folders itself.

const {data} = await inspectorSession.send('Page.captureScreenshot', {format: 'webp'});
const image = `data:image/webp;base64,${data}`;
throw new Error(
`Lighthouse in DevTool failed. DevTools screenshot:\n${image}`,
{cause: err}
);
}
throw err;
} finally {
await browser.close();
}
Expand Down
Loading