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

Chromedriver support #541

Merged
merged 11 commits into from
Nov 3, 2017
Merged

Chromedriver support #541

merged 11 commits into from
Nov 3, 2017

Conversation

shmargum
Copy link
Contributor

Addresses #521
This is fully compatible with the existing config.
Includes support for awaiting async JS execution before capture.
Includes support for capturing a single element by css selector.
Includes support for capturing the full page when no height is specified.

@shmargum
Copy link
Contributor Author

there are 2 failing tests for chromedriver not being installed on Travis, I am not too sure how to address these

there are also 3 failing tests (which are also failing in master) for CasperJS:

      expected: "0.0"
      got: "invalid"

@shmargum
Copy link
Contributor Author

I have updated the travis config to install chromedriver, but I am not sure how to address the 3 failing casperjs tests.

@t4k
Copy link

t4k commented Sep 28, 2017

I tried making this work. I’m not a Ruby developer, but using Wraith with Chromedriver would be huge for me.

This PR requires a specific Ruby version and quickly became a pain because I don’t have RVM or rbenv on my Mac. Wraith works fine by itself with the system Ruby version. (ruby 2.0.0p648 (2015-12-16 revision 53162) [universal.x86_64-darwin15] on my machine.)

I tried to do what the instructions in #521 said, and was somewhat successful. The gem seemed to build and install, but I didn’t get any of the new Chrome template files, like I thought I should have based on the commits here after running wraith setup.

@shmargum
Copy link
Contributor Author

This is what a successful wraith setup should look like using this build:

wraith setup
      create  configs
      create  configs/capture.yaml
      create  configs/history.yaml
      create  configs/spider.yaml
      create  javascript
      create  javascript/cookies_and_headers--casper.js
      create  javascript/cookies_and_headers--phantom.js
      create  javascript/disable_javascript--casper.js
      create  javascript/disable_javascript--phantom.js
      create  javascript/interact--casper.js
      create  javascript/interact--chrome.js
      create  javascript/interact--phantom.js
      create  javascript/wait--casper.js
      create  javascript/wait--chrome.js
      create  javascript/wait--phantom.js

The chrome javascript samples are in there.

If you are talking about the capture yaml, you can update the one it generates to say "chrome" instead of "phantomjs"

You can have a look at the chrome capture yaml used in the specs: https://github.com/shmargum/wraith/blob/f74a10ebbe58024b5555fb1524ad347864fbc9c8/spec/configs/test_config--chrome.yaml

@shmargum
Copy link
Contributor Author

shmargum commented Oct 2, 2017

@ChrisBAshton not sure if you are the right person to approve/give feedback here, but just sending you a ping

There are 3 test failures here related to CasperJS which are also failing for me locally on master and am not too sure how I should address them, otherwise I think this PR is pretty complete.

@shmargum
Copy link
Contributor Author

shmargum commented Oct 3, 2017

or @wildlyinaccurate i don't know if you are the right person to ask for feedback here

@wildlyinaccurate
Copy link
Contributor

If this does Just Work™ with existing configs then that is awesome! I'm no longer with the BBC and don't use Wraith anymore, but I'll pass this along to some other BBC colleagues who might be interested in reviewing and testing the PR.

@kieranshaw
Copy link

I've also had a go with this fork locally the last couple of days and can confirm that it broadly works. The only change I had to make was to create the directories where each screenshot will go before taking the screenshot. I'm guessing I've had to do this as I'm targeting a non-existent subdirectory and I think phantomjs handled this ok before.

@shmargum shmargum closed this Oct 28, 2017
@shmargum shmargum reopened this Oct 28, 2017
@staylos92
Copy link
Contributor

staylos92 commented Oct 30, 2017

I've had a look at this locally and all of our existing configs still work as expected 👍 I'm going to hopefully get this merged and a new version of Wraith as soon as the Travis tests are passing 🤞

@shmargum
Copy link
Contributor Author

I am not too sure how to get the tests passing on travis, I've definitely tried a few things.
Any help is appreciated.
Thanks.

@shmargum
Copy link
Contributor Author

hooray i got the travis build passing
very long story short -- I added the --hide-scrollbars option to chrome

language: ruby
rvm:
- 2.3.3
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bumped the version since 2.3.3 is not installed by default on travis and 2.4.1 is

