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

core(runner): asset manager helper #13519

Merged
merged 15 commits into from
Jan 7, 2022
4 changes: 4 additions & 0 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ async function runLighthouse(url, flags, config) {
let launchedChrome;

try {
if (url && flags.auditMode && !flags.gatherMode) {
log.warn('CLI', 'URL parameter is ignored if -A flag is used without -G flag');
}

const shouldGather = flags.gatherMode || flags.gatherMode === flags.auditMode;
const shouldUseLocalChrome = URL.isLikeLocalhost(flags.hostname);
if (shouldGather && shouldUseLocalChrome) {
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const {
awaitArtifacts,
} = require('./runner-helpers.js');
const prepare = require('../../gather/driver/prepare.js');
const {gotoURL} = require('../../gather/driver/navigation.js');
const {gotoURL, normalizeUrl} = require('../../gather/driver/navigation.js');
const storage = require('../../gather/driver/storage.js');
const emulation = require('../../lib/emulation.js');
const {defaultNavigationConfig} = require('../../config/constants.js');
Expand Down Expand Up @@ -279,7 +279,7 @@ async function _cleanup({requestedUrl, driver, config}) {
* @return {Promise<LH.RunnerResult|undefined>}
*/
async function navigation(options) {
const {url: requestedUrl, page, configContext = {}} = options;
const {url, page, configContext = {}} = options;
const {config} = initializeConfig(options.config, {...configContext, gatherMode: 'navigation'});
const computedCache = new Map();
const internalOptions = {
Expand All @@ -289,6 +289,7 @@ async function navigation(options) {
return Runner.run(
async () => {
const driver = new Driver(page);
const requestedUrl = normalizeUrl(url);
const context = {driver, config, requestedUrl, options: internalOptions};
const {baseArtifacts} = await _setup(context);
const {artifacts} = await _navigations({...context, baseArtifacts, computedCache});
Expand All @@ -297,7 +298,6 @@ async function navigation(options) {
return finalizeArtifacts(baseArtifacts, artifacts);
},
{
url: requestedUrl,
config,
computedCache: new Map(),
}
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/fraggle-rock/gather/snapshot-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ async function snapshot(options) {
return finalizeArtifacts(baseArtifacts, artifacts);
},
{
url,
config,
computedCache,
}
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/fraggle-rock/gather/timespan-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ async function startTimespan(options) {
return finalizeArtifacts(baseArtifacts, artifacts);
},
{
url: finalUrl,
config,
computedCache,
}
Expand Down
17 changes: 16 additions & 1 deletion lighthouse-core/gather/driver/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const {waitForFullyLoaded, waitForFrameNavigated, waitForUserToContinue} = requi
const constants = require('../../config/constants.js');
const i18n = require('../../lib/i18n/i18n.js');
const URL = require('../../lib/url-shim.js');
const LHError = require('../../lib/lh-error.js');

const UIStrings = {
/**
Expand Down Expand Up @@ -157,4 +158,18 @@ function getNavigationWarnings(navigation) {
return warnings;
}

module.exports = {gotoURL, getNavigationWarnings, UIStrings};
/**
* @param {string|undefined} url
* @return {string}
*/
function normalizeUrl(url) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
// Verify the url is valid and that protocol is allowed
if (url && URL.isValid(url) && URL.isProtocolAllowed(url)) {
// Use canonicalized URL (with trailing slashes and such)
return new URL(url).href;
} else {
throw new LHError(LHError.errors.INVALID_URL);
}
}

module.exports = {gotoURL, getNavigationWarnings, normalizeUrl, UIStrings};
7 changes: 4 additions & 3 deletions lighthouse-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const Runner = require('./runner.js');
const log = require('lighthouse-logger');
const ChromeProtocol = require('./gather/connections/cri.js');
const Config = require('./config/config.js');
const {normalizeUrl} = require('./gather/driver/navigation.js');

/** @typedef {import('./gather/connections/connection.js')} Connection */

Expand Down Expand Up @@ -42,12 +43,12 @@ async function lighthouse(url, flags = {}, configJSON, userConnection) {

const config = generateConfig(configJSON, flags);
const computedCache = new Map();
const options = {url, config, computedCache};
const options = {config, computedCache};
const connection = userConnection || new ChromeProtocol(flags.port, flags.hostname);

// kick off a lighthouse run
/** @param {{requestedUrl: string}} runnerData */
const gatherFn = ({requestedUrl}) => {
const gatherFn = () => {
const requestedUrl = normalizeUrl(url);
return Runner._gatherArtifactsFromBrowser(requestedUrl, options, connection);
};
return Runner.run(gatherFn, options);
Expand Down
104 changes: 52 additions & 52 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const stackPacks = require('./lib/stack-packs.js');
const assetSaver = require('./lib/asset-saver.js');
const fs = require('fs');
const path = require('path');
const URL = require('./lib/url-shim.js');
const Sentry = require('./lib/sentry.js');
const generateReport = require('../report/generator/report-generator.js').generateReport;
const LHError = require('./lib/lh-error.js');
Expand All @@ -29,8 +28,8 @@ const {version: lighthouseVersion} = require('../package.json');
class Runner {
/**
* @template {LH.Config.Config | LH.Config.FRConfig} TConfig
* @param {(runnerData: {requestedUrl: string, config: TConfig, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: TConfig, computedCache: Map<string, ArbitraryEqualityMap>, url?: string, driverMock?: Driver}} runOpts
* @param {(runnerData: {config: TConfig, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: TConfig, computedCache: Map<string, ArbitraryEqualityMap>, driverMock?: Driver}} runOpts
* @return {Promise<LH.RunnerResult|undefined>}
*/
static async run(gatherFn, runOpts) {
Expand All @@ -45,54 +44,7 @@ class Runner {
*/
const lighthouseRunWarnings = [];

const sentryContext = Sentry.getContext();
adamraine marked this conversation as resolved.
Show resolved Hide resolved
Sentry.captureBreadcrumb({
message: 'Run started',
category: 'lifecycle',
data: sentryContext?.extra,
});

// User can run -G solo, -A solo, or -GA together
// -G and -A will run partial lighthouse pipelines,
// and -GA will run everything plus save artifacts and lhr to disk.

// Gather phase
// Either load saved artifacts off disk or from the browser
let artifacts;
let requestedUrl;
if (settings.auditMode && !settings.gatherMode) {
// No browser required, just load the artifacts from disk.
const path = Runner._getDataSavePath(settings);
artifacts = assetSaver.loadArtifacts(path);
requestedUrl = artifacts.URL.requestedUrl;

if (!requestedUrl) {
throw new Error('Cannot run audit mode on empty URL');
}
if (runOpts.url && !URL.equalWithExcludedFragments(runOpts.url, requestedUrl)) {
throw new Error('Cannot run audit mode on different URL');
}
Comment on lines -72 to -74
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this check should be the only functional change to legacy mode. I didn't think it was needed because we can proceed with the requestedUrl from the artifacts. I'd be open to adding this back as a non-fatal warning or something in a different location.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The check makes more sense from the context of the CLI. Currently you can do -G and then -A separately but you must use the same URL, otherwise it errors because you probably made a mistake. Perhaps instead, we should have -A mode only use what is saved in the gathering step–url, settings, etc. and throw if a url is also provided

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer just logging a warning, since we could just ignore the passed in URL.

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 fine with just a warning. -G/A is pretty advanced usage so we don't really need these runtime failures to protect us, IMO

Copy link
Collaborator

Choose a reason for hiding this comment

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

A report warning, then, not logger. This will otherwise be ignored and is sure to mess me up eventually so a loud warning is necessary :)

Copy link
Member Author

@adamraine adamraine Dec 22, 2021

Choose a reason for hiding this comment

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

I would prefer switching to a fatal error in the CLI than that. We would need to send the warning message all the way down to Runner.run where run warnings are collected. Removing URL checks from Runner.run is part of the reason I created this PR.

The warning is quiet, but Lighthouse is still performing the more correct and expected operation IMO.

} else {
// verify the url is valid and that protocol is allowed
if (runOpts.url && URL.isValid(runOpts.url) && URL.isProtocolAllowed(runOpts.url)) {
// Use canonicalized URL (with trailing slashes and such)
requestedUrl = new URL(runOpts.url).href;
} else {
throw new LHError(LHError.errors.INVALID_URL);
}

artifacts = await gatherFn({
requestedUrl,
config: runOpts.config,
driverMock: runOpts.driverMock,
});

// -G means save these to ./latest-run, etc.
if (settings.gatherMode) {
const path = Runner._getDataSavePath(settings);
await assetSaver.saveArtifacts(artifacts, path);
}
}
const artifacts = await this.gatherAndManageAssets(gatherFn, runOpts);

// Potentially quit early
if (settings.gatherMode && !settings.auditMode) return;
Expand Down Expand Up @@ -133,7 +85,7 @@ class Runner {
/** @type {LH.RawIcu<LH.Result>} */
const i18nLhr = {
lighthouseVersion,
requestedUrl,
requestedUrl: artifacts.URL.requestedUrl,
finalUrl: artifacts.URL.finalUrl,
fetchTime: artifacts.fetchTime,
gatherMode: artifacts.GatherContext.gatherMode,
Expand Down Expand Up @@ -184,6 +136,54 @@ class Runner {
}
}

/**
* User can run -G solo, -A solo, or -GA together
* -G and -A will run partial lighthouse pipelines,
* and -GA will run everything plus save artifacts and lhr to disk.
*
* @template {LH.Config.Config | LH.Config.FRConfig} TConfig
* @param {(runnerData: {config: TConfig, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: TConfig, driverMock?: Driver}} options
* @return {Promise<LH.Artifacts>}
*/
static async gatherAndManageAssets(gatherFn, options) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const settings = options.config.settings;

const sentryContext = Sentry.getContext();
Sentry.captureBreadcrumb({
message: 'Run started',
category: 'lifecycle',
data: sentryContext?.extra,
});

// Gather phase
// Either load saved artifacts off disk or from the browser
adamraine marked this conversation as resolved.
Show resolved Hide resolved
let artifacts;
if (settings.auditMode && !settings.gatherMode) {
// No browser required, just load the artifacts from disk.
const path = this._getDataSavePath(settings);
artifacts = assetSaver.loadArtifacts(path);
const requestedUrl = artifacts.URL.requestedUrl;

if (!requestedUrl) {
throw new Error('Cannot run audit mode on empty URL');
}
} else {
artifacts = await gatherFn({
config: options.config,
driverMock: options.driverMock,
});

// -G means save these to ./latest-run, etc.
adamraine marked this conversation as resolved.
Show resolved Hide resolved
if (settings.gatherMode) {
const path = this._getDataSavePath(settings);
await assetSaver.saveArtifacts(artifacts, path);
}
}

return artifacts;
}

/**
* This handles both the auditMode case where gatherer entries need to be merged in and
* the gather/audit case where timingEntriesFromRunner contains all entries from this run,
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/test/fraggle-rock/gather/mock-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ function createMockContext() {
}

function mockDriverSubmodules() {
const navigationMock = {gotoURL: jest.fn()};
const navigationMock = {gotoURL: jest.fn(), normalizeUrl: jest.fn()};
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const prepareMock = {
prepareThrottlingAndNetwork: jest.fn(),
prepareTargetForTimespanMode: jest.fn(),
Expand All @@ -237,6 +237,7 @@ function mockDriverSubmodules() {

function reset() {
navigationMock.gotoURL = jest.fn().mockResolvedValue({finalUrl: 'https://example.com', warnings: [], timedOut: false});
navigationMock.normalizeUrl = jest.fn().mockImplementation(url => url);
prepareMock.prepareThrottlingAndNetwork = jest.fn().mockResolvedValue(undefined);
prepareMock.prepareTargetForTimespanMode = jest.fn().mockResolvedValue(undefined);
prepareMock.prepareTargetForNavigationMode = jest.fn().mockResolvedValue({warnings: []});
Expand Down
37 changes: 36 additions & 1 deletion lighthouse-core/test/gather/driver/navigation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
const {createMockDriver, mockTargetManagerModule} = require('../../fraggle-rock/gather/mock-driver.js'); // eslint-disable-line max-len
const targetManagerMock = mockTargetManagerModule();

const {gotoURL, getNavigationWarnings} = require('../../../gather/driver/navigation.js');
const {
gotoURL,
getNavigationWarnings,
normalizeUrl,
} = require('../../../gather/driver/navigation.js');
const {
createMockOnceFn,
makePromiseInspectable,
Expand Down Expand Up @@ -247,3 +251,34 @@ describe('.getNavigationWarnings()', () => {
expect(warnings).toHaveLength(1);
});
});

describe('.normalizeUrl', () => {
it('returns normalized URL', () => {
expect(normalizeUrl('https://example.com')).toEqual('https://example.com/');
});

it('rejects when not given a URL', () => {
expect(() => {
normalizeUrl(undefined);
}).toThrow('INVALID_URL');
});

it('rejects when given a URL of zero length', () => {
expect(() => {
normalizeUrl('');
}).toThrow('INVALID_URL');
});

it('rejects when given a URL without protocol', () => {
expect(() => {
normalizeUrl('localhost');
}).toThrow('INVALID_URL');
});

it('rejects when given a URL without hostname', () => {
expect(() => {
normalizeUrl('https://');
}).toThrow('INVALID_URL');
});
});

Loading