Skip to content

Commit

Permalink
Feedback from Paul. Removed python preprocessor. Added typechecking t…
Browse files Browse the repository at this point in the history
…o js preprocessor. 1 more test. Comment fixes.
  • Loading branch information
exterkamp committed Oct 15, 2018
1 parent e8be8e8 commit 95c0952
Show file tree
Hide file tree
Showing 8 changed files with 129 additions and 137 deletions.
89 changes: 61 additions & 28 deletions lighthouse-core/lib/proto-preprocessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,53 +3,64 @@
* 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

'use strict';

const fs = require('fs');

/**
* Helper functions to transform an LHR into a proto-ready LHR
* @fileoverview Helper functions to transform an LHR into a proto-ready LHR.
*
* FIXME: This file is 100% technical debt. Our eventual goal is for the
* roundtrip JSON to match the Golden LHR 1:1.
*/

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

// clean up that requires audits to exist
// Clean up actions that require 'audits' to exist
if ('audits' in reportJson) {
// clean up audits
Object.keys(reportJson.audits).forEach(audit => {
// clean up score display modes
if ('scoreDisplayMode' in reportJson.audits[audit]) {
if (reportJson.audits[audit].scoreDisplayMode === 'not-applicable') {
reportJson.audits[audit].scoreDisplayMode = 'not_applicable';
Object.keys(reportJson.audits).forEach(auditName => {
const audit = reportJson.audits[auditName];

// Rewrite the 'not-applicable' scoreDisplayMode to 'not_applicable'. #6201
if ('scoreDisplayMode' in audit) {
if (audit.scoreDisplayMode === 'not-applicable') {
// @ts-ignore Breaking the LH.Result type
audit.scoreDisplayMode = 'not_applicable';
}
}
// delete raw values
if ('rawValue' in reportJson.audits[audit]) {
delete reportJson.audits[audit].rawValue;
// Drop raw values. #6199
if ('rawValue' in audit) {
delete audit.rawValue;
}
// clean up display values
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(' | ');
}
// Normalize displayValue to always be a string, not an array. #6200

if (Array.isArray(audit.displayValue)) {
/** @type {Array<any>}*/
const values = [];
audit.displayValue.forEach(item => {
values.push(item);
});
audit.displayValue = values.join(' | ');
}
});
}

// delete i18n icuMsg paths
// Drop the i18n icuMessagePaths. Painful in proto, and low priority to expose currently.
if ('i18n' in reportJson && 'icuMessagePaths' in reportJson.i18n) {
delete reportJson.i18n.icuMessagePaths;
}

// remove empty strings
(function removeStrings(obj) {
// Remove any found empty strings, as they are dropped after round-tripping anyway
/**
* @param {any} obj
*/
function removeStrings(obj) {
if (obj && typeof obj === 'object' && !Array.isArray(obj)) {
Object.keys(obj).forEach(key => {
if (typeof obj[key] === 'string' && obj[key] === '') {
Expand All @@ -65,11 +76,33 @@ function processForProto(result) {
}
});
}
})(reportJson);
}

removeStrings(reportJson);

return JSON.stringify(reportJson);
}

module.exports = {
processForProto,
};
// @ts-ignore claims always false, but this checks if cli or module
if (require.main === module) {
// read in the argv for the input & output
const args = process.argv.slice(2);
let input;
let output;

if (args.length) {
input = args.find(flag => flag.startsWith('--in'));
output = args.find(flag => flag.startsWith('--out'));
}

if (input && output) {
// process the file
const report = processForProto(fs.readFileSync(input.replace('--in=', ''), 'utf-8'));

This comment has been minimized.

Copy link
@paulirish

paulirish Oct 15, 2018

Member

IMO you should move these .replace() up to L94/etc

This comment has been minimized.

Copy link
@exterkamp

exterkamp Oct 15, 2018

Author Member

Agree. I had it like that before, I just needed @brendankenny's wizardry to make it type-check.

// write to output from argv
fs.writeFileSync(output.replace('--out=', ''), report, 'utf-8');
}
} else {
module.exports = {
processForProto,
};
}
11 changes: 10 additions & 1 deletion lighthouse-core/test/lib/proto-preprocessor-test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* @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.
*/
Expand Down Expand Up @@ -65,6 +65,12 @@ describe('processing for proto', () => {
'content': 'paths',
},
'2': '',
'3': [
{
'hello': 'world',
'4': '',
},
],
},
};
const expectation = {
Expand All @@ -76,6 +82,9 @@ describe('processing for proto', () => {
},
},
'i18n': {
'3': [
{'hello': 'world'},
],
},
};
const output = processForProto(JSON.stringify(input));
Expand Down
6 changes: 2 additions & 4 deletions lighthouse-extension/app/src/lightrider-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,8 @@ async function runLighthouseInLR(connection, url, flags, {lrDevice, categoryIDs,
}

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

return results.report;
Expand Down
4 changes: 2 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 proto/*_processed.json proto/*_pb2.* proto/*_pb.* proto/__pycache__ proto/*.pyc *.report.html *.report.dom.html *.report.json *.devtoolslog.json *.trace.json || true",
"clean": "rimraf proto/scripts/*.json proto/scripts/*_pb2.* proto/scripts/*_pb.* proto/scripts/__pycache__ proto/scripts/*.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 @@ -66,7 +66,7 @@
"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"
"build-proto": "cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py"
},
"devDependencies": {
"@types/chrome": "^0.0.60",
Expand Down
11 changes: 11 additions & 0 deletions proto/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@
- [Protobuf Github Repo](https://github.com/protocolbuffers/protobuf)
- [Protobuf Docs](https://developers.google.com/protocol-buffers/docs/overview)

## LHR Round Trip Flow
```
LHR round trip flow:
(Compiling the Proto)
lighthouse_result.proto -> protoc --python_out ... -> lighthouse_result.pb2
(used by)
(Making a Round Trip JSON) ⭏
lhr.json --> proto_preprocessor.js -> lhr_processed.json -> json_roundtrip_via_proto.py -> lhr.round_trip.json
```

## Hacking Hints
- Clean out compiled proto and json with `yarn clean`
- Round trips might jumble the order of your JSON keys and lists!
Expand Down
43 changes: 43 additions & 0 deletions proto/scripts/json_roundtrip_via_proto.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import os
import sys
import json
import subprocess
import lighthouse_result_pb2 as lhr_pb2
from google.protobuf.json_format import Parse, MessageToJson

path = os.path.realpath(__file__)
path_dir = os.path.dirname(path)

path_sample_preprocessed = path_dir + '/sample_v2_processed.json'
path_sample = path_dir + '/../../lighthouse-core/test/results/sample_v2.json'
path_round_trip = path_dir + '/../sample_v2_round_trip.json'

def clean():
try:
os.remove(path_sample_preprocessed)
except OSError:
pass

# clean workspace
clean()

# preprocess the sample json
cmd = ["node", "./../../lighthouse-core/lib/proto-preprocessor.js", "--in=./../../lighthouse-core/test/results/sample_v2.json", "--out=./sample_v2_processed.json"]
process = subprocess.call(cmd, stdout=subprocess.PIPE)

# open json
with open(path_sample_preprocessed, 'r') as f:
data = json.load(f)

# make empty proto lhr
proto_lhr = lhr_pb2.LighthouseResult()

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

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

# write the json to disk
with open(path_round_trip, 'w') as f:
json.dump(round_trip_lhr, f, indent=4, sort_keys=True)
36 changes: 0 additions & 36 deletions proto/scripts/make_python_roundtrip.py

This file was deleted.

66 changes: 0 additions & 66 deletions proto/scripts/proto_preprocessor.py

This file was deleted.

0 comments on commit 95c0952

Please sign in to comment.