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

appengine/findit/crash/changelist_classifier.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.
 
 import logging
 from collections import defaultdict
 from collections import namedtuple
 
-from common import chrome_dependency_fetcher
+from common.chrome_dependency_fetcher import ChromeDependencyFetcher
 from crash import crash_util
 from crash.suspect import StackInfo
 from crash.suspect import Suspect
 from crash.suspect import SuspectMap
 from crash.scorers.aggregated_scorer import AggregatedScorer
 from crash.scorers.min_distance import MinDistance
 from crash.scorers.top_frame_index import TopFrameIndex
 from crash.stacktrace import CallStack
 from crash.stacktrace import Stacktrace
 from libs.gitiles.diff import ChangeType
 
 
 class ChangelistClassifier(namedtuple('ChangelistClassifier',
-    ['repository', 'top_n_results', 'confidence_threshold'])):
+    ['repository', 'get_repository', 'top_n_results', 'confidence_threshold'])):
   __slots__ = ()
 
-  def __new__(cls, repository, top_n_results=3, confidence_threshold=0.999):
+  def __new__(cls, repository, get_repository, top_n_results=3,
+              confidence_threshold=0.999):
     """Args:
-      repository (Repository): the Git repository for getting CLs to classify.
+      repository (Repository): the Git repository of the main project
+        we're trying to classify CLs from.
+      get_repository (callable): a function from DEP urls to ``Repository``
+        objects, so we can get changelogs and blame for each dep. Notably,
+        to keep the code here generic, we make no assumptions about
+        which subclass of ``Repository`` this function returns. Thus,
+        it is up to the caller to decide what class to return and handle
+        any other arguments that class may require (e.g., an http client
+        for ``GitilesRepository``).
       top_n_results (int): maximum number of results to return.
       confidence_threshold (float): In [0,1], above which we only return
         the first suspect.
     """
     return super(cls, ChangelistClassifier).__new__(
-        cls, repository, top_n_results, confidence_threshold)
+        cls, repository, get_repository, top_n_results, confidence_threshold)
 
   def __str__(self): # pragma: no cover
     return ('%s(top_n_results=%d, confidence_threshold=%g)'
         % (self.__class__.__name__,
            self.top_n_results,
            self.confidence_threshold))
 
   def __call__(self, report):
     """Finds changelists suspected of being responsible for the crash report.
 
     This function assumes the report's stacktrace has already had any necessary
     preprocessing (like filtering or truncating) applied.
 
     Args:
       report (CrashReport): the report to be analyzed.
 
     Returns:
       List of ``Suspect``s, sorted by confidence from highest to lowest.
     """
     if not report.regression_range:
       logging.warning('ChangelistClassifier.__call__: Missing regression range '
           'for report: %s', str(report))
       return []
     last_good_version, first_bad_version = report.regression_range
     logging.info('ChangelistClassifier.__call__: Regression range %s:%s',
         last_good_version, first_bad_version)
 
+    dependency_fetcher = ChromeDependencyFetcher(self.repository)
+
     # We are only interested in the deps in crash stack (the callstack that
     # caused the crash).
     # TODO(wrengr): we may want to receive the crash deps as an argument,
     # so that when this method is called via Findit.FindCulprit, we avoid
     # doing redundant work creating it.
     stack_deps = GetDepsInCrashStack(
         report.stacktrace.crash_stack,
-        chrome_dependency_fetcher.ChromeDependencyFetcher(
-            self.repository).GetDependency(report.crashed_version,
-                                           report.platform))
+        dependency_fetcher.GetDependency(
+            report.crashed_version, report.platform))
 
     # Get dep and file to changelogs, stack_info and blame dicts.
