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

misc(proto): Add proto definition for LHR #6183

Merged
merged 31 commits into from
Oct 16, 2018
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a92af15
Initial proto definition of lhr*.
exterkamp Oct 4, 2018
0b0052d
Struct details in proto, added python to convert sample into proto an…
exterkamp Oct 4, 2018
48b4774
Added comments to extern.d.ts fields.
exterkamp Oct 4, 2018
9ab2045
Moved protos around and made them more nested in the .proto
exterkamp Oct 4, 2018
f52221e
Added proto junk to yarn clean.
exterkamp Oct 4, 2018
c42bd3b
Added in better proto README and yarn command to compile proto.
exterkamp Oct 4, 2018
85a4a9e
Added i18n_v1 message w/ListValues.
exterkamp Oct 5, 2018
f3a69eb
Removed python cleaning of i18n since ListValues doesn't need it.
exterkamp Oct 5, 2018
a9beffb
Cleaning up some docs + removing weird change to yarn changelog.
exterkamp Oct 5, 2018
a53c44e
Cleaned up python validator a bit.
exterkamp Oct 5, 2018
f836339
Review changes round 1. Newlines added, filenames changed, docs upda…
exterkamp Oct 5, 2018
0d56054
Forgot the link in proto.
exterkamp Oct 5, 2018
a3a6c77
Added new proto field definitions using Wrappers to accomodate null's…
exterkamp Oct 9, 2018
53e9649
Ignore empty strings in preprocessing, made config settings more gene…
exterkamp Oct 9, 2018
0b18d1e
Made proto test self contained with sample_v2_round_trip json.
exterkamp Oct 9, 2018
a0759df
Merge branch 'master' into proto-lhr
exterkamp Oct 9, 2018
2d8b923
Updated round trip to new strings. Made the json dump sort keys in p…
exterkamp Oct 9, 2018
89f0563
Forgot to sort output json keys.
exterkamp Oct 10, 2018
fc9e227
Added crc empty children removal to preprocessing.
exterkamp Oct 11, 2018
72a06c3
Added preprocessing step to running lighthouse in LR.
exterkamp Oct 11, 2018
af41fae
Merge branch 'master' into proto-lhr
exterkamp Oct 11, 2018
7ff805c
Added preprocessor to LR.
exterkamp Oct 11, 2018
3c2fe15
Merge branch 'master' into proto-lhr
exterkamp Oct 11, 2018
d513396
Added LighthouseError to proto def.
exterkamp Oct 11, 2018
6f1ce30
Changed proto-test to use the lib/preprocessor.
exterkamp Oct 11, 2018
a3330a5
Added preprocessing tests and hardened preprocessor.
exterkamp Oct 11, 2018
7f92586
Removed crc chain removal from preprocessing.
exterkamp Oct 11, 2018
e8be8e8
misc(proto): move scripts into scripts/ folder
paulirish Oct 12, 2018
95c0952
Feedback from Paul. Removed python preprocessor. Added typechecking…
exterkamp Oct 15, 2018
fdf1d6d
Switched from Popen to call, removed Popen legacy arg from first try …
exterkamp Oct 15, 2018
2ce9df0
Made in/out in arg reading better.
exterkamp Oct 15, 2018
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
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,9 @@ yarn-error.log
/chrome.zip

plots/out**

*__pycache__
*.pyc
proto/scripts/*_pb2.*
proto/scripts/*_pb.*
proto/scripts/*_processed.json
75 changes: 75 additions & 0 deletions lighthouse-core/lib/proto-preprocessor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
// @ts-nocheck
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
'use strict';

/**
* Helper functions to transform an LHR into a proto-ready LHR
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
*/

