sitespeedio / coach

Clear Eyes. Full Hearts. Can’t Lose.
MIT License
1.21k stars 64 forks source link

Check for Browsertime errors in run(). #300

Closed jadonn closed 5 years ago

jadonn commented 5 years ago

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.

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!

soulgalore commented 5 years ago

Hi @jadonn thanks for the fix! Let me merge this before next release!

soulgalore commented 5 years ago

I need to think a little how we should handle it, I'm not 100% sure yet.

jadonn commented 5 years ago

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.

soulgalore commented 5 years ago

Yeah what about just throw an error? That what happens right now if Browsertime has some problems or ... maybe need to check :)

jadonn commented 5 years ago

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.

soulgalore commented 5 years ago

Let me fix that so we can move on with the release :) Thank you @jadonn !

jadonn commented 5 years ago

Thank you so much @soulgalore ! I apologize I was not able to make the correction in good time