-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(config): switch to lantern by default, add presets #5041
Changes from 9 commits
5bc5874
a7d0089
2b24c22
ed63596
08d65f8
b71d604
287c94c
52792e5
94f5155
3126132
4b7c1fb
0baf005
3412c82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -187,7 +187,7 @@ function deepClone(json) { | |
// injection of plugins. | ||
if (Array.isArray(json.passes)) { | ||
cloned.passes.forEach((pass, i) => { | ||
pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers); | ||
pass.gatherers = cloneArrayWithPluginSafety(json.passes[i].gatherers || []); | ||
}); | ||
} | ||
|
||
|
@@ -249,6 +249,8 @@ class Config { | |
skipAuditIds); | ||
} | ||
|
||
Config.adjustDefaultPassForThrottling(configJSON); | ||
|
||
// Store the directory of the config path, if one was provided. | ||
this._configDir = configPath ? path.dirname(configPath) : undefined; | ||
|
||
|
@@ -381,6 +383,26 @@ class Config { | |
return mergedItems; | ||
} | ||
|
||
/** | ||
* Observed throttling methods (devtools/provided) require at least 5s of quiet for the metrics to | ||
* be computed. This method adjusts the quiet thresholds to the required minimums if necessary. | ||
* | ||
* @param {LH.Config.Json} config | ||
*/ | ||
static adjustDefaultPassForThrottling(config) { | ||
if (config.settings.throttlingMethod !== 'devtools' && | ||
config.settings.throttlingMethod !== 'provided') { | ||
return; | ||
} | ||
|
||
const defaultPass = config.passes.find(pass => pass.passName === 'defaultPass'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hopefully in the future we can make a way to tell if it's a metric-measuring pass (similar to gather-runner's hacky There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah that makes sense, I suppose we could do it on all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed offline, it's an impl detail of TTI that we need 5s+ which is hardcoded to |
||
if (!defaultPass) return; | ||
|
||
defaultPass.pauseAfterLoadMs = Math.max(5250, defaultPass.pauseAfterLoadMs); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels like this 5250 magic number should be defined in constants.js There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call done |
||
defaultPass.cpuQuietThresholdMs = Math.max(5250, defaultPass.cpuQuietThresholdMs); | ||
defaultPass.networkQuietThresholdMs = Math.max(5250, defaultPass.networkQuietThresholdMs); | ||
} | ||
|
||
/** | ||
* Filter out any unrequested items from the config, based on requested top-level categories. | ||
* @param {!Object} oldConfig Lighthouse config object | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,8 @@ trap teardown EXIT | |
|
||
colorText "Generating a fresh LHR..." "$purple" | ||
set -x | ||
node "$lhroot_path/lighthouse-cli" -A="$lhroot_path/lighthouse-core/test/results/artifacts" --quiet --output=json --output-path="$freshLHRPath" http://localhost/dobetterweb/dbw_tester.html | ||
# TODO(phulce): add a lantern LHR-differ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should do this soon |
||
node "$lhroot_path/lighthouse-cli" -A="$lhroot_path/lighthouse-core/test/results/artifacts" --throttling-method=devtools --quiet --output=json --output-path="$freshLHRPath" http://localhost/dobetterweb/dbw_tester.html | ||
set +x | ||
|
||
# remove timing from both | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -454,6 +454,43 @@ describe('Config', () => { | |
assert.ok(auditNames.has('first-meaningful-paint'), 'did not include default audits'); | ||
}); | ||
|
||
it('ensures quiet thresholds are sufficient when using devtools', () => { | ||
const config = new Config({ | ||
extends: 'lighthouse:default', | ||
settings: { | ||
throttlingMethod: 'devtools', | ||
onlyCategories: ['performance'], | ||
}, | ||
}); | ||
|
||
assert.equal(config.settings.throttlingMethod, 'devtools'); | ||
assert.ok(config.passes[0].pauseAfterLoadMs >= 5000, 'did not adjust load quiet ms'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe add assertion that this is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
assert.ok(config.passes[0].cpuQuietThresholdMs >= 5000, 'did not adjust cpu quiet ms'); | ||
assert.ok(config.passes[0].networkQuietThresholdMs >= 5000, 'did not adjust network quiet ms'); | ||
}); | ||
|
||
it('does nothing when thresholds for devtools are already sufficient', () => { | ||
const config = new Config({ | ||
extends: 'lighthouse:default', | ||
settings: { | ||
throttlingMethod: 'devtools', | ||
onlyCategories: ['performance'], | ||
}, | ||
passes: [ | ||
{ | ||
pauseAfterLoadMs: 10001, | ||
cpuQuietThresholdMs: 10002, | ||
networkQuietThresholdMs: 10003, | ||
}, | ||
], | ||
}); | ||
|
||
assert.equal(config.settings.throttlingMethod, 'devtools'); | ||
assert.equal(config.passes[0].pauseAfterLoadMs, 10001); | ||
assert.equal(config.passes[0].cpuQuietThresholdMs, 10002); | ||
assert.equal(config.passes[0].networkQuietThresholdMs, 10003); | ||
}); | ||
|
||
it('merges settings with correct priority', () => { | ||
const config = new Config( | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ const RawProtocol = require('../../../lighthouse-core/gather/connections/raw'); | |
const Runner = require('../../../lighthouse-core/runner'); | ||
const Config = require('../../../lighthouse-core/config/config'); | ||
const defaultConfig = require('../../../lighthouse-core/config/default-config.js'); | ||
const fastConfig = require('../../../lighthouse-core/config/fast-config.js'); | ||
const log = require('lighthouse-logger'); | ||
|
||
/** | ||
|
@@ -22,7 +21,7 @@ const log = require('lighthouse-logger'); | |
window.runLighthouseForConnection = function( | ||
connection, url, options, categoryIDs, | ||
updateBadgeFn = function() { }) { | ||
const config = options && options.fastMode ? new Config(fastConfig, options.flags) : new Config({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where was this set before? I can't find it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's just set by lightrider, all runs for them will be simulated now anyway |
||
const config = new Config({ | ||
extends: 'lighthouse:default', | ||
settings: {onlyCategories: categoryIDs}, | ||
}, options.flags); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ | |
"changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file", | ||
"type-check": "tsc -p .", | ||
"update:sample-artifacts": "node lighthouse-core/scripts/update-report-fixtures.js -G", | ||
"update:sample-json": "node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --output=json --output-path=./lighthouse-core/test/results/sample_v2.json http://localhost/dobetterweb/dbw_tester.html && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing", | ||
"update:sample-json": "node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json http://localhost/dobetterweb/dbw_tester.html && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doing this to avoid the mixed settings error, right? so a followup PR to update both makes sense to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but speaking of. perhaps we should also have 1 golden LHR for simulate and 1 for throttled. Feels like we have a blind spot otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
totally agree ;) |
||
"diff:sample-json": "bash lighthouse-core/scripts/assert-golden-lhr-unchanged.sh", | ||
"update:crdp-typings": "node lighthouse-core/scripts/extract-crdp-mapping.js", | ||
"mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --config-path=./lighthouse-core/config/mixed-content.js" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,6 @@ Configuration: | |
CHROME_PATH: Explicit path of intended Chrome binary. If set must point to an executable of a build of | ||
Chromium version 54.0 or later. By default, any detected Chrome Canary or Chrome (stable) will be launched. | ||
[default: ""] | ||
--perf Use a performance-test-only configuration [boolean] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need presets and other new changes here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
--port The port to use for the debugging protocol. Use 0 for a random port [default: 0] | ||
--hostname The hostname to use for the debugging protocol. [default: "localhost"] | ||
--max-wait-for-load The timeout (in milliseconds) to wait before the page is considered done loading and the run should continue. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened here? Don't we require at least an empty array in the passes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well IMO, we shouldn't and the new defaults for passes make sure it's at least an empty array, but that doesn't work with the cloning, I can revert if you feel strongly all passes should declare empty gatherers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I think that makes sense in the extending case, if I'm reading your response correctly. Can you make gatherers optional in the
LH.Config.PassJson
side of things, then?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