/**
* @param {string} result
*/
function processForProto(result) {
const reportJson = JSON.parse(result);

// clean up that requires audits to exist
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if ('audits' in reportJson) {
// clean up audits
Object.keys(reportJson.audits).forEach(audit => {
// clean up score display modes
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if ('scoreDisplayMode' in reportJson.audits[audit]) {
if (reportJson.audits[audit].scoreDisplayMode === 'not-applicable') {
reportJson.audits[audit].scoreDisplayMode = 'not_applicable';
}
}
// delete raw values
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if ('rawValue' in reportJson.audits[audit]) {
delete reportJson.audits[audit].rawValue;
}
// clean up display values
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if ('displayValue' in reportJson.audits[audit]) {
if (Array.isArray(reportJson.audits[audit]['displayValue'])) {
const values = [];
reportJson.audits[audit]['displayValue'].forEach(item => {
values.push(item);
});
reportJson.audits[audit]['displayValue'] = values.join(' | ');
}
}
});
}

// delete i18n icuMsg paths
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
if ('i18n' in reportJson && 'icuMessagePaths' in reportJson.i18n) {
delete reportJson.i18n.icuMessagePaths;
}

// remove empty strings
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
(function removeStrings(obj) {
if (obj && typeof obj === 'object' && !Array.isArray(obj)) {
Copy link
Member

Choose a reason for hiding this comment

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

@patrickhulce @brendankenny do yall think we should use lodash here instead?

Object.keys(obj).forEach(key => {
if (typeof obj[key] === 'string' && obj[key] === '') {
delete obj[key];
} else if (typeof obj[key] === 'object' || Array.isArray(obj[key])) {
removeStrings(obj[key]);
}
});
} else if (Array.isArray(obj)) {
obj.forEach(item => {
if (typeof item === 'object' || Array.isArray(item)) {
removeStrings(item);
}
});
}
})(reportJson);

return JSON.stringify(reportJson);
}

module.exports = {
processForProto,
};
85 changes: 85 additions & 0 deletions lighthouse-core/test/lib/proto-preprocessor-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

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

