python-needle / needle

Automated tests for your CSS.
https://needle.readthedocs.io/
Other
590 stars 50 forks source link

Support Firefox via geckodriver #65

Closed jmbowman closed 7 years ago

jmbowman commented 7 years ago

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

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 commented 7 years ago

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

Thanks again!

jmbowman commented 7 years ago

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 commented 7 years ago

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: https://github.com/travis-ci/travis-ci/issues/272#issuecomment-283968954 Do you confirm?

jmbowman commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Excellent, thanks again for your work!

jphalip commented 7 years ago

@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 commented 7 years ago

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().