-
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
misc: localize logged GatherRunner error #9291
Conversation
fixes #8944 |
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
need to fix some test failures though |
ok, the test fixes are a little large, but I think help things. They don't overlap with the original commit but may be easier to review as a separate commit (e447753). The root cause was that those two failing tests were using mocked settings which didn't have a locale, so it was trying to do a lookup in I also updated the error message you get when you ask for an invalid locale (like You should probably take another look @patrickhulce :) |
@@ -183,6 +183,8 @@ const _ICUMsgNotFoundMsg = 'ICU message not found in destination locale'; | |||
*/ | |||
function _formatIcuMessage(locale, icuMessageId, fallbackMessage, values) { | |||
const localeMessages = LOCALES[locale]; | |||
if (!localeMessages) throw new Error(`Unsupported locale '${locale}'`); |
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.
this isn't something that should ever happen outside of tests since in a normal run we specifically pick which locale to use from the keys of LOCALES
itself.
...but if it does somehow happen in a normal run, this is a strictly better error message than "Cannot read property 'lighthouse-core/lib/lh-error.js | pageLoadFailed' of undefined"
anyways, so seems ok.
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.
turned into a different change haha but still LGTM!
another tiny one, apologies for the spam:)Right now when you hit a pageLoadError you get a logged message like
GatherRunner:error lighthouse-core/lib/lh-error.js | pageLoadFailedInsecure # 0 https://expired.badssl.com/ +3ms
This gets the full string for it.
fixes #8944edit: nope