[go: nahoru, domu]
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

proto: update processForProto method signature, string -> LH.Result #9016

Merged
merged 8 commits into from
Jun 11, 2019
Merged

proto: update processForProto method signature, string -> LH.Result #9016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c508f1f
lightrider-entry can assume json now
paulirish May 21, 2019
455a20b
processForProto took/returned a string, now takes/returns LH.Result.
paulirish May 21, 2019
204c0cf
add test that input is not modified
paulirish May 25, 2019
3bf14de
Merge branch 'master' into jsoninlr
paulirish May 25, 2019
2ed7231
Update lighthouse-core/lib/proto-preprocessor.js
paulirish May 21, 2019
3a49ae2
Merge branch 'master' into jsoninlr
paulirish Jun 5, 2019
83debc0
feedback and rebase updates
paulirish Jun 5, 2019
1fe5e33
final feedback
paulirish Jun 11, 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
12 changes: 4 additions & 8 deletions clients/lightrider/lightrider-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ const LR_PRESETS = {
* If configOverride is provided, lrDevice and categoryIDs are ignored.
* @param {Connection} connection
* @param {string} url
* @param {LH.Flags} flags Lighthouse flags, including `output`
* @param {LH.Flags} flags Lighthouse flags
Copy link
Member

Choose a reason for hiding this comment

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

make sure you aren't accidentally still passing in output: 'html' and doing all that extra work :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.. i dont think we need a stronger check.

Copy link
Member

Choose a reason for hiding this comment

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

Added a comment.. i dont think we need a stronger check.

yeah, I meant just double check existing code, definitely don't need a check in the code

* @param {{lrDevice?: 'desktop'|'mobile', categoryIDs?: Array<string>, logAssets: boolean, configOverride?: LH.Config.Json}} lrOpts Options coming from Lightrider
* @return {Promise<string|Array<string>|void>}
* @return {Promise<string|void>}
Copy link
Member

Choose a reason for hiding this comment

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

is void still a return possibility? (I think it can be dropped now)

Copy link
Member

Choose a reason for hiding this comment

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

is void still a return possibility?

Oh, yes it is for the if (!results) return case. Should it throw to get a runtime error when that happens? Weird to return nothing if Lighthouse was acting poorly (I think it's actually a // should never happen case to make tsc happy).

*/
async function runLighthouseInLR(connection, url, flags, lrOpts) {
const {lrDevice, categoryIDs, logAssets, configOverride} = lrOpts;
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -59,12 +59,8 @@ async function runLighthouseInLR(connection, url, flags, lrOpts) {
await assetSaver.logAssets(results.artifacts, results.lhr.audits);
}

// pre process the LHR for proto
if (flags.output === 'json' && typeof results.report === 'string') {
return preprocessor.processForProto(results.report);
}

return results.report;
const processedLHR = preprocessor.processForProto(results.lhr);
return JSON.stringify(processedLHR);
} catch (err) {
// If an error ruined the entire lighthouse run, attempt to return a meaningful error.
let runtimeError;
Expand Down
15 changes: 8 additions & 7 deletions lighthouse-core/lib/proto-preprocessor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@ const fs = require('fs');
*/

/**
* @param {string} result
* Transform an LHR into a proto-friendly LHR
paulirish marked this conversation as resolved.
Show resolved Hide resolved
* @param {LH.Result} lhr
* @return {LH.Result}
*/
function processForProto(result) {
/** @type {LH.Result} */
const reportJson = JSON.parse(result);
function processForProto(lhr) {
const reportJson = JSON.parse(JSON.stringify(lhr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we skip the cloning? Only the lr entry uses this fn, and the original lhr is not needed there. if some other client comes along with different requirements they can clone the input themselves.

assuming cloning a lhr takes a measurable amount of time.

I clocked 4ms for example.com and for 8ms nyt.com:

console.time('clone'); JSON.parse(JSON.stringify(lhr)); console.timeEnd('clone');

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 find it unexpected when methods mutate the passed in object, so this is my preferred pattern. It's also what we do for Util.prepareReportResult, swapLocale, our tests, etc.

We adopted this pattern in our tests, as previously we had some unexpected mutations that caused failures that were tricky to track down.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I prefer avoiding mutation too. if the few ms is worth the change, I'd suggest renaming the method to modifyForProto to properly set expectations.

Copy link
Member

Choose a reason for hiding this comment

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

the type error (failure in travis) is because this whole function is operating on an any reportJson (due to the return type of JSON.parse). Luckily there was a forEach in there or we wouldn't have noticed the whole thing wasn't being checked :)

Need to keep the /** @type {LH.Result} */ above this line.


// Clean up the configSettings
// Note: This is not strictly required for conversion if protobuf parsing is set to
Expand Down Expand Up @@ -95,7 +96,7 @@ function processForProto(result) {

removeStrings(reportJson);

return JSON.stringify(reportJson);
return reportJson;
}

// @ts-ignore claims always false, but this checks if cli or module
Expand All @@ -113,9 +114,9 @@ if (require.main === module) {

if (input && output) {
// process the file
const report = processForProto(fs.readFileSync(input, 'utf-8'));
const report = processForProto(JSON.parse(fs.readFileSync(input, 'utf-8')));
// write to output from argv
fs.writeFileSync(output, report, 'utf-8');
fs.writeFileSync(output, JSON.stringify(report), 'utf-8');
}
} else {
module.exports = {
Expand Down
26 changes: 13 additions & 13 deletions lighthouse-core/test/lib/proto-preprocessor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

const processForProto = require('../../lib/proto-preprocessor').processForProto;
Copy link
Member

Choose a reason for hiding this comment

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

can you add a test that it doesn't modify the passed-in lhr?

const {processForProto} = require('../../lib/proto-preprocessor');

/* eslint-env jest */
describe('processing for proto', () => {
Expand Down Expand Up @@ -44,9 +44,9 @@ describe('processing for proto', () => {
'onlyCategories': null,
},
};
const output = processForProto(JSON.stringify(input));
const output = processForProto(input);

expect(JSON.parse(output)).toMatchObject(expectation);
expect(output).toMatchObject(expectation);
});

it('cleans up default runtimeErrors', () => {
Expand All @@ -56,9 +56,9 @@ describe('processing for proto', () => {
},
};

const output = processForProto(JSON.stringify(input));
const output = processForProto(input);

expect(JSON.parse(output)).not.toHaveProperty('runtimeError');
expect(output).not.toHaveProperty('runtimeError');
});

it('non-default runtimeErrors are untouched', () => {
Expand All @@ -68,9 +68,9 @@ describe('processing for proto', () => {
},
};

const output = processForProto(JSON.stringify(input));
const output = processForProto(input);

expect(JSON.parse(output)).toMatchObject(input);
expect(output).toMatchObject(input);
});

it('cleans up audits', () => {
Expand All @@ -91,9 +91,9 @@ describe('processing for proto', () => {
},
},
};
const output = processForProto(JSON.stringify(input));
const output = processForProto(input);

expect(JSON.parse(output)).toMatchObject(expectation);
expect(output).toMatchObject(expectation);
});


Expand All @@ -108,9 +108,9 @@ describe('processing for proto', () => {
const expectation = {
'i18n': {},
};
const output = processForProto(JSON.stringify(input));
const output = processForProto(input);

expect(JSON.parse(output)).toMatchObject(expectation);
expect(output).toMatchObject(expectation);
});

it('removes empty strings', () => {
Expand Down Expand Up @@ -151,8 +151,8 @@ describe('processing for proto', () => {
],
},
};
const output = processForProto(JSON.stringify(input));
const output = processForProto(input);

expect(JSON.parse(output)).toMatchObject(expectation);
expect(output).toMatchObject(expectation);
});
});
35 changes: 16 additions & 19 deletions lighthouse-core/test/report/proto-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,60 +5,57 @@
*/
'use strict';

const path = require('path');
const fs = require('fs');

const sample = fs.readFileSync(path.resolve(__dirname, '../results/sample_v2.json'));
const sampleJson = require('../results/sample_v2.json');
const roundTripJson = require('../../../proto/sample_v2_round_trip');
const preprocessor = require('../../lib/proto-preprocessor.js');

/* eslint-env jest */

describe('round trip JSON comparison subsets', () => {
let sampleJson;
let processedLHR;

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

it('has the same audit results and details (if applicable)', () => {
for (const auditId of Object.keys(sampleJson.audits)) {
expect(roundTripJson.audits[auditId]).toEqual(sampleJson.audits[auditId]);
for (const auditId of Object.keys(processedLHR.audits)) {
expect(roundTripJson.audits[auditId]).toEqual(processedLHR.audits[auditId]);
}
});

it('has the same i18n rendererFormattedStrings', () => {
expect(roundTripJson.i18n).toMatchObject(sampleJson.i18n);
expect(roundTripJson.i18n).toMatchObject(processedLHR.i18n);
});

it('has the same top level values', () => {
// Don't test all top level properties that are objects.
Object.keys(sampleJson).forEach(audit => {
if (typeof sampleJson[audit] === 'object' && !Array.isArray(sampleJson[audit])) {
delete sampleJson[audit];
Object.keys(processedLHR).forEach(audit => {
if (typeof processedLHR[audit] === 'object' && !Array.isArray(processedLHR[audit])) {
delete processedLHR[audit];
}
});

// Properties set to their type's default value will be omitted in the roundTripJson.
// For an explicit list of properties, remove sampleJson values if set to a default.
if (Array.isArray(sampleJson.stackPacks) && sampleJson.stackPacks.length === 0) {
delete sampleJson.stackPacks;
if (Array.isArray(processedLHR.stackPacks) && processedLHR.stackPacks.length === 0) {
delete processedLHR.stackPacks;
}

expect(roundTripJson).toMatchObject(sampleJson);
expect(roundTripJson).toMatchObject(processedLHR);
});

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

describe('round trip JSON comparison to everything', () => {
let sampleJson;
let processedLHR;

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

// Proto conversion turns empty summaries into null. This is OK,
// and is handled in the PSI roundtrip just fine, but messes up the easy
Expand All @@ -71,6 +68,6 @@ describe('round trip JSON comparison to everything', () => {
});

it('has the same JSON overall', () => {
expect(sampleJson).toMatchObject(roundTripJson);
expect(processedLHR).toMatchObject(roundTripJson);
});
});