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

clients(lr): allow for custom config #7613

Merged
merged 6 commits into from
Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
19 changes: 13 additions & 6 deletions clients/lightrider-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,16 @@ const LR_PRESETS = {

/**
* Run lighthouse for connection and provide similar results as in CLI.
*
* If configOverride is provided, lrDevice and categoryIDs are ignored.
* @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 +40,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
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
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
17 changes: 12 additions & 5 deletions lighthouse-core/test/report/proto-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,28 @@ describe('round trip JSON comparison subsets', () => {
});

it('has the same config values', () => {
expect(roundTripJson.configSettings).toMatchObject(sampleJson.configSettings);
// Config settings from proto round trip should be a subset of the actual settings.
expect(sampleJson.configSettings).toMatchObject(roundTripJson.configSettings);
Copy link
Collaborator

Choose a reason for hiding this comment

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

wait how did it work the other way around then if one is a subset of the other 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These objects are actually 100% equal, so either way would be fine. It was only apparent that the order is wrong when I was allowing more properties to be in the proto result.

so the test change isn't needed, but it is more correct. whatever settings are in the round trip json (from proto) should match the values as found in the originating settings json

});
});

// Note: In a failing diff, if you see details.summary going from {} to null, it's OK.
// Jest considers this not a failure, and neither do we, here in the python roundtrip
// Meanwhile, The PSI roundtrip maintains {} to {}.
describe('round trip JSON comparison to everything', () => {
let sampleJson;

beforeEach(() => {
sampleJson = JSON.parse(preprocessor.processForProto(sample));

// Proto conversion turns null summaries into an empty object. This is OK,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment gave me pause because it seems like the process of going through proto conversion turns {} into null, the previous comment suggests this is the case as well.

wouldn't it make more sense to adjust our faulty result instead of the source json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The result is fine, but the jest matcher doesn't like {} when it expects null. The distinction is meaningless in proto world but important just for jest.

Copy link
Collaborator

@patrickhulce patrickhulce Apr 2, 2019

Choose a reason for hiding this comment

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

I get that, but proto doesn't actually seem to turn null summaries into {} based on the previous comments and the fact that you are turning {} into null in the sample json, which means that proto seems to be turning {} into null, not the other way around...right? or am I getting super confused by the names here and roundtripJson is actually the expectation not the one that went through the proto and back

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahhhh yes, I got it all mixed up here. updated with what I intended to do

// and is handled in the PSI roundtrip just fine, but messes up the easy
// jest sub-object matcher. So, we put null back in its place.
for (const audit of Object.values(sampleJson.audits)) {
if (audit.details && JSON.stringify(audit.details.summary) === '{}') {
audit.details.summary = null;
}
}
});

it('has the same JSON overall', () => {
expect(roundTripJson).toMatchObject(sampleJson);
expect(sampleJson).toMatchObject(roundTripJson);
});
});
1 change: 1 addition & 0 deletions proto/sample_v2_round_trip.json
Original file line number Diff line number Diff line change
Expand Up @@ -3706,6 +3706,7 @@
}
},
"configSettings": {
"channel": "cli",
"emulatedFormFactor": "mobile",
"locale": "en-US",
"onlyCategories": null
Expand Down
2 changes: 1 addition & 1 deletion proto/scripts/json_roundtrip_via_proto.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def clean():
proto_lhr = lhr_pb2.LighthouseResult()

# fill proto lhr with data from JSON
Parse(json.dumps(data), proto_lhr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess to finally figure out my real concern, is this something that anyone else will now have to change and match or are we the only people ever doing this proto-json conversion :)

apologies for being very slow on the uptake here 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pretty sure the proto stuff has one user: us, with LR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome sauce

Parse(json.dumps(data), proto_lhr, ignore_unknown_fields=True)

# convert proto back into json
round_trip_lhr = json.loads(MessageToJson(proto_lhr, including_default_value_fields=False))
Expand Down