Check for Browsertime errors in run(). #300
Conversation
When Coach is run from another script and Browsertime cannot access Selenium Grid, run() fails with TypeError: coach is undefined from result.browserScripts[0].coach. This commit adds rough error checking for Browsertime errors and returns a new Error object so the running script can handle the error.
|
Hi @jadonn thanks for the fix! Let me merge this before next release! |
|
I need to think a little how we should handle it, I'm not 100% sure yet. |
|
That is no problem! Thanks for the response. If there's a better way, I'm happy to work on this more and to learn how it could be better. |
|
Yeah what about just throw an error? That what happens right now if Browsertime has some problems or ... maybe need to check :) |
|
Thanks for the feedback! Throwing does make more sense. I don't remember why I initially chose to return the error instead of throwing it. I submitted another commit that changes the return to throw. I really appreciate the work you've done with the Coach and Sitespeed.io! It's a great tool, and I'm excited for the opportunity to give a little bit back to it. |
| result.errors[0].length !== 0 | ||
| ) { | ||
| const browsertimeError = new Error(result.errors); | ||
| browsertimeError.name = 'BROWSERTIMEERROR'; |
There was a problem hiding this comment.
Can you just rename this to BrowsertimeError instead, that's how we try to name errors.
|
Let me fix that so we can move on with the release :) Thank you @jadonn ! |
|
Thank you so much @soulgalore ! I apologize I was not able to make the correction in good time |
When Coach is run from another script and Browsertime cannot access
Selenium Grid, run() fails with TypeError: coach is undefined from
result.browserScripts[0].coach. This commit adds rough error checking for
Browsertime errors and returns a new Error object
so the running script can handle the error.
Your checklist for this pull request
Please review the guidelines for contributing to this repository.
I do not believe the change requires any changes to unit tests.
npm testand test linting bynpm run lintnpm test and npm run lint run successfully with the change.
The change should be reasonably simple. There is only one commit.
Description
Please describe your pull request and tell us the fix #
When Browsertime's attempt to connect to Selenium fails or times out, webcoach.run() fails with the following error:
TypeError: Cannot read property 'coach' of undefined
at runBrowsertime.then.result (/path/to/project/coach-api/node_modules/webcoach/lib/index.js:118:50)
at
at process._tickCallback (internal/process/next_tick.js:188:7)
I added a rough check for Browsertime errors to enable me to catch when this kind of problem happens in the script that runs webcoach.run(). If the check is too rough or if there is a better way to catch this error in Coach or my application, I would be happy to give writing an error check another go. I would really like to be able to catch this error in my own application, and I certainly want to give it back if it would be helpful to others.
Thank you for helping the coach!