-
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
[cli] Move bin.ts core into -cli/run.ts #2602
Conversation
needs a |
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.
even though no code is really changing, should probably have at least one new test that calling the code from a module works :)
need a rebase (not sure whats going on with codecov) |
|
||
return handleError(err); | ||
} | ||
} |
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.
new line
lighthouse-cli/test/cli/run-test.js
Outdated
@@ -0,0 +1,35 @@ | |||
/** | |||
* @license Copyright 2016 Google Inc. All Rights Reserved. |
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.
2017
lighthouse-cli/cli-flags.ts
Outdated
@@ -17,8 +17,10 @@ export interface Flags { | |||
view: boolean, maxWaitForLoad: number | |||
} | |||
|
|||
export function getFlags() { | |||
return yargs.help('help') | |||
export function getFlags(fakeInput?: string) { |
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.
nice, but can we call the parameter something else? It seems like a legitimate use, not just for testing, so what about manualInput
, manualArgv
, argvString
, or something something :)
@brendankenny sg. done. |
|
||
describe('CLI run', function() { | ||
it('runLighthouse completes a LH round trip', () => { | ||
const url = 'chrome://version'; |
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.
:)
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
(partial) fix for #2601
Unblocks issues like addyosmani/webpack-lighthouse-plugin#5
No functional changes was made in this PR; it was only a copypaste party.