[Predator] Add ``Project`` class and ``ClassifySuspect`` method to project and component classifier

appengine/findit/crash/component_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.
 
 from collections import namedtuple
 from collections import defaultdict
+import functools
 import logging
 import re
 
 from crash.component import Component
 from crash.occurrence import RankByOccurrence
 from libs.gitiles.diff import ChangeType
 
 
 class ComponentClassifier(object):
   """Determines the component of a crash.
 
   For example: ['Blink>DOM', 'Blink>HTML'].
   """
 
-  def __init__(self, components, top_n):
+  def __init__(self, components, top_n_frames):
     """Build a classifier for components.
 
     Args:
       components (list of crash.component.Component): the components to
         check for.
-      top_n (int): how many frames of the callstack to look at"""
+      top_n_frames (int): how many frames of the callstack to look at
+    """
     super(ComponentClassifier, self).__init__()
-    if not components:
-      logging.warning('Empty configuration for component classifier.')
-      components = [] # Ensure self.components is not None
-    self.components = components
-    self.top_n = top_n
-
-  def GetClassFromStackFrame(self, frame):
-    """Determine which component is responsible for this frame."""
-    for component in self.components:
-      if component.MatchesStackFrame(frame):
-        return component.component_name
-
-    return ''
-
-  # TODO(wrengr): refactor this into a method on Suspect which returns
-  # the cannonical frame (and documents why it's the one we return).
-  def GetClassFromSuspect(self, suspect):
-    """Determine which component is responsible for this suspect.
-
-    Note that Findit assumes files that the culprit suspect touched come from
-    the same component.
-    """
-    if suspect.file_to_stack_infos:
-      # file_to_stack_infos is a dict mapping file_path to stack_infos,
-      # where stack_infos is a list of (frame, callstack_priority)
-      # pairs. So ``.values()`` returns a list of the stack_infos in an
-      # arbitrary order; the first ``[0]`` grabs the "first" stack_infos;
-      # the second ``[0]`` grabs the first pair from the list; and
-      # the third ``[0]`` grabs the ``frame`` from the pair.
-      # TODO(wrengr): why is that the right frame to look at?
-      frame = suspect.file_to_stack_infos.values()[0][0][0]
-      return self.GetClassFromStackFrame(frame)
-
-    return ''
+    self.components = [] if components is None else components
+    self.top_n_frames = top_n_frames
 
   # TODO(http://crbug.com/657177): return the Component objects
   # themselves, rather than strings naming them.
-  def Classify(self, suspects, crash_stack):
+  def ClassifyCallStack(self, crash_stack, top_n=2):
     """Classifies component of a crash.
 
     Args:
-      suspects (list of Suspect): Culprit suspects.
       crash_stack (CallStack): The callstack that caused the crash.
 
     Returns:
       List of top 2 components.
     """
-    # If ``suspects`` are available, we use the components from there since
-    # they're more reliable than the ones from the ``crash_stack``.
-    if suspects:
-      classes = map(self.GetClassFromSuspect, suspects[:self.top_n])
-    else:
-      classes = map(self.GetClassFromStackFrame,
-          crash_stack.frames[:self.top_n])
+    def _GetClassFromStackFrame(frame):

[Martin Barbella, 2017/01/26 23:59:48]

s/Class/Component/ seems a bit clearer to me.

[Sharu Jiang, 2017/01/27 03:20:42]

Done.

+      """Determine which component is responsible for this frame."""
+      for component in self.components:
+        if component.MatchesStackFrame(frame):
+          return component.component_name
 
-    return RankByOccurrence(classes, 2)
-
-  # TODO(ymzhang): use component of new path as default. RENAME might
-  # need to return two (old path new path may have different components)
-  def GetClassFromFileChangeInfo(self, file_change_info):
-    """Determine which component is responsible for a touched file."""
-    if not file_change_info:
       return None
 
-    for component in self.components:
-      if (file_change_info.change_type == ChangeType.DELETE):
-        if component.MatchesFile(file_change_info.old_path):
-          return component.component_name
-      else:
-        if component.MatchesFile(file_change_info.new_path):
-          return component.component_name
+    classes = map(_GetClassFromStackFrame,

[Martin Barbella, 2017/01/26 23:59:48]

Similarly, classes is a bit too generic to be easily understood just from
glancing over the code.

[Sharu Jiang, 2017/01/27 03:20:42]

Done.

+                  crash_stack.frames[:self.top_n_frames])
+    return RankByOccurrence(classes, top_n) or None
 
-    return ''
+  # TODO(http://crbug.com/657177): return the Component objects
+  # themselves, rather than strings naming them.
+  def ClassifySuspects(self, suspects, top_n=2):
+    """Classifies component of a list of suspects.
 
-  def ClassifyChangeLog(self, change_log, top_n=2):
+    Args:
+      suspects (list of Suspect): Culprit suspects.
+
+    Returns:
+      List of top 2 components.
+    """
+    classes = []
+    for suspect in suspects:
+      classes.extend(self.ClassifySuspect(suspect))
+
+    return RankByOccurrence(classes, top_n) or None
+
+  def ClassifySuspect(self, suspect, top_n=2):
     """ Classifies components of a change log.
 
      Args:
        change_log: a change log
        top_n: number of components assigned to this change log, default is 2
 
      Returns:
        List of components
      """
-    if not change_log:
+    if not suspect or not suspect.changelog:
       return None
 
-    classes = map(self.GetClassFromFileChangeInfo, change_log.touched_files)
-    return RankByOccurrence(classes, top_n, rank_function=lambda x:-len(x))
+    # TODO(ymzhang): use component of new path as default. RENAME might

[Martin Barbella, 2017/01/26 23:59:48]

Same concern mentioned on this TODO in the previous file. Also, if it does seem
worth keeping, it feels unnecessary to have it in two places.

[Sharu Jiang, 2017/01/27 03:20:42]

Done.

+    # need to return two (old path new path may have different components)
+    def _GetClassFromTouchedFile(dep_path, touched_file):
+      """Determine which component is responsible for a touched file."""
+      for component in self.components:
+        if component.MatchesTouchedFile(dep_path, touched_file):
+          return component.component_name
+
+      return None
+
+    get_class = functools.partial(_GetClassFromTouchedFile, suspect.dep_path)
+    classes = map(get_class, suspect.changelog.touched_files)
+    return RankByOccurrence(classes, top_n,
+                            rank_function=lambda x:-len(x)) or None