-
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(lantern): more flexible graph edge creation #4933
Conversation
d463482
to
3e9f29a
Compare
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.
Looks great to me. Great 🗺🔍🕵️ work
} | ||
|
||
if (process.env.APPVEYOR) { | ||
// Appveyor is hella slow already, disable CPU throttling so we're not 16x slowdown |
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.
maybe link to #4891?
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
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 % nit
const command = 'node'; | ||
const args = [ | ||
'lighthouse-cli/index.js', | ||
url, | ||
`--config-path=${configPath}`, | ||
`--output-path=smokehouse.report.json`, |
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.
I often run yarn smokehouse
and yarn smoke
simultaneously which could conflict in this scenario.
also my smokehouse revamp PR means we'll have a lot of these running at the same time.
how about something like:
const filename = `smokehouse${Math.round(Math.random()*1000))}.report.json`;
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.
if we go down this path, can we delete the report by default then? :P
closes #4891
this should fix the appveyor flakiness we've been seeing which was 3-pronged
also I made smokehouse a bit more debuggable by using output-path for the JSON instead of stdout and adding a ENV variable to print stdout