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

Support Firefox via geckodriver #65

Merged
merged 1 commit into from
Mar 23, 2017

Conversation

jmbowman
Copy link
Contributor

Add support for using needle with recent versions of Firefox via geckodriver:

  • Try rect from the W3C WebDriver spec before falling back to the Selenium location and size calls if necessary (geckodriver doesn't support the latter).
  • Try directly getting a screenshot of the target element (supported for some drivers starting in selenium 2.46.1) before falling back to cropping a full-page screenshot if necessary. The fallback doesn't work correctly in geckodriver, so we can't just consistently use the legacy approach for all drivers.
  • Update the tox configuration to simplify testing with Firefox and Chrome. Both pass all tests for me locally.
  • Update the Travis configuration to also test Firefox/geckodriver with the latest selenium release. The selenium 2.x releases have a bug that often prevents Firefox from launching correctly, so I don't recommend using or testing that combination. The additional setup needed to run Firefox and geckodriver is skipped for PhantomJS test workers.

This seems like the minimum necessary step towards working, tested geckodriver support, but let me know if you want to split it up further into smaller PRs.

@jphalip
Copy link
Collaborator

jphalip commented Mar 22, 2017

Thanks a lot, this is great. Just a few comments:

  • It seems that the changes attempt to run the tests with chromedriver, but that doesn't seem to be taken into consideration in the travis configuration file. Should that be added, and does chromedriver also need to be installed?
  • Could you add a comment in the Travis conf about why tests aren't run with Firefox and Selenium 2.x?
  • If it's not too much hassle then it'd be great if you could break up the changes into separate logical commits.

Thanks again!

@jmbowman
Copy link
Contributor Author

I added support for testing chrome on local machines via tox, but didn't attempt to configure Travis to test it yet; I figured that would be a later PR.

Do you want a separate PR for each file? The changes to driver.py can't be verified without the changes to at least one of the other two files, although I could commit them separately now that I've tested them. And the test suite won't pass for Firefox either locally or in Travis without all of those changes in the driver module.

@jphalip
Copy link
Collaborator

jphalip commented Mar 22, 2017

Ok I see, no problem. I think it's ok to merge everything at once then. I'd just like to make sure chromedriver is covered for extra peace of mind. From a quick search, it seems it'd be fairly simple to install it on Travis: travis-ci/travis-ci#272 (comment)
Do you confirm?

@jmbowman
Copy link
Contributor Author

Hmm, that doesn't seem to have actually used the updated .travis.yml file. Don't have time to follow up on it right now, but I'll look into it soon.

@jmbowman
Copy link
Contributor Author

Not sure if it didn't like where I originally added the comment or it was just a force-push related hiccup, but looks like it worked this time. Let me know if you spot any other problems.

@jphalip jphalip merged commit b317302 into python-needle:master Mar 23, 2017
@jphalip
Copy link
Collaborator

jphalip commented Mar 23, 2017

Excellent, thanks again for your work!

@jphalip
Copy link
Collaborator

jphalip commented Mar 23, 2017

@jmbowman It looks like there is one failure after the merge. Do you know what that is?
https://travis-ci.org/bfirsh/needle/jobs/214317627

@jmbowman
Copy link
Contributor Author

It looks like the first test that tried to launch the browser failed, but all the later ones succeeded; maybe it just wasn't ready yet? I've seen some other Travis setups put in an explicit wait of 3 seconds to wait for xvfb to finish initializing; I thought the other subsequent setup tasks would take long enough for it to finish, but maybe not. I'll create a separate PR to both add that delay and add some retry logic with a timeout to get_web_driver().

@jmbowman jmbowman deleted the geckodriver_support branch March 23, 2017 19:38
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.

2 participants