Add support for "Remote" Chrome browser

volodymyr-mykhailyk commented 4 years ago

Extend Launcher with an option to connect to existing Chrome instance instead of starting Chrome on the local host.

This is useful if tests are running in the CI environment and instead of installing Chrome into Docker/Host - CI launching prebuilt chrome image and Apparition using it.

Additional items:

Open for any feedback on code organization even though tried to maintain the existing structure as much as possible
Local chrome launcher sleeps 3 seconds before retrieving ws_url. This slows down every browser start. Not sure about the purpose - so left it intact.
ChromeClient has get_ws_url function not used anywhere. Duplicated it in Remote launcher because I wasn't sure about an original method purpose.

volodymyr-mykhailyk commented 4 years ago

Still trying to figure out why all the tests are failing on Travis while working locally

volodymyr-mykhailyk commented 4 years ago

Hi @twalpole, I will probably need your help figuring out why majority of tests are failing right now. I tried running the tests on the latest master without my changes - results are the same.

twalpole commented 4 years ago

@volodymyr-mykhailyk master should run tests again. The linux distribution specified in .travis.yml was no longer receiving the latest chrome. Please rebase.

volodymyr-mykhailyk commented 4 years ago

@twalpole Thanks. I will update the branch.

In the background - I tried migrating to GitHub Actions and have running build. Are you interested in such a change? Can submit another PR

twalpole commented 4 years ago

@volodymyr-mykhailyk Re: Github actions - To what benefit? If there is a decent benefit to using Github actions then possibly, but I'm not interested in moving to Github actions just because it's new.

volodymyr-mykhailyk commented 4 years ago

@twalpole No particular benefit. GitHub build runners just have more software installed including latest chrome. Tried this out to make sure problem not in my code.

Back to this PR. Changes are ready, except few more failing tests unrelated to the change. Please let me know how I should proceed.

As part of this PR also included two changes

1st commit - fixing debug statements
2nd commit - fixing incorrect pause logic (at least my attempt). The problem is after input becomes available and has enough data to read, even though it doesn't end with a new line - pause was canceled. I detected this on the GitHub Actions runner. Seems like input stream there behaves differently

Looking into the additional fails - figured that wait_for_loaded method could cause similar issues as in selenium driver I was fixing recently. Is there a reason why you are not using Document.readyState to detect when the page and DOM is loaded?

twalpole commented 4 years ago

@volodymyr-mykhailyk Because the whole concept of "is loaded" is poorly defined - and every method used has pros and cons

volodymyr-mykhailyk commented 4 years ago

@twalpole Added your recent Rubocop changes to my branch. Changes are ready, except few failing tests unrelated to the change. Please let me know how I should proceed. Thanks

twalpole commented 4 years ago

Other than my caution around the pausing code, which I need to test on multiple platforms - could be please fix the conflict that currently exits. Thanks

volodymyr-mykhailyk commented 4 years ago

Other than my caution around the pausing code, which I need to test on multiple platforms - could be please fix the conflict that currently exits. Thanks

I understand. This fix was really not needed as part of this PR. Skipped two unrelated commits to the change. If you will even have problem with the pause method - feel free to ping me, I can try to help.

twalpole commented 4 years ago

Thanks

twalpole / apparition

Add support for "Remote" Chrome browser #52