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(config): specify lighthouse channel #7312

Merged
merged 17 commits into from
Mar 7, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions clients/devtools-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ function getDefaultConfigForCategories(categoryIDs) {
function runLighthouseInWorker(port, url, flags, categoryIDs) {
// Default to 'info' logging level.
flags.logLevel = flags.logLevel || 'info';
flags.channel = 'devtools';
const config = getDefaultConfigForCategories(categoryIDs);
const connection = new RawProtocol(port);

Expand Down
1 change: 1 addition & 0 deletions clients/extension/scripts/extension-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ async function runLighthouseInExtension(flags, categoryIDs) {
// Default to 'info' logging level.
flags.logLevel = flags.logLevel || 'info';
flags.output = 'html';
flags.channel = 'extension';

const connection = new ExtensionProtocol();
const url = await connection.getCurrentTabURL();
Expand Down
1 change: 1 addition & 0 deletions clients/lightrider-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
// disableStorageReset because it causes render server hang
flags.disableStorageReset = true;
flags.logLevel = flags.logLevel || 'info';
flags.channel = 'lr';
Copy link
Member

Choose a reason for hiding this comment

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

can assert this in lightrider-entry-test.js


const config = lrDevice === 'desktop' ? LR_PRESETS.desktop : LR_PRESETS.mobile;
if (categoryIDs) {
Expand Down
7 changes: 7 additions & 0 deletions clients/test/extension/extension-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('Lighthouse chrome extension', function() {
let browser;
let extensionPage;
let originalManifest;
let lhr;

function getAuditElementsIds({category, selector}) {
return extensionPage.evaluate(
Expand Down Expand Up @@ -102,6 +103,8 @@ describe('Lighthouse chrome extension', function() {
throw new Error(lighthouseResult.exceptionDetails.text);
}

lhr = lighthouseResult.result.value.lhr;

extensionPage = (await browser.pages()).find(page =>
page.url().includes('blob:chrome-extension://')
);
Expand Down Expand Up @@ -176,4 +179,8 @@ describe('Lighthouse chrome extension', function() {
// this audit has regressed in the extension twice, so make sure it passes
assert.ok(await extensionPage.$('#is-crawlable.lh-audit--pass'), 'did not pass is-crawlable');
});

it('should specify the channel as extension', async () => {
assert.equal(lhr.configSettings.channel, 'extension');
Copy link
Member

@brendankenny brendankenny Mar 5, 2019

Choose a reason for hiding this comment

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

👍 👍

});
});
15 changes: 15 additions & 0 deletions clients/test/lightrider-entry-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
'use strict';

const assert = require('assert');
const jest = require('jest-mock');
const lhBackground = require('../lightrider-entry.js');
const Runner = require('../../lighthouse-core/runner.js');
const LHError = require('../../lighthouse-core/lib/lh-error.js');

/* eslint-env mocha */
Expand Down Expand Up @@ -56,5 +58,18 @@ describe('lightrider-entry', () => {
assert.strictEqual(parsedResult.runtimeError.code, LHError.UNKNOWN_ERROR);
assert.ok(parsedResult.runtimeError.message.includes(errorMsg));
});

it('specifies the channel as lr', async () => {
const runStub = jest.spyOn(Runner, 'run');

const mockConnection = {};
const url = 'https://example.com';

await lhBackground.runLighthouseInLR(mockConnection, url, {}, {});
const config = runStub.mock.calls[0][1].config;
assert.equal(config.settings.channel, 'lr');

runStub.mockRestore();
});
});
});
2 changes: 2 additions & 0 deletions lighthouse-cli/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ function getFlags(manualArgv) {
.array('skipAudits')
.array('output')
.string('extraHeaders')
.string('channel')
.string('precomputedLanternDataPath')
.string('lanternDataOutputPath')

Expand All @@ -146,6 +147,7 @@ function getFlags(manualArgv) {
.default('port', 0)
.default('hostname', 'localhost')
.default('enable-error-reporting', undefined) // Undefined so prompted by default
.default('channel', 'cli')
Copy link
Member

Choose a reason for hiding this comment

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

can assert this in 'runLighthouse completes a LH round trip' in run-test.js

.check(/** @param {LH.CliFlags} argv */ (argv) => {
// Lighthouse doesn't need a URL if...
// - We're just listing the available options.
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -1202,6 +1202,7 @@ Object {
"additionalTraceCategories": null,
"auditMode": false,
"blockedUrlPatterns": null,
"channel": "cli",
"disableDeviceEmulation": false,
"disableStorageReset": false,
"emulatedFormFactor": "mobile",
Expand Down Expand Up @@ -1331,6 +1332,7 @@ Object {
"additionalTraceCategories": null,
"auditMode": true,
"blockedUrlPatterns": null,
"channel": "cli",
"disableDeviceEmulation": false,
"disableStorageReset": false,
"emulatedFormFactor": "mobile",
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-cli/test/cli/run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ describe('CLI run', function() {
assert.deepStrictEqual(results.timing, lhr.timing);
assert.ok(results.timing.total !== 0);

assert.equal(results.configSettings.channel, 'cli');

fs.unlinkSync(filename);
});
}, 20 * 1000);
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/config/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const defaultSettings = {
disableStorageReset: false,
disableDeviceEmulation: false,
emulatedFormFactor: 'mobile',
channel: 'node',
Copy link
Member

Choose a reason for hiding this comment

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

and can assert this in lighthouse-core/test/index-test.js (basically just copy 'should return formatted LHR when given no categories' and delete most of it :)


// the following settings have no defaults but we still want ensure that `key in settings`
// in config will work in a typechecked way
Expand Down
23 changes: 23 additions & 0 deletions lighthouse-core/test/index-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,29 @@ describe('Module Tests', function() {
});
});

it('should specify the channel as node by default', async function() {
const exampleUrl = 'https://www.reddit.com/r/nba';
const results = await lighthouse(exampleUrl, {}, {
settings: {
auditMode: __dirname + '/fixtures/artifacts/perflog/',
},
audits: [],
});
assert.equal(results.lhr.configSettings.channel, 'node');
});

it('lets consumers pass in a custom channel', async function() {
const exampleUrl = 'https://www.reddit.com/r/nba';
const results = await lighthouse(exampleUrl, {}, {
settings: {
auditMode: __dirname + '/fixtures/artifacts/perflog/',
channel: 'custom',
},
audits: [],
});
assert.equal(results.lhr.configSettings.channel, 'custom');
});

it('should return a list of audits', function() {
assert.ok(Array.isArray(lighthouse.getAuditList()));
});
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -3175,6 +3175,7 @@
"disableStorageReset": false,
"disableDeviceEmulation": false,
"emulatedFormFactor": "mobile",
"channel": "cli",
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

should it also be in the top level of the LHR? Maybe inside environment, next to benchmarkIndex, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a benefit over using lhr.configSettings.channel?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a benefit over using lhr.configSettings.channel?

because it's not really a setting, but now it kind of is, so that's fine

"locale": "en-US",
"blockedUrlPatterns": null,
"additionalTraceCategories": null,
Expand Down
3 changes: 3 additions & 0 deletions proto/lighthouse-result.proto
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ message LighthouseResult {
// List of the categories that were run, empty if all were run
// nullable list of strings
google.protobuf.Value only_categories = 3;

// How Lighthouse was run, e.g. from the Chrome extension or from the npm module
string channel = 4;
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
}

// The settings that were used to run this audit
Expand Down
2 changes: 2 additions & 0 deletions types/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ declare global {
skipAudits?: string[] | null;
/** List of extra HTTP Headers to include. */
extraHeaders?: Crdp.Network.Headers | null; // See extraHeaders TODO in bin.js
/** How Lighthouse was run, e.g. from the Chrome extension or from the npm module */
channel?: string
/** Precomputed lantern estimates to use instead of observed analysis. */
precomputedLanternData?: PrecomputedLanternData | null;
}
Expand Down