Changed FindSuspects to take a Repository factory, rather than mutating it

Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code, FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` interface. This
CL solves the second of those: it moves the mutation out of FindSuspects,
instead taking a factory-function argument for mapping the dep urls to
their Repository objects.

To keep the size of this CL manageable, we don't address the first
issue. That is, the actual functions we pass for the ``get_repository``
argument will themselves perform the mutation. This way, the code is
still functionally identical to the old code. Of course, a future CL
should still address this issue, as per https://crbug.com/677224

BUG=
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2608483002
Committed: https://chromium.googlesource.com/infra/infra/+/a07e0327ff09d5cc120fc9597db5fad399efedba

[wrengr, 2016-12-27 23:20:34]

Description was changed from

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` API. This CL
removes the mutation from FindSuspects by instead passing a factory
function in which maps dep urls to Repository objects. To keep this CL
brief and to minimize functional differences, the factory functions we
pass do the same mutation themselves; however, their effects are better
documented, and the mutation itself can be cleaned up in future CLs.

BUG=
TBR=stgao@chromium.org
==========

to

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` API. This CL
removes the mutation from FindSuspects by instead passing a factory
function in which maps dep urls to Repository objects. To keep this CL
brief and to minimize functional differences, the factory functions we
pass do the same mutation themselves; however, their effects are better
documented, and the mutation itself can be cleaned up in future CLs.

BUG=
TBR=stgao@chromium.org
==========

[wrengr, 2016-12-27 23:20:34]

wrengr@chromium.org changed reviewers:
+ inferno@chromium.org, katesonia@chromium.org, mbarbella@chromium.org

[wrengr, 2016-12-27 23:20:51]

non-trivial reorganization, PTAL

[Sharu Jiang, 2016-12-28 00:45:34]

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:23: ['repository',
'get_repository', 'top_n_results', 'confidence_threshold'])):
So after we cleaned the mutation in ``get_repository``(make it allocates new
repository object instead of doing mutation), we will get rid of this
``repository`` parameter, right?

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:23: ['repository',
'get_repository', 'top_n_results', 'confidence_threshold'])):
The ``get_repository`` here is not actually a factory, it is to mutate
repository, so this name is a little misleading. Better add comments to explain
that it will be cleaned in the future.

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:71: dependency_fetcher =
ChromeDependencyFetcher(self.repository)
I think ``ChromeDependencyFetcher`` should also take a repository factory as
construct parameter instead of repository instance, because it's ``Chrome``
dependency fetcher, which means the dep url has to be chromium dep url, so
repository should be created inside ``ChromeDependencyFetcher`` with dep url set
to chromium url.

[wrengr, 2016-12-28 01:00:56]

Description was changed from

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` API. This CL
removes the mutation from FindSuspects by instead passing a factory
function in which maps dep urls to Repository objects. To keep this CL
brief and to minimize functional differences, the factory functions we
pass do the same mutation themselves; however, their effects are better
documented, and the mutation itself can be cleaned up in future CLs.

BUG=
TBR=stgao@chromium.org
==========

to

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` interface. This CL
solves the second of those: it moves the mutation out of FindSuspects, instead
taking a factory-function argument for mapping the dep urls to their Repository
objects.

To keep the size of this CL manageable, we don't address the first issue. That
is, the actual functions we pass for the ``get_repository`` argument will
themselves perform the mutation. This way, the code is still functionally
identical to the old code. Of course, a future CL should still address this
issue, as per https://crbug.com/677224

BUG=
TBR=stgao@chromium.org
==========

[wrengr, 2016-12-28 01:02:19]

Description was changed from

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` interface. This CL
solves the second of those: it moves the mutation out of FindSuspects, instead
taking a factory-function argument for mapping the dep urls to their Repository
objects.

To keep the size of this CL manageable, we don't address the first issue. That
is, the actual functions we pass for the ``get_repository`` argument will
themselves perform the mutation. This way, the code is still functionally
identical to the old code. Of course, a future CL should still address this
issue, as per https://crbug.com/677224

BUG=
TBR=stgao@chromium.org
==========