language: ruby
rvm:
- 2.3.3
before_install:
- gem update
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would update all the system gems -- not sure why we need it
bundler is always up to date by default, and this doesn't update the bundle which hasn't been installed yet

@JimmyMultani
Copy link

@bbcnews, any news on when this might be merged? Thanks!

@staylos92
Copy link
Contributor

Now that the Travis tests are passing we are aiming to merge this tomorrow and release a new version of Wraith at the same time.

@staylos92 staylos92 merged commit 85dc219 into bbc:master Nov 3, 2017
@wildlyinaccurate
Copy link
Contributor

Nice work on this 👌

@L0wry
Copy link

L0wry commented Nov 9, 2017

YEAHHH BOII 💪

@kentr
Copy link

kentr commented Feb 22, 2018

@shmargum Do you have any insight on how to get debug output with this setup? I need to see errors returned by the browser, by the driver, etc.

FWIW, in my case, the images are completely failing for my dev instance of the site on my local box. The site works fine when I visit it in the browser.

@shmargum
Copy link
Contributor Author

You are not seeing any errors? such as ERROR: Net::ReadTimeout

I would suggest to only do a single comparison at a time locally, as the full job can take a while.

Also the gem is configured to run with a concurrency of 8, and there is currently no way to configure this (currently), so this could also be causing issues; again if you just do one comparison at a time this won't be a problem.

@kentr
Copy link

kentr commented Feb 27, 2018

@shmargum

You are not seeing any errors? such as ERROR: Net::ReadTimeout

Yes, no errors in the output.

I would suggest to only do a single comparison at a time locally, as the full job can take a while.

I did this, with the same results. The local screenshot is completely blank, and the prod screenshot is normal.

Debugging the reason for the blank page is my end-goal, but to do that I'd like to know how to see the errors from the browser. They're getting swallowed somewhere.

Since you know more about the components involved in using Chromedriver I thought you might have insight on how to bring the errors to the surface.

@shmargum
Copy link
Contributor Author

Hmm I am not sure what the reason is.
Sounds like the issue might be with chrome accessing your local server. This can be related to your local setup or maybe the config.
Can you paste your config? Maybe open a new issue as well. See https://github.com/BBC-News/wraith/blob/master/.github/ISSUE_TEMPLATE.md
If you'd like to step through the code with binding.pry these are the related chrome lines:
https://github.com/BBC-News/wraith/blob/master/lib/wraith/save_images.rb#L93-L137
this is the entry point to the chrome code:
https://github.com/BBC-News/wraith/blob/master/lib/wraith/save_images.rb#L124

@patricknelson
Copy link

patricknelson commented Oct 2, 2018

How stable is this? @shmargum do you think it’s worth gathering up some of your knowledge thus far and writing up documentation for gh-pages to reflect setup and config of this feature now? That way the world can more easily know about the new path forward, since PhantomJS/CasperJS are practically dead 💀

I actually started in the docs, which made no mention of the modernized Chromedriver support and ended up in #364 but found that some of this work has now been done in this PR. I’m not a ruby dev myself either, but I’d be potentially interested in picking this up, especially if it were to have Windows and/or Linux support at some point in the future (if not already).

@shmargum
Copy link
Contributor Author

shmargum commented Oct 2, 2018

I'd say this is pretty stable; I've been running automated screenshot tests on chrome on a linux jenkins box after successful staging deployments to compare our staging and prod env for about a year now. The only dependencies are ruby and chrome, so I would expect this to work on Windows as well. This should even be able to run in docker, though I only recently started using docker myself.

I can try to add some docs for the gh pages, not sure where to do that though.

I'd also be down with stripping out some of the phantomjs code to clean things up

@patricknelson
Copy link

patricknelson commented Oct 13, 2018

@shmargum

I can try to add some docs for the gh pages, not sure where to do that though.

You can do that by doing a pull request against the gh-pages branch (which is the basis for the site at http://bbc-news.github.io/wraith/).

p.s. I'm actually hacking this into my own VQA regression testing workflow right now. Curious: Do you know where to start if I wanted to find a way to add the before_capture hook into this new chrome browser type? Would this be configured somehow in Chromedriver?

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

Successfully merging this pull request may close these issues.

None yet

9 participants