[Issue 415] Add support for python 3

[AndreiH]

This PR is for Issue #415 to add support for Python 3

[coveralls]

Coverage increased (+0.1%) to 92.439% when pulling 6ff7006 on AndreiH:mozdownload_python3 into f1c43aa on mozilla:master.

[AndreiH]

@whimboo , can you take a look over this ?
Thank you !

[whimboo]

Hi @AndreiH. It's great to see that you pushed a first attempt for the Python3 support! Thanks a lot.

Before I'm going to review this patch we should make sure that at least Travis CI also runs a job for Python3 for both Linux and Mac. This can be done by updating https://github.com/mozilla/mozdownload/blob/master/.travis.yml for the new platforms.

For appveyor it might also be simple but we would most likely need a matrix in https://github.com/mozilla/mozdownload/blob/master/.appveyor.yml.

[whimboo]

@AndreiH I have seen that you already modified the travis CI config. Please make sure to keep Python2.7 in the matrix.

[AndreiH]

Thank you @whimboo , I have updated the matrix as it was before.
It seems that there are some test failures that I need to fix before I can ask for the full review.

[whimboo]

@AndreiH now you removed Python3 again and only run Python2.7 jobs. What we want is to have both 2.7 and at least 3.6 tests running.

[whimboo]

Travis CI still doesn't pick up those other jobs. I assume that is because you changed the indentation from 4 to only 2. Make sure to revert it, and it might fix that particular problem.

[AndreiH]

Hello @whimboo !
I have updated the changes.
I fixed all tests except one, that I am not really sure why it is failing and I could not find a fix for it.

From debugging and investigating I have reached the conclusion that it's because treeherder-client library. Which does not entirely support Python3 yet. BUG -> https://bugzilla.mozilla.org/show_bug.cgi?id=1330474
I see that they have released version 5.0.0, but it still fails.

The single test that is failing is -> https://github.com/AndreiH/mozdownload/blob/master/tests/treeherder/test_api.py#L59

Fails with:

self = <test_api.TestAPI testMethod=test_query_tinderbox_builds>

    def test_query_tinderbox_builds(self):
        self.httpd.router.register('GET', '/api/*', handle_rest_api)
    
        for platform in PLATFORM_MAP:
            # mac64 is identical to mac
            if platform == 'mac64':
                continue
    
            application = 'firefox' if not platform.startswith('android') else 'mobile'
            th = Treeherder(application, 'mozilla-beta', platform,
                            server_url='http://{}:{}'.format(self.httpd.host, self.httpd.port))
            builds = th.query_builds_by_revision('29258f59e545')
    
>           self.assertEqual(len(builds), 1)
E           AssertionError: 0 != 1

tests/treeherder/test_api.py:72: AssertionError
------------------------------------------------------------------------------------------------ Captured log call -------------------------------------------------------------------------------------------------
client.py                  500 ERROR    HTTPError 500 requesting http://127.0.0.1:8080/api/project/mozilla-beta/jobs/?job_type_name=Build&result_set_id=5319&exclusion_profile=False&build_platform=linux64&option_collection_hash=102210fe594ee9b33d82058545b1ed14f4c8206e: b'{"error": {"code": 500, "message": "Traceback (most recent call last):\\n  File \\"/home/andreihutusoru/mozdownload/.tox/py35/lib/python3.5/site-packages/wptserve/handlers.py\\", line 312, in __call__\\n    rv = self.func(request, response)\\n  File \\"/home/andreihutusoru/mozdownload/.tox/py35/lib/python3.5/site-packages/wptserve/handlers.py\\", line 354, in handle_request\\n    value = enc(rv)\\n  File \\"/usr/lib/python3.5/json/__init__.py\\", line 230, in dumps\\n    return _default_encoder.encode(obj)\\n  File \\"/usr/lib/python3.5/json/encoder.py\\", line 198, in encode\\n    chunks = self.iterencode(o, _one_shot=True)\\n  File \\"/usr/lib/python3.5/json/encoder.py\\", line 256, in iterencode\\n    return _iterencode(o, 0)\\n  File \\"/usr/lib/python3.5/json/encoder.py\\", line 179, in default\\n    raise TypeError(repr(o) + \\" is not JSON serializable\\")\\nTypeError: <filter object at 0x7feedaef2550> is not JSON serializable\\n"}}'
treeherder.py              105 ERROR    Failure occurred when querying Treeherder for builds
Traceback (most recent call last):
  File "/home/andreihutusoru/mozdownload/mozdownload/treeherder.py", line 96, in query_builds_by_revision
    jobs = self.client.get_jobs(self.branch, **kwargs)
  File "/home/andreihutusoru/mozdownload/.tox/py35/lib/python3.5/site-packages/thclient/client.py", line 684, in get_jobs
    return self._get_json_list(self.JOBS_ENDPOINT, project, **params)
  File "/home/andreihutusoru/mozdownload/.tox/py35/lib/python3.5/site-packages/thclient/client.py", line 490, in _get_json_list
    return self._get_json(endpoint, project=project, **params)["results"]
  File "/home/andreihutusoru/mozdownload/.tox/py35/lib/python3.5/site-packages/thclient/client.py", line 497, in _get_json
    resp.raise_for_status()
  File "/home/andreihutusoru/mozdownload/.tox/py35/lib/python3.5/site-packages/requests/models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http://127.0.0.1:8080/api/project/mozilla-beta/jobs/?job_type_name=Build&result_set_id=5319&exclusion_profile=False&build_platform=linux64&option_collection_hash=102210fe594ee9b33d82058545b1ed14f4c8206e