to

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code, FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` interface. This
CL solves the second of those: it moves the mutation out of FindSuspects,
instead taking a factory-function argument for mapping the dep urls to
their Repository objects.

To keep the size of this CL manageable, we don't address the first
issue. That is, the actual functions we pass for the ``get_repository``
argument will themselves perform the mutation. This way, the code is
still functionally identical to the old code. Of course, a future CL
should still address this issue, as per https://crbug.com/677224

BUG=
TBR=stgao@chromium.org
==========

[corbin8951, 2016-12-28 04:05:19]

corbin8951@gmail.com changed reviewers:
+ corbin8951@gmail.com

[corbin8951, 2016-12-28 04:05:20]

lgtm

[wrengr, 2016-12-28 18:23:33]

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:23: ['repository',
'get_repository', 'top_n_results', 'confidence_threshold'])):
On 2016/12/28 00:45:33, Sharu Jiang wrote:
> So after we cleaned the mutation in ``get_repository``(make it allocates new
> repository object instead of doing mutation), we will get rid of this
> ``repository`` parameter, right?

In principle we could... but since the main ``repository`` is used frequently
and longlived-ly, we should probably keep it. This way the caller of
ChangelistClassifier.__init__ can build the repo object once, pass it to the
constructor, and then forget about it, the url associated with it, etc (if
needed, the caller can always retrieve that information from the
ChangelistClassifier object itself).

The simplest alternative to keeping it around would be to pass the url to the
constructor, and understand that the constructor will call ``get_repository`` on
it and save the result. That seems less clean than passing the repo object, imo

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:23: ['repository',
'get_repository', 'top_n_results', 'confidence_threshold'])):
On 2016/12/28 00:45:33, Sharu Jiang wrote:
> The ``get_repository`` here is not actually a factory, it is to mutate
> repository, so this name is a little misleading. Better add comments to
explain
> that it will be cleaned in the future. 

The ``get_repository`` argument is indeed (intended to be) a factory.
``FindSuspects`` doesn't rely on/require it to return novel objects (e.g., it
doesn't save them anywhere or anything requiring longevity beyond from when
``get_repository`` is called to when ``FindSuspects`` returns). Which is why we
can get away with passing a fake "factory" that doesn't actually build anything.
Yes, passing a fake factory is bad; but that's firmly the fault of the caller
not the callee.

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:71: dependency_fetcher =
ChromeDependencyFetcher(self.repository)
On 2016/12/28 00:45:33, Sharu Jiang wrote:
> I think ``ChromeDependencyFetcher`` should also take a repository factory as
> construct parameter instead of repository instance, because it's ``Chrome``
> dependency fetcher, which means the dep url has to be chromium dep url, so
> repository should be created inside ``ChromeDependencyFetcher`` with dep url
set
> to chromium url.

I'm not sure I follow. Perhaps you could write up a CL demonstrating the change
to ChromeDependencyFetcher you have in mind

[wrengr, 2016-12-28 19:09:24]

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:71: dependency_fetcher =
ChromeDependencyFetcher(self.repository)
On 2016/12/28 18:23:33, wrengr wrote:
> On 2016/12/28 00:45:33, Sharu Jiang wrote:
> > I think ``ChromeDependencyFetcher`` should also take a repository factory as
> > construct parameter instead of repository instance, because it's ``Chrome``
> > dependency fetcher, which means the dep url has to be chromium dep url, so
> > repository should be created inside ``ChromeDependencyFetcher`` with dep url
> set
> > to chromium url.
> 
> I'm not sure I follow. Perhaps you could write up a CL demonstrating the
change
> to ChromeDependencyFetcher you have in mind

ah, nm. I think I see what you mean now. I'm working on cleaning u the
ChromeDependencyFetcher stuff too in the CL that gets rid of the mutation

[wrengr, 2016-12-28 20:55:52]

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2608483002/diff/1/appengine/findit/crash/chan...
appengine/findit/crash/changelist_classifier.py:71: dependency_fetcher =
ChromeDependencyFetcher(self.repository)
On 2016/12/28 19:09:24, wrengr wrote:
> On 2016/12/28 18:23:33, wrengr wrote:
> > On 2016/12/28 00:45:33, Sharu Jiang wrote:
> > > I think ``ChromeDependencyFetcher`` should also take a repository factory
as
> > > construct parameter instead of repository instance, because it's
``Chrome``
> > > dependency fetcher, which means the dep url has to be chromium dep url, so
> > > repository should be created inside ``ChromeDependencyFetcher`` with dep
url
> > set
> > > to chromium url.
> > 
> > I'm not sure I follow. Perhaps you could write up a CL demonstrating the
> change
> > to ChromeDependencyFetcher you have in mind
> 
> ah, nm. I think I see what you mean now. I'm working on cleaning u the
> ChromeDependencyFetcher stuff too in the CL that gets rid of the mutation

See https://codereview.chromium.org/2605943002 for cleaning all this up

[Martin Barbella, 2016-12-29 21:51:06]

LGTM provided it lands with 2605943002

https://codereview.chromium.org/2608483002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2608483002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/changelist_classifier.py:264: # TODO(katesonia): Remove
the repository argument after refatoring cl committed.
Is this now done, or was additional refactoring planned?

[Sharu Jiang, 2016-12-29 21:58:09]

On 2016/12/29 21:51:06, Martin Barbella wrote:
> LGTM provided it lands with 2605943002
> 
>
https://codereview.chromium.org/2608483002/diff/20001/appengine/findit/crash/...
> File appengine/findit/crash/changelist_classifier.py (right):
> 
>
https://codereview.chromium.org/2608483002/diff/20001/appengine/findit/crash/...
> appengine/findit/crash/changelist_classifier.py:264: # TODO(katesonia): Remove
> the repository argument after refatoring cl committed.
> Is this now done, or was additional refactoring planned?

+1

lgtm if it lands with 2605943002

[Sharu Jiang, 2016-12-29 21:58:20]

https://codereview.chromium.org/2608483002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/changelist_classifier.py (right):

https://codereview.chromium.org/2608483002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/changelist_classifier.py:264: # TODO(katesonia): Remove
the repository argument after refatoring cl committed.
On 2016/12/29 21:51:05, Martin Barbella wrote:
> Is this now done, or was additional refactoring planned?

No, this TODO is about moving this function to be a method of
ChangelistClassifier.

[wrengr, 2017-01-03 19:01:42]

The CQ bit was checked by wrengr@chromium.org

[commit-bot: I haz the power, 2017-01-03 19:01:48]

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, 2017-01-03 19:10:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 19:10:39]

Failed to apply patch for
appengine/findit/crash/test/changelist_classifier_test.py:
While running git apply --index -p1;
  error: patch failed:
appengine/findit/crash/test/changelist_classifier_test.py:118
  error: appengine/findit/crash/test/changelist_classifier_test.py: patch does
not apply

Patch:       appengine/findit/crash/test/changelist_classifier_test.py
Index: appengine/findit/crash/test/changelist_classifier_test.py
diff --git a/appengine/findit/crash/test/changelist_classifier_test.py
b/appengine/findit/crash/test/changelist_classifier_test.py
index
c195088b7da34541896fdf073b9c3d2190e416af..e1ca7374e9f7325699f9dd2388925bd063372a0a
100644
--- a/appengine/findit/crash/test/changelist_classifier_test.py
+++ b/appengine/findit/crash/test/changelist_classifier_test.py
@@ -118,12 +118,28 @@ DUMMY_REPORT = CrashReport(None, None, None,
Stacktrace(DUMMY_CALLSTACKS,
                                                         DUMMY_CALLSTACKS[0]),
                            (None, None))
 
+
 class ChangelistClassifierTest(CrashTestSuite):
 
   def setUp(self):
     super(ChangelistClassifierTest, self).setUp()
+
+    repository = GitilesRepository(self.GetMockHttpClient())
+
+    # TODO(crbug.com/677224): should replace this with an actual factory.
+    def MutateTheRepo(dep_url): # pragma: no cover
+      """A factory function for returning ``Repository`` objects.
+
+      The current definition captures the functionality of before
+      we factored out this factory method. That is, it's not really a
+      "factory" but rather mutates the main repo object in place. In
+      the future this should be changed to do the right thing instead.
+      """
+      repository.repo_url = dep_url
+      return repository
+
     self.changelist_classifier = changelist_classifier.ChangelistClassifier(
-        GitilesRepository(self.GetMockHttpClient()), 7)
+        repository, MutateTheRepo, 7)
 
   def testSkipAddedAndDeletedRegressionRolls(self):
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
@@ -312,7 +328,7 @@ class ChangelistClassifierTest(CrashTestSuite):
 
     suspects = changelist_classifier.FindSuspects(
         dep_file_to_changelogs, dep_file_to_stack_infos, stack_deps,
-        GitilesRepository(self.GetMockHttpClient()))
+        lambda repo_url: GitilesRepository(self.GetMockHttpClient(), repo_url))
     self.assertListEqual([suspect.ToDict() for suspect in suspects],
                          expected_suspects)

[wrengr, 2017-01-03 19:52:49]

The CQ bit was checked by wrengr@chromium.org

[wrengr, 2017-01-03 19:52:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
katesonia@chromium.org, mbarbella@chromium.org, corbin8951@gmail.com
Link to the patchset: https://codereview.chromium.org/2608483002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-03 19:53:00]

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, 2017-01-03 20:02:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483473169094670,
"parent_rev": "900548fa35745ce1181ea6e48addd0a2bbfef2cb", "commit_rev":
"a07e0327ff09d5cc120fc9597db5fad399efedba"}

[commit-bot: I haz the power, 2017-01-03 20:02:36]

Description was changed from

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code, FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` interface. This
CL solves the second of those: it moves the mutation out of FindSuspects,
instead taking a factory-function argument for mapping the dep urls to
their Repository objects.