/* eslint-env jest */
describe('processing for proto', () => {
it('cleans up audits', () => {
const input = {
'audits': {
'critical-request-chains': {
'scoreDisplayMode': 'not-applicable',
'rawValue': 14.3,
'displayValue': ['hello %d', 123],
},
},
};
const expectation = {
'audits': {
'critical-request-chains': {
'scoreDisplayMode': 'not_applicable',
'displayValue': 'hello %d | 123',
},
},
};
const output = processForProto(JSON.stringify(input));

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


it('removes i18n icuMessagePaths', () => {
const input = {
'i18n': {
'icuMessagePaths': {
'content': 'paths',
},
},
};
const expectation = {
'i18n': {},
};
const output = processForProto(JSON.stringify(input));

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

it('removes empty strings', () => {
Copy link
Member

Choose a reason for hiding this comment

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

i think you'll want another test case here? something involving arrays? details?

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 little addition with an array containing an object that has an empty string. It doesn't clear empty strings that are in an array by design though.

const input = {
'audits': {
'critical-request-chains': {
'details': {
'chains': {
'1': '',
},
},
},
},
'i18n': {
'icuMessagePaths': {
'content': 'paths',
},
'2': '',
},
};
const expectation = {
'audits': {
'critical-request-chains': {
'details': {
'chains': {},
},
},
},
'i18n': {
},
};
const output = processForProto(JSON.stringify(input));

expect(JSON.parse(output)).toMatchObject(expectation);
});
});
72 changes: 72 additions & 0 deletions lighthouse-core/test/report/proto-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be under test/report/. Should the contents just be in proto-preprocessor-test.js? Or, if the preprocessor is going to eventually go away, seems like it should just be in proto/test/?

* @license Copyright 2017 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

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

const sample = fs.readFileSync(path.resolve(__dirname, '../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;

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

it('has the same audit results sans details', () => {
Object.keys(sampleJson.audits).forEach(audit => {
delete sampleJson.audits[audit].details;
});

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

it('has the same audit results & details if applicable', () => {
Object.keys(sampleJson.audits).forEach(auditId => {
expect(roundTripJson.audits[auditId]).toMatchObject(sampleJson.audits[auditId]);

if ('details' in sampleJson.audits[auditId]) {
expect(roundTripJson.audits[auditId].details)
.toMatchObject(sampleJson.audits[auditId].details);
}
});
});

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

it('has the same top level values', () => {
Object.keys(sampleJson).forEach(audit => {
if (typeof sampleJson[audit] === 'object' && !Array.isArray(sampleJson[audit])) {
delete sampleJson[audit];
}
});

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

it('has the same config values', () => {
expect(roundTripJson.configSettings).toMatchObject(sampleJson.configSettings);
});
});

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

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

it('has the same JSON overall', () => {
expect(roundTripJson).toMatchObject(sampleJson);
});
});
9 changes: 9 additions & 0 deletions lighthouse-extension/app/src/lightrider-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const lighthouse = require('../../../lighthouse-core/index');

const assetSaver = require('../../../lighthouse-core/lib/asset-saver.js');
const LHError = require('../../../lighthouse-core/lib/lh-error.js');
const preprocessor = require('../../../lighthouse-core/lib/proto-preprocessor.js');
exterkamp marked this conversation as resolved.
Show resolved Hide resolved

/** @type {Record<'mobile'|'desktop', LH.Config.Json>} */
const LR_PRESETS = {
Expand Down Expand Up @@ -46,6 +47,14 @@ async function runLighthouseInLR(connection, url, flags, {lrDevice, categoryIDs,
if (logAssets) {
await assetSaver.logAssets(results.artifacts, results.lhr.audits);
}

// pre process the LHR for proto
if (flags.output === 'json') {
Copy link
Member

Choose a reason for hiding this comment

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

if (flags.output === 'json' && typeof results.report === 'string')

Copy link
Member

Choose a reason for hiding this comment

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

actually instead...

call preprocessor.processForProto(results.lhr); and avoid the parse inside of that method?

if (typeof results.report === 'string') {
return preprocessor.processForProto(results.report);
}
}

return results.report;
} catch (err) {
// If an error ruined the entire lighthouse run, attempt to return a meaningful error.
Expand Down
7 changes: 5 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"build-all:task:windows": "yarn build-extension && yarn build-viewer",
"build-extension": "cd ./lighthouse-extension && yarn build",
"build-viewer": "cd ./lighthouse-viewer && yarn build",
"clean": "rimraf *.report.html *.report.dom.html *.report.json *.devtoolslog.json *.trace.json || true",
"clean": "rimraf proto/*_processed.json proto/*_pb2.* proto/*_pb.* proto/__pycache__ proto/*.pyc *.report.html *.report.dom.html *.report.json *.devtoolslog.json *.trace.json || true",
"lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
"smoke": "node lighthouse-cli/test/smokehouse/run-smoke.js",
"debug": "node --inspect-brk ./lighthouse-cli/index.js",
Expand Down Expand Up @@ -63,7 +63,10 @@
"diff:sample-json": "yarn i18n:checks && bash lighthouse-core/scripts/assert-golden-lhr-unchanged.sh",
"ultradumbBenchmark": "./lighthouse-core/scripts/benchmark.js",
"mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --preset=mixed-content",
"minify-latest-run": "./lighthouse-core/scripts/lantern/minify-trace.js ./latest-run/defaultPass.trace.json ./latest-run/defaultPass.trace.min.json && ./lighthouse-core/scripts/lantern/minify-devtoolslog.js ./latest-run/defaultPass.devtoolslog.json ./latest-run/defaultPass.devtoolslog.min.json"
"minify-latest-run": "./lighthouse-core/scripts/lantern/minify-trace.js ./latest-run/defaultPass.trace.json ./latest-run/defaultPass.trace.min.json && ./lighthouse-core/scripts/lantern/minify-devtoolslog.js ./latest-run/defaultPass.devtoolslog.json ./latest-run/defaultPass.devtoolslog.min.json",
"update:proto": "yarn compile-proto && yarn build-proto",
"compile-proto": "protoc --python_out=./ ./proto/lighthouse-result.proto && mv ./proto/*_pb2.py ./proto/scripts",
"build-proto": "cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python make_python_roundtrip.py"
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
},
"devDependencies": {
"@types/chrome": "^0.0.60",
Expand Down
18 changes: 18 additions & 0 deletions proto/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
## How to compile protos + use locally

1. Install the proto compiler
1. Manual install
1. Get the latest proto [release](https://github.com/protocolbuffers/protobuf/releases) (select one with python included if you want to run this validator)
1. Install the [C++ Protocol Buffer Runtime](https://github.com/protocolbuffers/protobuf/blob/master/src/README.md)
1. Brew install
1. `brew install protobuf`
1. Run `yarn compile-proto` then `yarn build-proto`

## Proto Resources
- [Protobuf Github Repo](https://github.com/protocolbuffers/protobuf)
- [Protobuf Docs](https://developers.google.com/protocol-buffers/docs/overview)

## Hacking Hints
- Clean out compiled proto and json with `yarn clean`
- Round trips might jumble the order of your JSON keys and lists!
- Is your `six` installation troubling your `pip install protobuf`? Mine did. Try `pip install --ignore-installed six`.
Loading