-
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
extension: return runnerResult #6839
Conversation
Conform with the given return type definition.
Also, it may be masking errors in the extension test: https://github.com/GoogleChrome/lighthouse/blob/master/clients/test/extension/extension-test.js#L94 |
whitespace
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.
in extension-test.js, let's assert something about lighthouseResult
.. like it has fetchedAt
on it?
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.
wow, good catch :)
@@ -104,6 +104,8 @@ async function runLighthouseInExtension(flags, categoryIDs) { | |||
const reportHtml = /** @type {string} */ (runnerResult.report); | |||
const blobURL = createReportPageAsBlob(reportHtml); | |||
await new Promise(resolve => chrome.windows.create({url: blobURL}, resolve)); | |||
|
|||
return runnerResult; |
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.
can you update the return type, too? Since the undefined
branch throws an error now, void
isn't necessary any more
yeah, we shouldn't have had a lack-of-something test by itself in JS :) |
discussed with @hoten, I'll take the test fix cause I see a few other things it would be nice to clean up in there. |
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!
No change in behavior.
I noticed that this function was typed to return a runner result, but it never does. It would be useful for #6831 if it actually did.