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

Update minimum splinter version and remove phantomjs support #110

Merged
merged 6 commits into from Aug 30, 2018
Merged

Update minimum splinter version and remove phantomjs support #110

merged 6 commits into from Aug 30, 2018

Conversation

jsfehler
Copy link
Contributor

Splinter 0.9.0 removes phantomjs support, so pytest-splinter should as well.

Copy link
Member

@bubenkoff bubenkoff left a comment

Choose a reason for hiding this comment

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

in requirements-testing there's a selenium version pinned with <= to support all the browser manipulations
with the latest selenium those stopped working
so this PR forces to update also the selenium
you can try to resolve the root issue of course
i had no luck with it: #100

Copy link
Member

@bubenkoff bubenkoff left a comment

Choose a reason for hiding this comment

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

.travis.yml also has pinned selenium version

@@ -322,7 +322,7 @@ def get_args(driver=None,
# https://github.com/mozilla/geckodriver#firefox-capabilities
kwargs['moz:firefoxOptions'] = driver_kwargs.get('moz:firefoxOptions', {})
kwargs['moz:firefoxOptions']['profile'] = profile.encoded
elif driver in ('phantomjs', 'chrome'):
elif driver in ('chrome'):
Copy link
Member

Choose a reason for hiding this comment

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

missing trailing comma to be a tuple

@jsfehler
Copy link
Contributor Author

@bubenkoff I've skipped the download_file tests, due to the following firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1366035

Disabling the save file dialog will allow the file to download, but the browser hangs afterwards.

Also, I had to make a change in patch_webdriver() to use urllib3. I'm not terribly familiar with the inner workings of selenium's RemoteConnection or urllib3's PoolManager, so tell me if I got it wrong. I didn't include any py27 support yet, Do you have a preferred way you want to handle that? Just include urllib3 as a dependency?

This gets us down to one error, in test_restore_browser[firefox-2] but I can't make heads or tails of it. Any ideas?

@coveralls
Copy link

coveralls commented Aug 29, 2018

Coverage Status

Coverage decreased (-0.6%) to 88.087% when pulling ffd0f5c on jsfehler:no_phantom_js into c9832a1 on pytest-dev:master.

@jsfehler
Copy link
Contributor Author

@bubenkoff And all fixed, although I still need to handle py27 and urllib3. It might be worth it to make this a 1.10.0 or 2.0 to indicate that phantomjs support is gone and we're using up to date selenium and splinter.

@bubenkoff
Copy link
Member

@jsfehler great effort, thank you very much!
about download file tests -> but then should we wait for the issue being resolved? Otherwise file downloads testing will be practically impossible
about urllib3 and test_restore_browser[firefox-2] -> this is related. This test intentionally quits the browser and it should automatically be started to avoid weird situations of the lost browser (happens)
and because now urllib3 is used, connection exceptions are different and this code fails to cath them:

        try:
            if splinter_webdriver not in browser.driver_name.lower():
                raise IOError('webdriver does not match')
            if hasattr(browser, 'driver'):
                browser.driver.implicitly_wait(splinter_selenium_implicit_wait)
                browser.driver.set_speed(splinter_selenium_speed)
                browser.driver.command_executor.set_timeout(splinter_selenium_socket_timeout)
                browser.driver.command_executor._conn.timeout = splinter_selenium_socket_timeout
                if splinter_window_size and splinter_webdriver != "chrome":
                    # Chrome cannot resize the window
                    # https://github.com/SeleniumHQ/selenium/issues/3508
                    browser.driver.set_window_size(*splinter_window_size)
            try:
                browser.cookies.delete()
            except (IOError, HTTPException, WebDriverException):
                LOGGER.warning('Error cleaning browser cookies', exc_info=True)
            for url in splinter_clean_cookies_urls:
                browser.visit(url)
                browser.cookies.delete()
            if hasattr(browser, 'driver'):
                browser.visit_condition = splinter_browser_load_condition
                browser.visit_condition_timeout = splinter_browser_load_timeout
                browser.visit('about:blank')
        except (IOError, HTTPException, WebDriverException):

about urllib3 for python27, we need to use exactly the same approach as selenium itself (and it uses urlib3
https://github.com/SeleniumHQ/selenium/blob/master/py/setup.py

Allow passing args to pytest

Add MaxRetryError to caught exceptions

Add urllib3 to install requirements
@jsfehler
Copy link
Contributor Author

@bubenkoff For file downloading, the bug's been open for quite a while now. Anyone using a modern version of Firefox/Selenium/Geckodriver is going to encounter it. While this feature would be broken for anyone using pytest-splinter, it's also broken for everyone.

Personally, controlling the browser to download files and verify their presence on disk isn't something I'd do. I'd instead verify the download url on the page, then in another test I'd use requests to download the file and verify the contents. However I wouldn't want to surprise anyone who upgrades pytest-splinter. Up to you if you want to wait or not.

Everything's passing on TravisCI, so there's that at least.

@bubenkoff
Copy link
Member

@jsfehler makes total sense to me
thanks again for all the hard work

@bubenkoff
Copy link
Member

@jsfehler please squash & merge

@bubenkoff
Copy link
Member

@jsfehler had to restart the job so it could report the status

@bubenkoff
Copy link
Member

@jsfehler added you on pypi so you can release it (pls don't forget to make a tag on github)

@jsfehler jsfehler merged commit 63d8543 into pytest-dev:master Aug 30, 2018
@bubenkoff
Copy link
Member

@jsfehler please make a 2.0.0 release as there are breaking changes

@bubenkoff
Copy link
Member

@jsfehler ok made it myself, next time please take into account that we're trying to follow major.minor.patch versioning

@jsfehler
Copy link
Contributor Author

@bubenkoff Sorry about that, thanks for cleaning it up so fast.

@jsfehler jsfehler deleted the no_phantom_js branch August 31, 2018 17:49
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

3 participants