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 5 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();
});
});
});
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);
});
});