To keep the size of this CL manageable, we don't address the first
issue. That is, the actual functions we pass for the ``get_repository``
argument will themselves perform the mutation. This way, the code is
still functionally identical to the old code. Of course, a future CL
should still address this issue, as per https://crbug.com/677224

BUG=
TBR=stgao@chromium.org
==========

to

==========
Changed FindSuspects to take a Repository factory, rather than mutating it

In the old code, FindSuspects mutated the Repository object in place. This
is bad for two reasons: (1) it has unforseen and undocumented side
effects, and (2) it relies on the existence of a settable ``repo_url``
property, which is not guaranteed by the ``Repository`` interface. This
CL solves the second of those: it moves the mutation out of FindSuspects,
instead taking a factory-function argument for mapping the dep urls to
their Repository objects.

To keep the size of this CL manageable, we don't address the first
issue. That is, the actual functions we pass for the ``get_repository``
argument will themselves perform the mutation. This way, the code is
still functionally identical to the old code. Of course, a future CL
should still address this issue, as per https://crbug.com/677224

BUG=
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2608483002
Committed:
https://chromium.googlesource.com/infra/infra/+/a07e0327ff09d5cc120fc9597db5f...
==========

[commit-bot: I haz the power, 2017-01-03 20:02:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/a07e0327ff09d5cc120fc9597db5f...