Skip to content
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

Error handling improvements for Update #488

Open
wants to merge 2 commits into
base: legacy
Choose a base branch
from

Conversation

thw0rted
Copy link

@thw0rted thw0rted commented Mar 17, 2021

Fixes #487. The main change is to try to preserve Promise rejections throughout the Update process and handle them correctly at the end.

I had to make a few small changes to get test-unit passing before I wrote any new code. I also removed some dead code and made a couple of error messages more accurate.

Once I started changing tests to incorporate the better error-handling behavior, I found that the old version of Jasmine being used handled Promises poorly, so I updated that to latest, which required a new Typescript, which required new types for Node and q.

Fun fact: running tsc inside prepublish means that if you install a package that's incompatible with the installed version of Typescript, it's impossible to roll back. Because prepublish runs before npm install -- as dumb as that is -- your can't run install once your Typescript build is broken. I renamed to prepack so that format-enforce and the build step will still run before publishing. This is also why there are a lot of whitespace changes in this PR -- I ran format trying to get format-enforce to pass, and it wanted to change indentation and wrapping, especially on a bunch of Promise chains.

Between the dependency updates and the format changes, this wound up being a significantly bigger change than I originally intended. I think all the changes are a net positive but understand if you want to push back or split it into multiple requests.

Only assign versionCustom for binaries that will actually be used
Build+copy before running test-unt by itself
@google-cla
Copy link

google-cla bot commented Mar 17, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Mar 17, 2021
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@thw0rted
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Mar 17, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Fail when requesting chromedriver version that isn't in the manifest.
Don't swallow downloader errors.
Fail loudly if trying to download a Binary with no URL.
Exit with nonzero status on update error.
Update to latest Jasmine for improved Promise handling in specs.
@thw0rted
Copy link
Author

I think the test won't run on CircleCI because the updated version of Jasmine may need a newer Node -- the minimum Node for Jasmine 3.x is v10. You wouldn't have to change the engines in package.json for consumers of this project, the new requirement would only be for running tests. Of course, nobody could blame you for dropping support for Node versions before v10, which itself is now two LTS cycles old and hits EOL in a few months.

I'm not familiar with CircleCI so I wasn't able to figure out how to view/change the CI config. (It shows me as being in the "angular" group when I look at the test results, but if I click on "Projects" in the sidebar, it takes me to an error page. Same with trying to look at the job's Configuration File...)

@thw0rted
Copy link
Author

Ping @cnishina, you seem to be the only active contributor left on this project? I saw in the v12 release notes that the team is "figuring out the best future for Protractor" and I believe this project is a part of Protractor, right? Would it be safe to assume the "future" of webdriver-manager is also uncertain?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhandled promise rejection when trying to install non-existent browser version
2 participants