search

karma-runner / karma-sauce-launcher

A Karma plugin. Launch any browser on SauceLabs!
MIT License
199 stars 103 forks source link

Switch launcher to selenium-webdriver #156

Closed devversion closed 5 years ago

devversion commented 5 years ago
devversion commented 5 years ago

A breaking change will be that node >= 8.9.0 is required.

devversion commented 5 years ago

Definitely. I've tried to add as much comments to unclear parts (there were a lot), but at some point it just was out of scope. Thanks for the review

jdmarshall commented 5 years ago

Feedback: The main reason I use this module instead of a particular 3rd party module is the flakiness of selenium-webdriver + saucelabs + sauce tunnels. Disconnects and unusual timeout errors were quite common (they aren't nonexistant with karma-sauce-launcher, but they happen less frequently).

It will represent a regression in behavior if we go back to that mess.

devversion commented 5 years ago

@jdmarshall Where do you have this information from? I couldn't find anything about that.. also it seemed to work as expected when I tried testing with selenium-webdriver?

I cannot think of any plausible reason why wd should run more stable than selenium-webdriver. Similarly I cannot proof that selenium-webdriver runs more stable than wd. A big point is just that there is an official first party library that seems to work as expected and is maintained.

devversion commented 5 years ago

Also wd has been added with the initial commit and didn't seem to have been used as a result of issues with selenium-webdriver (see here). Do you have some specific SHA in mind?

vikerman commented 5 years ago

Will merge and bump up the major version. Shouldn't affect anyone using the current version.

badeball commented 5 years ago

It seems like this PR broke the default value of the build configuration, as described in the readme. I'm experiencing that the TRAVIS_BUILD_NUMBER is no longer picked up and I cannot see it being referenced anymore. @devversion, can you confirm my suspicion?

devversion commented 5 years ago

@badeball That is correct. I intentionally removed that as the developer should explicitly specify the build name. We should update the README. good catch. thank you! A PR would be nice 😄

karmarunnerbot commented 4 years ago

:tada: This PR is included in version 2.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

drzraf commented 4 years ago

@devversion : 9a5a147 did not made it, regression in #217