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.

Sign up for GitHub

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

Jump to bottom

clients(lr): allow for custom config #7613

Merged
merged 6 commits into from
Apr 4, 2019
Merged

clients(lr): allow for custom config #7613

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8cdcea9
clients(lr): allow for custom config. do not prune config in lhr.
connorjclark Mar 21, 2019
913d02b
add comment saying lrdevice/catids get ignored
connorjclark Mar 21, 2019
b5d2e8c
fix test
connorjclark Mar 21, 2019
fd10674
Merge remote-tracking branch 'origin/master' into lr-config-proto-lr-…
connorjclark Mar 29, 2019
dee3d3f
add back the lr config setting pruning
connorjclark Mar 29, 2019
361537c
fix
connorjclark Apr 2, 2019
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
17 changes: 11 additions & 6 deletions clients/lightrider-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ const LR_PRESETS = {
* @param {Connection} connection
* @param {string} url
* @param {LH.Flags} flags Lighthouse flags, including `output`
* @param {{lrDevice?: 'desktop'|'mobile', categoryIDs?: Array<string>, logAssets: boolean, keepRawValues: boolean}} lrOpts Options coming from Lightrider
* @param {{lrDevice?: 'desktop'|'mobile', categoryIDs?: Array<string>, logAssets: boolean, keepRawValues: boolean, configOverride?: LH.Config.Json}} lrOpts Options coming from Lightrider
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
* @return {Promise<string|Array<string>|void>}
*/
async function runLighthouseInLR(connection, url, flags, lrOpts) {
const {lrDevice, categoryIDs, logAssets, keepRawValues} = lrOpts;
const {lrDevice, categoryIDs, logAssets, keepRawValues, configOverride} = lrOpts;

// Certain fixes need to kick in under LR, see https://github.com/GoogleChrome/lighthouse/issues/5839
global.isLightrider = true;
Expand All @@ -38,10 +38,15 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
flags.logLevel = flags.logLevel || 'info';
flags.channel = 'lr';

const config = lrDevice === 'desktop' ? LR_PRESETS.desktop : LR_PRESETS.mobile;
if (categoryIDs) {
config.settings = config.settings || {};
config.settings.onlyCategories = categoryIDs;
let config;
if (configOverride) {
config = configOverride;
} else {
config = lrDevice === 'desktop' ? LR_PRESETS.desktop : LR_PRESETS.mobile;
if (categoryIDs) {
config.settings = config.settings || {};
config.settings.onlyCategories = categoryIDs;
}
}

try {
Expand Down
48 changes: 48 additions & 0 deletions clients/test/lightrider-entry-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,53 @@ describe('lightrider-entry', () => {

runStub.mockRestore();
});

it('uses the desktop config preset when device is desktop', async () => {
const runStub = jest.spyOn(Runner, 'run');

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

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

runStub.mockRestore();
});

it('uses the mobile config preset when device is mobile', async () => {
const runStub = jest.spyOn(Runner, 'run');

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

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

runStub.mockRestore();
});

it('overrides the default config when one is provided', async () => {
const runStub = jest.spyOn(Runner, 'run');

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

const configOverride = {
default: 'lighthouse:default',
settings: {
onlyAudits: ['network-requests'],
},
};
await lhBackground.runLighthouseInLR(mockConnection, url, {}, {configOverride});
const config = runStub.mock.calls[0][1].config;
assert.equal(config.settings.onlyAudits.length, 1);
assert.equal(config.settings.onlyAudits[0], 'network-requests');

runStub.mockRestore();
});
});
});
11 changes: 0 additions & 11 deletions lighthouse-core/lib/proto-preprocessor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,6 @@ function processForProto(result, opts = {}) {
/** @type {LH.Result} */
const reportJson = JSON.parse(result);

// Clean up the configSettings
// Note: This is not strictly required for conversion if protobuf parsing is set to
// 'ignore unknown fields' in the language of conversion.
if (reportJson.configSettings) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this being removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was pruning the JSON sent back to LR, which would then place it in a Config proto that only had {emulatedFormFactor, locale, onlyCategories}. Since the rest weren't saved it made sense to cut the extras here. With the proto changes, most of the settings (i left out things like output) can be saved, so I'm undoing the pruning. The fields that aren't in the proto are safely ignored when serialized.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, yeah basically I'm wondering why we don't need to prune anymore even though we still don't support 100% of the settings :)

Are the remaining extra properties any less of a concern than it was before? Based on the comments here, it seems like it was still technically unnecessary then but we decided to do it anyway 🤷‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's just a couple string values now. before it was like, 95% of it. pruning is slightly more unnecessary now :)

// The settings that are in both proto and LHR
const {emulatedFormFactor, locale, onlyCategories} = reportJson.configSettings;

// @ts-ignore - intentionally only a subset of settings.
reportJson.configSettings = {emulatedFormFactor, locale, onlyCategories};
}

// Remove runtimeError if it is NO_ERROR
if (reportJson.runtimeError && reportJson.runtimeError.code === 'NO_ERROR') {
delete reportJson.runtimeError;
Expand Down