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

Conform to the api of urllib2 for adding header for a request #3803

Merged
merged 3 commits into from
Apr 12, 2017

Conversation

clubfest
Copy link
Contributor

urllib2 capitalize the header key and relies on this capitalization to decide if the 'Content-type' field is filled in and if not, fill it in with the default of 'application/x-www-form-urlencoded'. A recent change causes the header in the request named 'Content-type' to change to 'Content-Type' and urllib2 assumes the 'Content-type' field in the request header is not filled in, so tests are using the default instead and are giving the error:
<class 'selenium.common.exceptions.WebDriverException'>: Message: Given content type 'application/x-www-form-urlencoded' is not 'application/json;charset=UTF-8'.

urllib2 capitalize the header key and relies on this capitalization to decide whether to fill in the Content-type using the default of 'application/x-www-form-urlencoded'. If using this default, it causes tests to err with:
<class 'selenium.common.exceptions.WebDriverException'>: Message: Given content type 'application/x-www-form-urlencoded' is not 'application/json;charset=UTF-8'.
@isaulv
Copy link
Contributor

isaulv commented Apr 10, 2017

@clubfest This is pretty good. Have you tested this under Python 2 and Python 3?

@clubfest
Copy link
Contributor Author

It works for the webdriver tests that I have in Python 2 that were failing with the error.

@isaulv
Copy link
Contributor

isaulv commented Apr 10, 2017

@clubfest would it be any trouble to test this change with Python 3 (3.5+)? You can create a virtual environment and just write a short script to confirm.

@davehunt
Copy link
Contributor

Could you also provide a test in https://github.com/SeleniumHQ/selenium/blob/master/py/test/unit/selenium/webdriver/remote/test_remote_connection.py so we don't regress again? You can then run this test against Python 2/3 using Tox as detailed here: https://github.com/SeleniumHQ/selenium/wiki/Python-Bindings#tests

@clubfest
Copy link
Contributor Author

It's a bit difficult to add a test for this currently, unless I create an independent function that returns the request from line 495
https://github.com/clubfest/selenium/blob/cbf367b5506ad02354eb004ef7cc86fa7e5a0352/py/selenium/webdriver/remote/remote_connection.py#L495

Or is it sufficient to add a comment to say that the request header must be modified with add_header instead of modifying the headers field?

@clubfest
Copy link
Contributor Author

I am a bit confused. Many of the tests that failed are in ruby, but the change is only for python. Is there a way to re-run the continuous integration?

@lmtierney
Copy link
Member

@clubfest ignore the ruby test failures

@clubfest
Copy link
Contributor Author

I am unable to run tests locally with tox. Am I missing some additional setup?
$ tox -e py27-unit

logs:
...
error: can't copy 'selenium/webdriver/firefox/x86/x_ignore_nofocus.so': doesn't exist or not a regular file
Running setup.py install for selenium: finished with status 'error'
...

@lmtierney
Copy link
Member

Yes, to get that file you need to follow the instructions here

Specifically ./go py_prep_for_install_release

@clubfest
Copy link
Contributor Author

I ran ./go py_prep_for_install_release, and then encounter another error:
Running command python setup.py egg_info
Traceback (most recent call last):
File "", line 1, in
File "/tmp/pip-QOi0I0-build/setup.py", line 35, in
'long_description': open(join(abspath(dirname(file)), "README.rst")).read(),
IOError: [Errno 2] No such file or directory: '/tmp/pip-QOi0I0-build/README.rst'

$ ls /tmp/pip-QOi0I0-build/
CHANGES pip-delete-this-directory.txt PKG-INFO selenium.egg-info setup.py
MANIFEST.in pip-egg-info selenium setup.cfg

@clubfest
Copy link
Contributor Author

Never mind. Just upgraded via pip install -U setuptools pip.

@clubfest
Copy link
Contributor Author

For the remaining failures, I don't think they are related to my changes. Please take a look, thanks!

@davehunt
Copy link
Contributor

Thanks!

@davehunt davehunt merged commit 06e35ac into SeleniumHQ:master Apr 12, 2017
@clubfest clubfest deleted the patch-2 branch April 12, 2017 12:29
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.

4 participants