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

tests(lightwallet): add perf-budget smoke test #8853

Merged
merged 2 commits into from
May 6, 2019
Merged

tests(lightwallet): add perf-budget smoke test #8853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6353a7e
tests(lightwallet): add perf-budget smoke test
brendankenny May 4, 2019
3c37ab0
add undefined comment
brendankenny May 5, 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
72 changes: 72 additions & 0 deletions lighthouse-cli/test/fixtures/perf/perf-budgets/big-script.js
View file
Open in desktop

Large diffs are not rendered by default.

46 changes: 46 additions & 0 deletions lighthouse-cli/test/fixtures/perf/perf-budgets/load-things.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<!doctype html>
<!--
* Copyright 2019 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.
-->
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<style>
@font-face {
font-family: 'Lobster';
src: local('Lobster'), url('../lobster-v20-latin-regular.woff2') format('woff2');
}
@font-face {
font-family: 'Lobster Two';
src: local("Lobster Two"), url("../lobster-two-v10-latin-700.woff2") format('woff2');
}
.webfont {
font-family: Lobster, sans-serif;
}
strong.webfont {
font-family: Lobster Two, sans-serif;
}
</style>
<link rel="stylesheet" href="http://localhost:10503/perf/cors-fonts.css">
<script src="big-script.js"></script>
<script>
async function yah() {
// Load file as 'other' resource.
const response = await fetch('http://localhost:10503/preload.html');
const ab = await response.arrayBuffer();
document.querySelector('#corsfont').textContent = ab.length;
}
yah();
</script>
</head>
<body>
<img src="../../dobetterweb/lighthouse-480x318.jpg" width="480" height="57">
<img style="width: 100px; height: 100px;" src="../../byte-efficiency/large.svg">
<p class="webfont">Ripping off some webfont smoke tests</p>
<p><strong class="webfont">Do we need such text</strong></p>
<p id="corsfont"></p>
<script src="../level-2.js"></script>
</body>
</html>
80 changes: 79 additions & 1 deletion lighthouse-cli/test/smokehouse/perf/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
'use strict';

