[Culprit-Finder] Refactor GitilesRepostory to make http_client required argument.

[Culprit-Finder] Refactor GitilesRepostory to make http_client required argument.

BUG=659449
Committed: https://chromium.googlesource.com/infra/infra/+/cae4d6478d25b9be3607bb20989713569eae9c49

[Sharu Jiang, 2016-11-22 03:57:35]

Description was changed from

==========
[Predator] Refactor GitilesRepostory to make http_client required argument.

BUG=659449
==========

to

==========
[Culprit-Finder] Refactor GitilesRepostory to make http_client required
argument.

BUG=659449
==========

[Sharu Jiang, 2016-11-22 03:57:35]

katesonia@chromium.org changed reviewers:
+ chanli@chromium.org, lijeffrey@chromium.org, stgao@chromium.org,
wrengr@chromium.org

[Sharu Jiang, 2016-11-22 03:58:28]

PTAL :)

[wrengr, 2016-11-22 19:11:53]

lgtm, with comments/question

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
File appengine/findit/crash/test/changelist_classifier_test.py (right):

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
appengine/findit/crash/test/changelist_classifier_test.py:315: 7,
GitilesRepository(self.GetMockHttpClient()))
Since we do this line a lot in a bunch of tests, maybe we should just have a
self.GetChangelistClassifier() method

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
File appengine/findit/crash/test/stacktrace_test_suite.py (right):

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
appengine/findit/crash/test/stacktrace_test_suite.py:10: class
StacktraceTestSuite(CrashTestCase):  #pragma: no cover.
Is this change related to the rest of the CL?

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/lib/gitile...
File appengine/findit/lib/gitiles/gitiles_repository.py (right):

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/lib/gitile...
appengine/findit/lib/gitiles/gitiles_repository.py:36: # TODO(crbug.com/659449):
Refactor the http_client to be required argument.
Can get rid of this todo

[Sharu Jiang, 2016-11-22 20:15:33]

Patchset #2 (id:20001) has been deleted

[Sharu Jiang, 2016-11-22 20:15:38]

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
File appengine/findit/crash/test/changelist_classifier_test.py (right):

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
appengine/findit/crash/test/changelist_classifier_test.py:315: 7,
GitilesRepository(self.GetMockHttpClient()))
On 2016/11/22 19:11:53, wrengr wrote:
> Since we do this line a lot in a bunch of tests, maybe we should just have a
> self.GetChangelistClassifier() method

Done.

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
File appengine/findit/crash/test/stacktrace_test_suite.py (right):

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/crash/test...
appengine/findit/crash/test/stacktrace_test_suite.py:10: class
StacktraceTestSuite(CrashTestCase):  #pragma: no cover.
On 2016/11/22 19:11:53, wrengr wrote:
> Is this change related to the rest of the CL?

Yes, since the GetMockHttpClient is in TestCase in the dependency cl
https://codereview.chromium.org/2521773004#ps20001

But it's better that this change goes to the dependency cl.

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/lib/gitile...
File appengine/findit/lib/gitiles/gitiles_repository.py (right):

https://codereview.chromium.org/2524633002/diff/1/appengine/findit/lib/gitile...
appengine/findit/lib/gitiles/gitiles_repository.py:36: # TODO(crbug.com/659449):
Refactor the http_client to be required argument.
On 2016/11/22 19:11:53, wrengr wrote:
> Can get rid of this todo

Done.

[chanli, 2016-11-23 06:06:07]

https://codereview.chromium.org/2524633002/diff/40001/appengine/findit/lib/gi...
File appengine/findit/lib/gitiles/gitiles_repository.py (right):

https://codereview.chromium.org/2524633002/diff/40001/appengine/findit/lib/gi...
appengine/findit/lib/gitiles/gitiles_repository.py:36: def __init__(self,
http_client, repo_url=None):
Any reason you changed the sequence of the arguments?

[Sharu Jiang, 2016-11-23 19:24:39]

https://codereview.chromium.org/2524633002/diff/40001/appengine/findit/lib/gi...
File appengine/findit/lib/gitiles/gitiles_repository.py (right):

https://codereview.chromium.org/2524633002/diff/40001/appengine/findit/lib/gi...
appengine/findit/lib/gitiles/gitiles_repository.py:36: def __init__(self,
http_client, repo_url=None):
On 2016/11/23 06:06:06, chanli wrote:
> Any reason you changed the sequence of the arguments?

Because the http_client is required arguments now, it should be before the
optional argument.

[stgao, 2016-11-29 18:30:00]

lgtm

[Sharu Jiang, 2016-11-29 19:17:23]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-11-29 19:17:24]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2524633002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-29 19:17:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 19:32:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480447043856730,
"parent_rev": "4684f8bf2a8d367f182cef6612ffc1a7da087db9", "commit_rev":
"cae4d6478d25b9be3607bb20989713569eae9c49"}

[commit-bot: I haz the power, 2016-11-29 19:32:32]

Description was changed from

==========
[Culprit-Finder] Refactor GitilesRepostory to make http_client required
argument.

BUG=659449
==========

to

==========
[Culprit-Finder] Refactor GitilesRepostory to make http_client required
argument.

BUG=659449

Committed:
https://chromium.googlesource.com/infra/infra/+/cae4d6478d25b9be3607bb2098971...
==========

[commit-bot: I haz the power, 2016-11-29 19:32:34]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/cae4d6478d25b9be3607bb2098971...