I currently xfailed with: @pytest.mark.skip(reason="Bug 1330474 - Switch Treeherder from Python 2.7 to Python 3.6")
But I think I need to post another Bug for treeherder-client.

Please let me know what you think and if you have any idea how to fix this test.

Thank you !

[whimboo]

@edmorley mind having a look at the last comment from @AndreiH if the assessment is right? Maybe we need a workaround for now until thclient has been updated.

[edmorley]

@edmorley mind having a look at the last comment from @AndreiH if the assessment is right? Maybe we need a workaround for now until thclient has been updated.

Hi!

treeherder-client is currently tested under Python 3 in the Treeherder repository and passes our tests (albeit coverage isn't great), as well as passing flake8 and having been checked using pylint's py3 mode - hence us marking it as Python 3 compatible in the classifiers.

Looking at the test failure in #536 (comment), I see:

requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http://127.0.0.1:8080/api/project/mozilla-beta/jobs/?....

It looks like the handle_rest_api test utility in this repo is generating the HTTP 500, not anything to do with treeherder-client? (My hunch would be that the legacy urlparse usage is what's causing the HTTP 500)

Is there a reason that the custom handle_rest_api() is being used instead of an existing solutution like responses?

[edmorley]

(Comment edited to add the missing parts, whoops)

[AndreiH]

Thank you for the responses @edmorley @whimboo !
I have managed to fix the failing test, it was something that I needed to change indeed in handle_rest_api() method. It was NOT something to do with treeherder-client.
https://github.com/mozilla/mozdownload/pull/536/files#diff-862a7ed7e988689f6ae6678761b9ee14R49
I have never thought of looking into that method. Thanks for the clue.

The PR is now ready for a full review @whimboo !
Travis now has all the running ENVs required, I have changed a bit how we run Travis, as we now have more ENVs.

[whimboo]

That is great work @AndreiH! It looks mostly fine, but I have a couple of questions regarding the Travis CI changes, and some nits which need to be fixed. Please have a look at those.

[whimboo]

It would have been easier to review when you wouldn't do a force push. But it looks way better now.

[whimboo]

Due you know for which value of date this fails? Which test is causing that?

[AndreiH]

Well, I see in the stacktrace that there are already the args and values which the tests fail with: https://travis-ci.org/mozilla/mozdownload/jobs/503557491

[whimboo]

As best pick one of those failing tests and run it alone by using the -k option like tox -- -k test_name. Also specify -s so that you get all the output. I would like to know what date.time() differs in between Python2.7 and Python3.

[AndreiH]

Test test_build_indices
I don't know why -s command did what it should do. I add it at the end: tox -e py27 -- -k test_build_indices > output.txt -s

Python 3.6: https://gist.github.com/AndreiH/d8cb89dd509a7a4733d58ba706fbdb27
Python 2.7: https://gist.github.com/AndreiH/7b32e6c9a22cfb2212d58ab5d5602769

[whimboo]

So here is the snippet for your testing: https://gist.github.com/whimboo/63ac15a7029fa2b6be9da33858fb0a91

Note the different behavior between Python2 and Python3. We have to get it fixed, and comparing to 00:00:00 is not a good idea given that at this second we also could have a sub folder.

[whimboo]

@AndreiH will you have a chance to continue on this PR? Note, that there are also conflicts.

[AndreiH]

@AndreiH will you have a chance to continue on this PR? Note, that there are also conflicts.

@whimboo Unfortunately I cannot update this on a daily basis, my time is very limited now to work on this.
Hope this is good enough to go ahead with a merge. I know the fix and date.strftime('%H-%M-%S') != '00-00-00' is not a good one for now. Maybe next weekend I can take a look. Thanks !

[whimboo]

We can wait a bit, no problem. There are also remote test failures I would like to see fixed first before merging this PR.

[whimboo]

@whimboo Unfortunately I cannot update this on a daily basis, my time is very limited now to work on this.
Hope this is good enough to go ahead with a merge. I know the fix and date.strftime('%H-%M-%S') != '00-00-00' is not a good one for now. Maybe next weekend I can take a look. Thanks !

Yes, let take it for now. It would also fail in Python2 for exactly this time, so that it isn't a regression.

Note that we are going to skip the remote TB tests which cause those failures via #550. Once that landed I think we can also merge this PR. I just want to have a green test suite first.

[whimboo]

@AndreiH would you mind to rebase your branch against the latest master, and repush? I would like to see how the test failure rate has been improved now with PR #554 landed. Thanks.

[whimboo]

As just noticed the newly added packaging package hadn't been added to the requirements.txt file. As such all the release candidate tests were failing. By adding this now both Travis and Appveyor are happy.

[whimboo]

@AndreiH thanks again for working on this patch! Very much appreciated to see py3 support coming up for the next mozdownload release.