)]}'
{"jenkins/__init__.py":[{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":620,"context_line":"        except urllib2.HTTPError:"},{"line_number":621,"context_line":"            raise JenkinsException(\u0027job[%s] number[%d] does not exist\u0027"},{"line_number":622,"context_line":"                                   % (name, number))"},{"line_number":623,"context_line":"        except ValueError:"},{"line_number":624,"context_line":"            raise JenkinsException("},{"line_number":625,"context_line":"                \u0027Could not parse JSON info for job[%s] number[%d]\u0027"},{"line_number":626,"context_line":"                % (name, number)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_bcb76fd7","side":"PARENT","line":623,"updated":"2014-05-14 09:10:41.000000000","message":"This code was never used?","commit_id":"d5f09282ea7b65669fc09397c39c64add66c5899"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":620,"context_line":"        except urllib2.HTTPError:"},{"line_number":621,"context_line":"            raise JenkinsException(\u0027job[%s] number[%d] does not exist\u0027"},{"line_number":622,"context_line":"                                   % (name, number))"},{"line_number":623,"context_line":"        except ValueError:"},{"line_number":624,"context_line":"            raise JenkinsException("},{"line_number":625,"context_line":"                \u0027Could not parse JSON info for job[%s] number[%d]\u0027"},{"line_number":626,"context_line":"                % (name, number)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_6988aec5","side":"PARENT","line":623,"in_reply_to":"7adec928_bcb76fd7","updated":"2014-05-15 17:22:49.000000000","message":"Nope. It had zero coverage when I ran tests and measured coverage.\n\nI think what happened is that it was copy/pasted from other methods in this file. However other methods, call json.loads which will raise a ValueError for invalid JSON, but this method doesn\u0027t call json.loads because Jenkins just returns the console output as a simple string.\n\nNote that the exception it raises is \"Could not parse JSON info...\" but there is no other mention of JSON in this function and the return value according to the doc string is `str`.","commit_id":"d5f09282ea7b65669fc09397c39c64add66c5899"},{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":49,"context_line":"from httplib import BadStatusLine"},{"line_number":50,"context_line":"import json"},{"line_number":51,"context_line":"import urllib"},{"line_number":52,"context_line":"from urllib2 import Request, HTTPError, urlopen"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"LAUNCHER_SSH \u003d \u0027hudson.plugins.sshslaves.SSHLauncher\u0027"},{"line_number":55,"context_line":"LAUNCHER_COMMAND \u003d \u0027hudson.slaves.CommandLauncher\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_5cc66b4a","line":52,"updated":"2014-05-14 09:10:41.000000000","message":"In OpenStack, there is a rule to only import modules, not symbols:\nhttp://docs.openstack.org/developer/hacking/#imports\n\nI don\u0027t know if python-jenkins respects these rules or not, but it may help to respect them if you would like to move from StackForge to the OpenStack project.\n\n(I don\u0027t like this rule, but I don\u0027t want to discuss them here :-))","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":49,"context_line":"from httplib import BadStatusLine"},{"line_number":50,"context_line":"import json"},{"line_number":51,"context_line":"import urllib"},{"line_number":52,"context_line":"from urllib2 import Request, HTTPError, urlopen"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"LAUNCHER_SSH \u003d \u0027hudson.plugins.sshslaves.SSHLauncher\u0027"},{"line_number":55,"context_line":"LAUNCHER_COMMAND \u003d \u0027hudson.slaves.CommandLauncher\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_8e018008","line":52,"in_reply_to":"7adec928_5cc66b4a","updated":"2014-05-15 17:22:49.000000000","message":"Acknowledging your comment. I think I\u0027m going to keep it this way for now and maybe revisit this later. Reasons:\n\n1. Changing this will create merge conflicts with my python3_squash change (https://review.openstack.org/90114)\n\n2. python-jenkins only very recently came to stackforge; it used to live on LaunchPad as a non-OpenStack project. I don\u0027t think it conformed to OpenStack rules and I think it should be given time to conform to rules, if that is even desired.\n\n3. Even jenkins-job-builder, which is an OpenStack Infra project does not follow all the rules like this enforced by \"hacking\". James E. Blair said that he didn\u0027t want to make Infra projects have to follow these rules (I think the idea was to make the barrier/resistance for outside contributors lower?) See https://review.openstack.org/86679 and https://github.com/openstack-infra/jenkins-job-builder/blob/master/tox.ini#L38\n\n4. I like consistent style in code, but I think it\u0027s more important to get test coverage first. This will get us test coverage and then we can be more confident about other changes.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":175,"context_line":"        :returns: dictionary of job information"},{"line_number":176,"context_line":"        \u0027\u0027\u0027"},{"line_number":177,"context_line":"        try:"},{"line_number":178,"context_line":"            response \u003d self.jenkins_open(Request("},{"line_number":179,"context_line":"                self.server + JOB_INFO % locals()))"},{"line_number":180,"context_line":"            if response:"},{"line_number":181,"context_line":"                return json.loads(response)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_fcff3765","line":178,"updated":"2014-05-14 09:10:41.000000000","message":"It might be better to change the import style in a different commit, it\u0027s not related to your \"100% coverage\" change. But it\u0027s up to you :-) It\u0027s just a remark.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":175,"context_line":"        :returns: dictionary of job information"},{"line_number":176,"context_line":"        \u0027\u0027\u0027"},{"line_number":177,"context_line":"        try:"},{"line_number":178,"context_line":"            response \u003d self.jenkins_open(Request("},{"line_number":179,"context_line":"                self.server + JOB_INFO % locals()))"},{"line_number":180,"context_line":"            if response:"},{"line_number":181,"context_line":"                return json.loads(response)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_e9f59e57","line":178,"in_reply_to":"7adec928_fcff3765","updated":"2014-05-15 17:22:49.000000000","message":"I see your point and I kind of wish now that I did this a bit differently. The reason for changing the import style was to make it easier to mock these things in the tests. I have some ideas on how to do this a little differently (basically doing more of a dependency injection thing where urlopen and such are attributes of the Jenkins class) and I started playing with them, but just concluded that it\u0027s going to make too much of a mess of this PR and https://review.openstack.org/90114\n\nAre you ok with merging as-is (after I address other comments of course) and then perhaps changing this later?","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"}],"test-requirements.txt":[{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"20b9381cc12315f8f77dc8b53118f7822b9241db","unresolved":false,"context_lines":[{"line_number":1,"context_line":"discover"},{"line_number":2,"context_line":"flake8"},{"line_number":3,"context_line":"mock"},{"line_number":4,"context_line":"unittest2"}],"source_content_type":"text/plain","patch_set":7,"id":"9ad9bd40_67ec86c8","line":4,"updated":"2014-05-04 09:10:09.000000000","message":"Note that \"unittest2\" does not work on Python 3. There is a port of it called \"unittest2py3k\". This means that for Python 3, I would need 2 test-requirements.txt files - sigh.\n\nAn alternative is to use pytest which has its own assertRaises and then we wouldn\u0027t need unittest2/unittest2py3k - I love pytest, so this is probably my preferred route.\n\nOr monkeypatch in our own assertRaises (a la https://marmida.com/blog/index.php/2012/08/08/monkey-patching-assert_raises/), but that\u0027s kind of yucky.","commit_id":"f5a13b198fa73ae036400593f10798ed85694a6b"}],"tests/test_jenkins.py":[{"author":{"_account_id":11020,"name":"Alex Conrad","email":"alexandre.conrad@gmail.com","username":"aconrad"},"change_message_id":"f7d8617945b7b803535a0afd502b012d5275c651","unresolved":false,"context_lines":[{"line_number":867,"context_line":"                \u0027create[test_node] failed\u0027)"},{"line_number":868,"context_line":""},{"line_number":869,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":870,"context_line":"    def test_enable_node(self, jenkins_mock):"},{"line_number":871,"context_line":"        node_info \u003d {"},{"line_number":872,"context_line":"            \u0027displayName\u0027: \u0027nodes\u0027,"},{"line_number":873,"context_line":"            \u0027totalExecutors\u0027: 5,"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAXX%2F%2FzHo%3D","line":870,"updated":"2014-04-27 01:50:08.000000000","message":"A bunch of these test_disable_node* / test_disable_node* tests don\u0027t have assert statements. Is that normal?","commit_id":"b8034aa31562547f495614e138c0932e22533a99"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"ad24bc0409eeece747afa9293592ac47d8079447","unresolved":false,"context_lines":[{"line_number":867,"context_line":"                \u0027create[test_node] failed\u0027)"},{"line_number":868,"context_line":""},{"line_number":869,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":870,"context_line":"    def test_enable_node(self, jenkins_mock):"},{"line_number":871,"context_line":"        node_info \u003d {"},{"line_number":872,"context_line":"            \u0027displayName\u0027: \u0027nodes\u0027,"},{"line_number":873,"context_line":"            \u0027totalExecutors\u0027: 5,"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAXX%2F%2FyxE%3D","line":870,"in_reply_to":"AAAAXX%2F%2FzHo%3D","updated":"2014-04-27 09:27:51.000000000","message":"Good point! I\u0027m going to add some more asserts to these...","commit_id":"b8034aa31562547f495614e138c0932e22533a99"},{"author":{"_account_id":11020,"name":"Alex Conrad","email":"alexandre.conrad@gmail.com","username":"aconrad"},"change_message_id":"9e66d488ab2efb4e04c8fa7781204ea067bbc69b","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        j \u003d jenkins.Jenkins(\u0027http://example.com/\u0027, \u0027test\u0027, \u0027test\u0027)"},{"line_number":42,"context_line":"        request \u003d jenkins.Request(\u0027http://example.com/job/TestJob\u0027)"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        j.maybe_add_crumb(request)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":47,"context_line":"    def test_maybe_add_crumb__with_data(self, jenkins_mock):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyKw%3D","line":44,"updated":"2014-04-27 19:52:31.000000000","message":"Missing assert statements.","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"2eeee82ec092e6b81be44cd45dff36d78e31c298","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        j \u003d jenkins.Jenkins(\u0027http://example.com/\u0027, \u0027test\u0027, \u0027test\u0027)"},{"line_number":42,"context_line":"        request \u003d jenkins.Request(\u0027http://example.com/job/TestJob\u0027)"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        j.maybe_add_crumb(request)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":47,"context_line":"    def test_maybe_add_crumb__with_data(self, jenkins_mock):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FxBU%3D","line":44,"in_reply_to":"AAAAXX%2F%2FyKw%3D","updated":"2014-04-28 06:00:27.000000000","message":"Done","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"acdeb94e6d6ed404abd3c4e1390900866b6e8c72","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        j \u003d jenkins.Jenkins(\u0027http://example.com/\u0027, \u0027test\u0027, \u0027test\u0027)"},{"line_number":42,"context_line":"        request \u003d jenkins.Request(\u0027http://example.com/job/TestJob\u0027)"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"        j.maybe_add_crumb(request)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":47,"context_line":"    def test_maybe_add_crumb__with_data(self, jenkins_mock):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyCo%3D","line":44,"in_reply_to":"AAAAXX%2F%2FyKw%3D","updated":"2014-04-27 21:21:39.000000000","message":"Good call. I\u0027ll look at this later.","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11020,"name":"Alex Conrad","email":"alexandre.conrad@gmail.com","username":"aconrad"},"change_message_id":"9e66d488ab2efb4e04c8fa7781204ea067bbc69b","unresolved":false,"context_lines":[{"line_number":88,"context_line":"        try:"},{"line_number":89,"context_line":"            j.jenkins_open(request, add_crumb\u003dFalse)"},{"line_number":90,"context_line":"            if 1:  # pragma: no cover"},{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyKs%3D","line":91,"updated":"2014-04-27 19:52:31.000000000","message":"why don\u0027t you call self.fail() directly, what\u0027s with the \"if 1\" statement?","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"acdeb94e6d6ed404abd3c4e1390900866b6e8c72","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            j.jenkins_open(request, add_crumb\u003dFalse)"},{"line_number":90,"context_line":"            if 1:  # pragma: no cover"},{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyB0%3D","line":92,"updated":"2014-04-27 21:21:39.000000000","message":"Ha ha. Yeah, this is stupid. It\u0027s because if I put `#pragma: no cover` on the self.fail line then it\u0027s more than 78 characters. I\u0027m thinking of using unittest.assertRaises as you suggest below (and importing unittest2 for 2.6)","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11020,"name":"Alex Conrad","email":"alexandre.conrad@gmail.com","username":"aconrad"},"change_message_id":"9e66d488ab2efb4e04c8fa7781204ea067bbc69b","unresolved":false,"context_lines":[{"line_number":90,"context_line":"            if 1:  # pragma: no cover"},{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":96,"context_line":"    def test_create_job(self, jenkins_mock):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyHY%3D","line":93,"updated":"2014-04-27 19:52:31.000000000","message":"You could also have self.assertRaises(jenkins.JenkinsException, j.jenkins_open, request, add_crumb\u003dFalse), but then I guess you couldn\u0027t test for the actual string. I\u0027m assuming that this needs to work in Py26, but in py27 assertRaises is also a context manager:\n\nwith self.assertRaises(jenkins.JenkinsException) as exc:\n    j.jenkins_open(request, add_crumb\u003dFalse)\nself.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"acdeb94e6d6ed404abd3c4e1390900866b6e8c72","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":96,"context_line":"    def test_create_job(self, jenkins_mock):"},{"line_number":97,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyBk%3D","line":94,"updated":"2014-04-27 21:21:39.000000000","message":"Yeah. I like this idea. It\u0027s clearer than what I have. I could import unittest2 for Python 2.6. Or use pytest.raises, but OpenStack projects like to use testrepository along with unittest discover. Folks may or may not want to introduce pytest.","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"2eeee82ec092e6b81be44cd45dff36d78e31c298","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":96,"context_line":"    def test_create_job(self, jenkins_mock):"},{"line_number":97,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FxBQ%3D","line":94,"in_reply_to":"AAAAXX%2F%2FyAE%3D","updated":"2014-04-28 06:00:27.000000000","message":"Done","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"e0c0b4fad5fe061a4b2dbab6b4fc72cc08fe418e","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":96,"context_line":"    def test_create_job(self, jenkins_mock):"},{"line_number":97,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2Fxg0%3D","line":94,"in_reply_to":"AAAAXX%2F%2FyAE%3D","updated":"2014-04-28 02:29:38.000000000","message":"In order to add the dependency, we need to modify setup.py or setup.cfg (I guess using pbr), so I\u0027m not really wanting to do this before https://review.openstack.org/90455 gets merged. Even then I\u0027m not sure how with pbr you can specify that you want unittest2 for Python 2 but not for Python 3.","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":11020,"name":"Alex Conrad","email":"alexandre.conrad@gmail.com","username":"aconrad"},"change_message_id":"12d430e689523a32b2d42144b5368a3fb705a209","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                self.fail(\"This should\u0027ve failed with JenkinsException\")"},{"line_number":92,"context_line":"        except jenkins.JenkinsException as exc:"},{"line_number":93,"context_line":"            self.assertEqual(str(exc), \u0027Error in request.Possibly authentication failed [401]\u0027)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    @patch.object(jenkins.Jenkins, \u0027jenkins_open\u0027)"},{"line_number":96,"context_line":"    def test_create_job(self, jenkins_mock):"},{"line_number":97,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAXX%2F%2FyAE%3D","line":94,"in_reply_to":"AAAAXX%2F%2FyBk%3D","updated":"2014-04-27 21:53:09.000000000","message":"Yeah, let\u0027s go with unittest2. Let\u0027s not introduce a py.test requirement just for pytest.raises, unittest2 fits the bill and people are more familiar with its API.","commit_id":"c1646bbcee9a88fa4864dae4a5768616967e33df"},{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":7,"context_line":"    if sys.version_info \u003c (2, 7):"},{"line_number":8,"context_line":"        # On Pythons 2.6 and older, we must have unittest2, because the"},{"line_number":9,"context_line":"        # standard library doesn\u0027t have what we need"},{"line_number":10,"context_line":"        raise  # pragma: no cover"},{"line_number":11,"context_line":"    else:"},{"line_number":12,"context_line":"        # On Pythons 2.7 and newer, the standard library has what we need"},{"line_number":13,"context_line":"        import unittest"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_1c3a2336","line":10,"updated":"2014-05-14 09:10:41.000000000","message":"I don\u0027t understand the purpose of this check. You should not check if requirements are correctly installed in the code. Improve setup.py (ex: use pbr) and ensure that test-requirements.txt is correct (it\u0027s the case, unittest2 is present there).\n\nYou should remove this check to only keep the \"else:\" block below.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":7,"context_line":"    if sys.version_info \u003c (2, 7):"},{"line_number":8,"context_line":"        # On Pythons 2.6 and older, we must have unittest2, because the"},{"line_number":9,"context_line":"        # standard library doesn\u0027t have what we need"},{"line_number":10,"context_line":"        raise  # pragma: no cover"},{"line_number":11,"context_line":"    else:"},{"line_number":12,"context_line":"        # On Pythons 2.7 and newer, the standard library has what we need"},{"line_number":13,"context_line":"        import unittest"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_e96afeec","line":10,"in_reply_to":"7adec928_1c3a2336","updated":"2014-05-15 17:22:49.000000000","message":"You\u0027re right. This isn\u0027t needed if we assume that the user installed everything in test-requirements.txt.\n\nI think I was probably trying to make it work in the case that the user checked out the repo and tried to run it without creating a virtualenv or doing a pip install of anything. If they\u0027re on Python 2.7 maybe this could work since unittest has everything needed.\n\nHowever, maybe this code is too ugly to have just to support an edge case like that?\n\nI could change this to just:\n\n    import unittest2 as unittest","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":25,"context_line":"        self.assertEqual(j.auth, \u0027Basic dGVzdDp0ZXN0\u0027)"},{"line_number":26,"context_line":"        self.assertEqual(j.crumb, None)"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"        j \u003d jenkins.Jenkins(\u0027http://example.com\u0027, \u0027test\u0027, \u0027test\u0027)"},{"line_number":29,"context_line":"        self.assertEqual(j.server, \u0027http://example.com/\u0027)"},{"line_number":30,"context_line":"        self.assertEqual(j.auth, \u0027Basic dGVzdDp0ZXN0\u0027)"},{"line_number":31,"context_line":"        self.assertEqual(j.crumb, None)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_fcb577ee","line":28,"range":{"start_line":28,"start_character":9,"end_line":28,"end_character":10},"updated":"2014-05-14 09:10:41.000000000","message":"Oh, it took me 5 minutes to notice the difference between this test and the previous one. It would be nice to add a comment mentionning that this test checks an URL without trailing slash.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":25,"context_line":"        self.assertEqual(j.auth, \u0027Basic dGVzdDp0ZXN0\u0027)"},{"line_number":26,"context_line":"        self.assertEqual(j.crumb, None)"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"        j \u003d jenkins.Jenkins(\u0027http://example.com\u0027, \u0027test\u0027, \u0027test\u0027)"},{"line_number":29,"context_line":"        self.assertEqual(j.server, \u0027http://example.com/\u0027)"},{"line_number":30,"context_line":"        self.assertEqual(j.auth, \u0027Basic dGVzdDp0ZXN0\u0027)"},{"line_number":31,"context_line":"        self.assertEqual(j.crumb, None)"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_293f86d7","line":28,"in_reply_to":"7adec928_fcb577ee","updated":"2014-05-15 17:22:49.000000000","message":"Sorry. I\u0027ll split this into 3 tests with appropriate names to make it clearer. Thanks!","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":62,"context_line":"        self.assertFalse(\u0027.crumb\u0027 in request.headers)"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":65,"context_line":"    def test_maybe_add_crumb__with_data(self, jenkins_mock):"},{"line_number":66,"context_line":"        crumb_data \u003d {"},{"line_number":67,"context_line":"            \"crumb\": \"dab177f483b3dd93483ef6716d8e792d\","},{"line_number":68,"context_line":"            \"crumbRequestField\": \".crumb\","}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_dc16fba6","line":65,"updated":"2014-05-14 09:10:41.000000000","message":"Oh, it\u0027s the first thime that I see \"__\" (instead of just \"_\") in a function. Is there a reason to use \"__\"?\n\nIf you want to group similar tests but with small variations, it\u0027s maybe better to use a dedicated \"MaybeAddCrumTest\" TestCase class to not have to repeat the \"maybe_add_crumb\" prefix.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":62,"context_line":"        self.assertFalse(\u0027.crumb\u0027 in request.headers)"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":65,"context_line":"    def test_maybe_add_crumb__with_data(self, jenkins_mock):"},{"line_number":66,"context_line":"        crumb_data \u003d {"},{"line_number":67,"context_line":"            \"crumb\": \"dab177f483b3dd93483ef6716d8e792d\","},{"line_number":68,"context_line":"            \"crumbRequestField\": \".crumb\","}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_0911ea5c","line":65,"in_reply_to":"7adec928_dc16fba6","updated":"2014-05-15 17:22:49.000000000","message":"Using the double underscore is a habit of mine. The idea is that the method name has underscores in it so the double underscore makes it clearer that the parts afterwards is not part of the method name. Maybe that\u0027s silly though, as I\u0027m probably the only one who knows to interpret it that way. :-)\n\nI thought of the  \"MaybeAddCrumTest\" TestCase class as well, but I didn\u0027t do it because there are only two tests and they\u0027re not super similar because one has crumb_data and one doesn\u0027t.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":9107,"name":"Victor Stinner","email":"vstinner@redhat.com","username":"haypo"},"change_message_id":"1f52ac92311632d7966e1b6bbe94e54db62ec7b2","unresolved":false,"context_lines":[{"line_number":77,"context_line":"            jenkins_mock.call_args[0][0].get_full_url(),"},{"line_number":78,"context_line":"            \u0027http://example.com/crumbIssuer/api/json\u0027)"},{"line_number":79,"context_line":"        self.assertEqual(j.crumb, crumb_data)"},{"line_number":80,"context_line":"        self.assertEqual(request.headers[\u0027.crumb\u0027], crumb_data[\u0027crumb\u0027])"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":83,"context_line":"    def test_jenkins_open(self, jenkins_mock):"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_bc7a0fef","line":80,"updated":"2014-05-14 09:10:41.000000000","message":"It looks like most of the code of this function is duplicated with test_maybe_add_crumb(). You may try to share more code. Maybe write a common subfunction and just keep differences in tests.\n\nSame remarks for other functions in this file.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"},{"author":{"_account_id":11011,"name":"Marc Abramowitz","email":"marc@marc-abramowitz.com","username":"msabramo"},"change_message_id":"9bbee053c80d707fb45bd7add3b403356575cf75","unresolved":false,"context_lines":[{"line_number":77,"context_line":"            jenkins_mock.call_args[0][0].get_full_url(),"},{"line_number":78,"context_line":"            \u0027http://example.com/crumbIssuer/api/json\u0027)"},{"line_number":79,"context_line":"        self.assertEqual(j.crumb, crumb_data)"},{"line_number":80,"context_line":"        self.assertEqual(request.headers[\u0027.crumb\u0027], crumb_data[\u0027crumb\u0027])"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"    @patch(\u0027jenkins.urlopen\u0027)"},{"line_number":83,"context_line":"    def test_jenkins_open(self, jenkins_mock):"}],"source_content_type":"text/x-python","patch_set":8,"id":"7adec928_c4844760","line":80,"in_reply_to":"7adec928_bc7a0fef","updated":"2014-05-15 17:22:49.000000000","message":"Yeah, this is pretty repetitive and maybe could be refactored later. I don\u0027t think I want to do it now though since the Python 3 PR is based on this one. Maybe later.\n\nI do tend to do less refactoring and code sharing in tests, because I like them to be very linear and easy to read and not too clever, but this is pretty darn repetitive, so it would be nice to clean it up a bit. I\u0027m not excited about doing it now though.","commit_id":"7ad5045af5ca5d577a3f4bbb133a75519213e5a8"}]}