-    dep_rolls = chrome_dependency_fetcher.ChromeDependencyFetcher(
-        self.repository).GetDependencyRollsDict(
-            last_good_version, first_bad_version, report.platform)
+    dep_rolls = dependency_fetcher.GetDependencyRollsDict(
+        last_good_version, first_bad_version, report.platform)
 
     # Regression of a dep added/deleted (old_revision/new_revision is None) can
     # not be known for sure and this case rarely happens, so just filter them
     # out.
     regression_deps_rolls = {}
     for dep_path, dep_roll in dep_rolls.iteritems():
       if not dep_roll.old_revision or not dep_roll.new_revision:
         logging.info('Skip %s denpendency %s',
                      'added' if dep_roll.new_revision else 'deleted', dep_path)
         continue
       regression_deps_rolls[dep_path] = dep_roll
 
     dep_to_file_to_changelogs, ignore_cls = GetChangeLogsForFilesGroupedByDeps(
         regression_deps_rolls, stack_deps, self.repository)
     dep_to_file_to_stack_infos = GetStackInfosForFilesGroupedByDeps(
         report.stacktrace, stack_deps)
 
-    # TODO: argument order is inconsistent from others. Repository should
-    # be last argument.
     suspects = FindSuspects(dep_to_file_to_changelogs,
                             dep_to_file_to_stack_infos,
-                            stack_deps, self.repository, ignore_cls)
+                            stack_deps, self.get_repository, ignore_cls)
     if not suspects:
       return []
 
     # Set confidence, reasons, and changed_files.
     aggregated_scorer = AggregatedScorer([TopFrameIndex(), MinDistance()])
     map(aggregated_scorer.Score, suspects)
 
     # Filter all the 0 confidence results.
     suspects = filter(lambda suspect: suspect.confidence != 0, suspects)
     if not suspects:
@@ skipping 138 matching lines @@
       # We only care about those dependencies in crash stack.
       if frame.dep_path not in stack_deps:
         continue
 
       dep_to_file_to_stack_infos[frame.dep_path][frame.file_path].append(
           StackInfo(frame, callstack.priority))
 
   return dep_to_file_to_stack_infos
 
 
 # TODO(katesonia): Remove the repository argument after refatoring cl committed.

[Martin Barbella, 2016/12/29 21:51:05]

Is this now done, or was additional refactoring planned?

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

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

 def FindSuspects(dep_to_file_to_changelogs,
                  dep_to_file_to_stack_infos,
-                 stack_deps, repository,
+                 stack_deps, get_repository,
                  ignore_cls=None):
   """Finds suspects by matching stacktrace and changelogs in regression range.
 
   This method only applies to those crashes with regression range.
 
   Args:
     dep_to_file_to_changelogs (dict): Maps dep_path to a dict mapping file path
       to ChangeLogs that touched this file.
     dep_to_file_to_stack_infos (dict): Maps dep path to a dict mapping file path
       to a list of stack information of this file. A file may occur in several
       frames, one stack info consist of a StackFrame and the callstack priority
       of it.
     stack_deps (dict): Represents all the dependencies shown in the crash stack.
-    repository (Repository): Repository to get changelogs and blame from.
+    get_repository (callable): a function from urls to ``Repository``
+      objects, so we can get changelogs and blame for each dep.
     ignore_cls (set): Set of reverted revisions.
 
   Returns:
     A list of ``Suspect`` instances with confidence and reason unset.
   """
   suspects = SuspectMap(ignore_cls)
 
   for dep, file_to_stack_infos in dep_to_file_to_stack_infos.iteritems():
     file_to_changelogs = dep_to_file_to_changelogs[dep]
 
     for crashed_file_path, stack_infos in file_to_stack_infos.iteritems():
       for touched_file_path, changelogs in file_to_changelogs.iteritems():
         if not crash_util.IsSameFilePath(crashed_file_path, touched_file_path):
           continue
 
-        repository.repo_url = stack_deps[dep].repo_url
+        repository = get_repository(stack_deps[dep].repo_url)
         blame = repository.GetBlame(touched_file_path,
                                     stack_deps[dep].revision)
 
         # Generate/update each suspect(changelog) in changelogs, blame is used
         # to calculate distance between touched lines and crashed lines in file.
         suspects.GenerateSuspects(
             touched_file_path, dep, stack_infos, changelogs, blame)
 
   return suspects.values()