-
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(lightwallet): add budget to sample artifacts config #8783
Conversation
@@ -31,7 +50,7 @@ async function update() { | |||
url, | |||
].join(' '); | |||
const flags = cliFlags.getFlags(rawFlags); | |||
await cli.runLighthouse(url, flags, undefined); | |||
await cli.runLighthouse(url, flags, budgetedConfig); |
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.
since we don't run bin.js
(which would allow loading from a --budget-path
), this could either put a budget on flags.budgets
or make a config as it does here. The config seemed cleaner, but I'm not overly attached to the approach.
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.
config sgtm 👍
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.
LGTM
though not sure I'd call it core
.... :)
tests
, misc
?
@@ -31,7 +50,7 @@ async function update() { | |||
url, | |||
].join(' '); | |||
const flags = cliFlags.getFlags(rawFlags); | |||
await cli.runLighthouse(url, flags, undefined); | |||
await cli.runLighthouse(url, flags, budgetedConfig); |
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.
config sgtm 👍
good point :) |
#8427 manually added a budget to the sample artifacts. This PR changes
update-report-fixtures.js
to add a budget to the run so that it will always be in the sample artifacts when it's updated in the future.Originally discussed in #8427 (comment).
I just copied the budget added in #8427. The property order changes in the
artifacts.json
file because this is the order thatbudget.js
initializes things.