Removing the mutation in the factories for getting dep repositories

appengine/findit/crash/test/changelist_classifier_test.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 from collections import defaultdict
 import copy
 
 from common.dependency import Dependency
 from common.dependency import DependencyRoll
 from common import chrome_dependency_fetcher
@@ skipping 103 matching lines @@
                               CallStackFormatType.DEFAULT, LanguageType.CPP),
                     CallStack(1, [],
                               CallStackFormatType.DEFAULT, LanguageType.CPP)]
 DUMMY_REPORT = CrashReport(None, None, None, Stacktrace(DUMMY_CALLSTACKS,
                                                         DUMMY_CALLSTACKS[0]),
                            (None, None))
 
 
 class ChangelistClassifierTest(CrashTestSuite):
 
+  @property
+  def _mock_http_gitiles_repo_factory(self):
+    # N.B., we must return a lambda rather than taking ``repo_url``
+    # as an additional argument to the method/property, to ensure that
+    # things have the right arity when this is eventually called. That
+    # is, we want a function of ``repo_url`` which closes over ``self``,
+    # rather than a function taking two arguments at once.
+    return lambda repo_url: GitilesRepository(
+        self.GetMockHttpClient(), repo_url)
+
   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(
-        repository, MutateTheRepo, 7)
+        self._mock_http_gitiles_repo_factory, 7)
 
   def testSkipAddedAndDeletedRegressionRolls(self):
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
               'GetDependency', lambda *_: {})
     dep_rolls = {
         'src/dep': DependencyRoll('src/dep1', 'https://url_dep1', None, '9'),
         'src/': DependencyRoll('src/', ('https://chromium.googlesource.com/'
                                         'chromium/src.git'), '4', '5')
     }
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
@@ skipping 55 matching lines @@
     def _MockGetChangeLogs(_, start_rev, end_rev):
       if start_rev == '4' and end_rev == '5':
         return [DUMMY_CHANGELOG1, DUMMY_CHANGELOG2, DUMMY_CHANGELOG3]
 
       return []
 
     self.mock(GitilesRepository, 'GetChangeLogs', _MockGetChangeLogs)
 
     dep_file_to_changelogs, ignore_cls = (
         changelist_classifier.GetChangeLogsForFilesGroupedByDeps(
-            regression_deps_rolls, stack_deps,
-            GitilesRepository(self.GetMockHttpClient())))
+            regression_deps_rolls,
+            stack_deps,
+            self._mock_http_gitiles_repo_factory))
     dep_file_to_changelogs_json = defaultdict(lambda: defaultdict(list))
     for dep, file_to_changelogs in dep_file_to_changelogs.iteritems():
       for file_path, changelogs in file_to_changelogs.iteritems():
         for changelog in changelogs:
           dep_file_to_changelogs_json[dep][file_path].append(changelog.ToDict())
 
     expected_dep_file_to_changelogs_json = {
         'src/': {
             'a.cc': [DUMMY_CHANGELOG1.ToDict()],
             'f.cc': [DUMMY_CHANGELOG3.ToDict()]
@@ skipping 91 matching lines @@
         'project_path': 'src/',
         'author': 'r@chromium.org',
         'time': 'Thu Mar 31 21:24:43 2016',
         'reasons': None,
         'confidence': None,
         'changed_files': None
     }]
 
     suspects = changelist_classifier.FindSuspects(
         dep_file_to_changelogs, dep_file_to_stack_infos, stack_deps,
-        lambda repo_url: GitilesRepository(self.GetMockHttpClient(), repo_url))
+        self._mock_http_gitiles_repo_factory)
     self.assertListEqual([suspect.ToDict() for suspect in suspects],
                          expected_suspects)
 
   # TODO(http://crbug.com/659346): why do these mocks give coverage
   # failures? That's almost surely hiding a bug in the tests themselves.
   def testFindItForCrashNoRegressionRange(self): # pragma: no cover
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
         'GetDependencyRollsDict', lambda *_: {})
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
         'GetDependency', lambda *_: {})
@@ skipping 156 matching lines @@
       return [suspect1, suspect2]
 
     self.mock(changelist_classifier, 'FindSuspects', _MockFindSuspects)
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
         'GetDependencyRollsDict',
         lambda *_: {'src/': DependencyRoll('src/', 'https://repo', '1', '2')})
     self.mock(chrome_dependency_fetcher.ChromeDependencyFetcher,
         'GetDependency', lambda *_: {})
 
     self.assertListEqual(self.changelist_classifier(DUMMY_REPORT), [])