/**
* Expected Lighthouse audit values for --preset=perf tests
* Expected Lighthouse audit values for perf tests.
*/
module.exports = [
{
Expand Down Expand Up @@ -65,6 +65,84 @@ module.exports = [
},
},
},
{
lhr: {
requestedUrl: 'http://localhost:10200/perf/perf-budgets/load-things.html',
finalUrl: 'http://localhost:10200/perf/perf-budgets/load-things.html',
audits: {
'resource-summary': {
score: null,
displayValue: '11 requests • 164 KB',
details: {
items: [
{resourceType: 'total', requestCount: 11, size: '168000±1000'},
Copy link
Member Author

Choose a reason for hiding this comment

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

used ± to leave some leeway on some of the assets because most of them are shared with other smoke tests. This will allow the test to absorb changes to them of a few hundred bytes or so, but we can always make it exact.

{resourceType: 'font', requestCount: 2, size: '80000±1000'},
{resourceType: 'script', requestCount: 3, size: '55000±1000'},
{resourceType: 'image', requestCount: 2, size: '28000±1000'},
{resourceType: 'document', requestCount: 1, size: '2100±100'},
{resourceType: 'other', requestCount: 2, size: '1250±50'},
{resourceType: 'stylesheet', requestCount: 1, size: '450±100'},
{resourceType: 'media', requestCount: 0, size: 0},
{resourceType: 'third-party', requestCount: 0, size: 0},
],
},
},
'performance-budget': {
score: null,
details: {
// Undefined items are asserting that the property isn't included in the table item.
items: [
{
resourceType: 'total',
countOverBudget: '3 requests',
sizeOverBudget: '65000±1000',
},
{
resourceType: 'script',
countOverBudget: '2 requests',
sizeOverBudget: '25000±1000',
},
{
resourceType: 'font',
countOverBudget: undefined,
sizeOverBudget: '4000±500',
},
{
resourceType: 'document',
countOverBudget: '1 request',
sizeOverBudget: '1100±50',
},
{
resourceType: 'stylesheet',
countOverBudget: undefined,
sizeOverBudget: '450±100',
},
{
resourceType: 'image',
countOverBudget: '1 request',
sizeOverBudget: undefined,
Copy link
Member Author

Choose a reason for hiding this comment

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

Letting undefined values in the expectations match not defined properties in actual allows testing for things like sizeOverBudget when not over budget, when its value is set to undefined and so it lost in the JSON shuffle.

If we just left this sizeOverBudget off the expectations it would indeed match an actual undefined value, which is good...but it would also match if actual was erroneously returning a numeric sizeOverBudget, so it would be useless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add a comment somewhere to this effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

should we add a comment somewhere to this effect?

added a comment to make it clear to anyone reading the expectations file

},
{
resourceType: 'media',
countOverBudget: undefined,
sizeOverBudget: undefined,
},
{
resourceType: 'other',
countOverBudget: '1 request',
sizeOverBudget: undefined,
},
{
resourceType: 'third-party',
countOverBudget: undefined,
sizeOverBudget: undefined,
},
],
},
},
},
},
},
{
lhr: {
requestedUrl: 'http://localhost:10200/perf/fonts.html',
Expand Down
50 changes: 50 additions & 0 deletions lighthouse-cli/test/smokehouse/perf/perf-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* @license Copyright 2019 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';

/** @type {LH.Config.Json} */
const perfConfig = {
extends: 'lighthouse:default',
settings: {
throttlingMethod: 'devtools',
onlyCategories: ['performance'],

// A mixture of under, over, and meeting budget to exercise all paths.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe include the path to the file where these budgets are being tested so we can look them up for correctness later? it's a bit weird that our "expectations" are basically here in the config that's shared with several other smokes

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a bit weird that our "expectations" are basically here in the config
I mean, that's kind of what all configs are :)

that's shared with several other smokes

I could split it into a new smoke budget category? Or is it fine to leave as one big perf category

budgets: [{
resourceCounts: [
{resourceType: 'total', budget: 8},
{resourceType: 'stylesheet', budget: 1}, // meets budget
{resourceType: 'image', budget: 1},
{resourceType: 'media', budget: 0},
{resourceType: 'font', budget: 2}, // meets budget
{resourceType: 'script', budget: 1},
{resourceType: 'document', budget: 0},
{resourceType: 'other', budget: 1},
{resourceType: 'third-party', budget: 0},
],
resourceSizes: [
{resourceType: 'total', budget: 100},
{resourceType: 'stylesheet', budget: 0},
{resourceType: 'image', budget: 30}, // meets budget
{resourceType: 'media', budget: 0},
{resourceType: 'font', budget: 75},
{resourceType: 'script', budget: 30},
{resourceType: 'document', budget: 1},
{resourceType: 'other', budget: 2}, // meets budget
{resourceType: 'third-party', budget: 0},
],
timings: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

these aren't being used right now, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

these aren't being used right now, correct?

right, they're parsed by budget.js, but the audit doesn't check them yet

{metric: 'first-contentful-paint', budget: 2000, tolerance: 100},
{metric: 'first-cpu-idle', budget: 2000, tolerance: 100},
{metric: 'interactive', budget: 2000, tolerance: 100},
{metric: 'first-meaningful-paint', budget: 2000, tolerance: 100},
{metric: 'estimated-input-latency', budget: 2000, tolerance: 100},
],
}],
},
};

module.exports = perfConfig;
2 changes: 1 addition & 1 deletion lighthouse-cli/test/smokehouse/smoke-test-dfns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ const SMOKE_TEST_DFNS = [{
}, {
id: 'perf',
expectations: 'perf/expectations.js',
config: 'lighthouse-core/config/perf-config.js',
config: 'perf/perf-config.js',
batch: 'perf-metric',
}, {
id: 'lantern',
Expand Down
9 changes: 4 additions & 5 deletions lighthouse-cli/test/smokehouse/smokehouse-report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const log = require('lighthouse-logger');

const VERBOSE = Boolean(process.env.LH_SMOKE_VERBOSE);
const NUMBER_REGEXP = /(?:\d|\.)+/.source;
const OPS_REGEXP = /<=?|>=?|\+\/-/.source;
const OPS_REGEXP = /<=?|>=?|\+\/-|±/.source;
// An optional number, optional whitespace, an operator, optional whitespace, a number.
const NUMERICAL_EXPECTATION_REGEXP =
new RegExp(`^(${NUMBER_REGEXP})?\\s*(${OPS_REGEXP})\\s*(${NUMBER_REGEXP})$`);
Expand All @@ -20,6 +20,7 @@ const NUMERICAL_EXPECTATION_REGEXP =
* - Greater than/less than operators, e.g. "<100", ">90"
* - Regular expressions
* - Strict equality
* - plus or minus a margin of error, e.g. '10+/-5', '100±10'
*
* @param {*} actual
* @param {*} expected
Expand All @@ -39,6 +40,7 @@ function matchesExpectation(actual, expected) {
case '<=':
return actual <= postfixNumber;
case '+/-':
case '±':
return Math.abs(actual - prefixNumber) <= postfixNumber;
default:
throw new Error(`unexpected operator ${operator}`);
Expand Down Expand Up @@ -80,16 +82,13 @@ function findDifference(path, actual, expected) {
}

// We only care that all expected's own properties are on actual (and not the other way around).
// Note an expected `undefined` can match an actual that is either `undefined` or not defined.
for (const key of Object.keys(expected)) {
// Bracket numbers, but property names requiring quotes will still be unquoted.
const keyAccessor = /^\d+$/.test(key) ? `[${key}]` : `.${key}`;
const keyPath = path + keyAccessor;
const expectedValue = expected[key];

if (!(key in actual)) {
return {path: keyPath, actual: undefined, expected: expectedValue};
}

const actualValue = actual[key];
const subDifference = findDifference(keyPath, actualValue, expectedValue);

Expand Down