[Findit] Add skeleton code for delta test script.

appengine/findit/util_scripts/crash_queries/delta_test/delta_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.
+
+import json
+import logging
+import os
+import pickle
+import subprocess
+
+from crash_queries import crash_iterator
+from crash_queries.delta_test import delta_util
+
+AZALEA_RESULTS_DIRECTORY = os.path.join(os.path.dirname(__file__),
+                                        'azalea_results')
+DELTA_TEST_DIRECTORY = os.path.dirname(__file__)
+
+
+class Delta(object):
+  """Stands for delta between two results."""
+
+  def __init__(self, result1, result2, fields):
+    self._result1 = result1
+    self._result2 = result2
+    self._fields = fields
+    self._delta_dict = {}
+    self._delta_str_dict = {}
+
+  @property
+  def delta_dict(self):
+    """Dict representation of delta."""

[lijeffrey, 2016/10/20 15:52:14]

is it possible to include an example of what the resulting dict should look
like?

[Sharu Jiang, 2016/10/20 22:39:06]

Done.

+    if self._delta_dict:
+      return self._delta_dict
+
+    for field in self._fields:
+      value1 = getattr(self._result1, field)
+      value2 = getattr(self._result2, field)
+      if value1 != value2:
+        if hasattr(value1, 'ToDict') and callable(value1.ToDict):
+          value1 = value1.ToDict()
+          value2 = value2.ToDict()

[lijeffrey, 2016/10/20 15:52:14]

does value2 need to be checked if callable(value2.ToDict)?

[Sharu Jiang, 2016/10/20 22:39:06]

The result1 and result2 should be the same kind, so no need to check value2. 

Added a Note of that in Delta class doc.

+        self._delta_dict[field] = (value1, value2)
+
+    return self._delta_dict
+
+  @property
+  def delta_str_dict(self):
+    """Converts delta of each field to a string."""
+    if self._delta_str_dict:
+      return self._delta_str_dict
+
+    for key, (value1, value2) in self.delta_dict.iteritems():
+      self._delta_str_dict[key] = '%s: %s, %s' % (key, value1, value2)
+
+    return self._delta_str_dict
+
+  def ToDict(self):
+    return self.delta_dict
+
+  def __str__(self):
+    return '\n'.join(self.delta_str_dict.values())
+
+  def __bool__(self):
+    return bool(self.delta_dict)
+
+  def __nonzero__(self):
+    return self.__bool__()
+
+
+def GetDeltasFromTwoSetsOfResults(set1, set2):
+  """Gets delta from two sets of results.
+

[lijeffrey, 2016/10/20 15:52:14]

nit: include in the docstring that set1 and set2 are dicts (?)

[Sharu Jiang, 2016/10/20 22:39:06]

Done.

+  Results are a list of (message, matches, component_name, cr_label)
+  Returns a list of delta results (results1, results2).
+  """
+  deltas = {}
+  for crash_id, result1 in set1.iteritems():
+    # Even when the command are exactly the same, it's possible that one set is
+    # loaded from local result file, another is just queried from database,
+    # sometimes some crash results would get deleted.
+    if crash_id not in set2:

[lijeffrey, 2016/10/20 15:52:14]

nit: set2.keys()?

[Sharu Jiang, 2016/10/20 22:39:06]

This is valid usage for dict.

+      continue
+
+    result2 = set2[crash_id]
+    delta = Delta(result1, result2, result1.fields)
+    if delta:
+      deltas[crash_id] = delta
+
+  return deltas
+
+
+def GetResults(crashes, client_id, git_hash, result_path, verbose=False):
+  """Returns an evaluator function to compute delta between 2 findit githashes.
+
+  Args:
+    crashes (list): A list of crash infos.
+    client_id (str): Possible values - fracas/cracas/clustefuzz.
+    git_hash (str): A git hash of findit repository.
+    result_path (str): file path for subprocess to write results on.
+    verbose (bool): If True, print all the findit results.
+
+  Return:
+    A dict mapping crash id to culprit for every crashes analyzed by
+    git_hash version.
+  """
+  if not crashes:
+    return {}
+
+  if verbose:
+    logging.info('\n\n***************************')
+    logging.info('Switch to git %s', git_hash)

[lijeffrey, 2016/10/20 15:52:14]

nit: Switching?

[Sharu Jiang, 2016/10/20 22:39:06]

Done.

+    logging.info('***************************\n\n')
+
+  dev_null_handle = open(os.devnull, 'w')
+  subprocess.check_call(
+      'cd %s; git checkout %s' % (DELTA_TEST_DIRECTORY, git_hash),
+      stdout=dev_null_handle,
+      stderr=dev_null_handle,
+      shell=True)
+
+  if not os.path.exists(result_path):
+    args = ['python', 'run-azalea.py', result_path, '--client', client_id]
+    if verbose:
+      args.append('--verbose')
+    p = subprocess.Popen(args, stdin=subprocess.PIPE)
+    p.communicate(input=json.dumps(crashes))

[stgao, 2016/10/20 01:40:20]

Just a note: not sure if the passing a big chunk of data through stdio would be
a problem or not.

[Sharu Jiang, 2016/10/20 22:39:06]

It works fine for the largest batch, and the crash data won't be largely
expanded, so it should be ok.

Added a todo, I will address this potential problem by caching the crashes data
and let the run-azalea read corresponding cache file to get input.

+  else:
+    logging.info('\nLoading results from %s', result_path)
+
+  if not os.path.exists(result_path):
+    logging.error('Fail to get results.')

[lijeffrey, 2016/10/20 15:52:14]

nit: Failed

[Sharu Jiang, 2016/10/20 22:39:06]

Done.

+    return {}
+
+  with open(result_path) as f:

[stgao, 2016/10/20 01:40:20]

As the result_path is passed in, why this function want to read the file and
return the content?

[Sharu Jiang, 2016/10/20 22:39:06]

We can read the file outside this function however I think encapsulate this part
in GetResults would be cleaner.

[stgao, 2016/10/21 01:41:39]

Then why we pass the result_path into this function? That's not encapsulation.

[Sharu Jiang, 2016/10/21 18:45:53]

The result path is needed so we can check whether we already have results to
determine whether we should call run-azalea(run-predator) or not.

+    return pickle.load(f)
+
+  return {}
+
+
+def DeltaEvaluator(git_hash1, git_hash2,
+                   client_id, app_id,
+                   start_date, end_date, batch_size,
+                   property_values=None, verbose=False):
+  """Evaluates delta between git_hash1 and git_hash2 on a set of Testcases.
+
+  Args:
+    git_hash1 (str): A git hash of findit repository.
+    git_hash2 (str): A git hash of findit repository.
+    start_date (str): Run delta test on testcases after (including)
+      the start_date, format should be '%Y-%m-%d'.
+    end_date (str): Run delta test on testcases before (not including)
+      the end_date, format should be '%Y-%m-%d'.
+    client_id (CrashClient): Possible values are 'fracas', 'cracas',
+      'cluterfuzz'.
+    app_id (str): Appengine app id to query.
+    batch_size (int): Size of a batch that can be queried at one time.
+    property_values (dict): Property values to query.
+       batch_size (int): The size of crashes that can be queried at one time.
+    verbose (bool): If True, print all the findit results.
+  Return:
+    (deltas, crash_count).
+    deltas (dict): Mappings id to delta for each culprit value.
+    crash_count (int): Total count of all the crashes.
+  """
+  head_branch_name = subprocess.check_output(
+      ['git', 'rev-parse', '--abbrev-ref', 'HEAD']).replace('\n', '')
+  try:
+    deltas = {}
+    crash_count = 0
+    for index, crashes in enumerate(
+        crash_iterator.IterateCrashes(client_id, app_id,
+                                      property_values=property_values,
+                                      start_date=start_date,
+                                      end_date=end_date,
+                                      batch_size=batch_size,
+                                      batch_run=True)):
+
+      results = []
+      for git_hash in [git_hash1, git_hash2]:
+        result_path = os.path.join(
+            AZALEA_RESULTS_DIRECTORY, delta_util.GenerateFileName(
+                client_id, property_values, start_date, end_date,
+                batch_size, index, git_hash))
+        results.append(GetResults(crashes, client_id, git_hash, result_path,
+                                  verbose=verbose))
+
+      crash_count += len(crashes)
+      deltas.update(GetDeltasFromTwoSetsOfResults(*results))
+
+    return deltas, crash_count
+  finally:
+    dev_null_handle = open(os.devnull, 'w')

[stgao, 2016/10/20 01:40:20]

Should we do a with clause here? Otherwise a file handle is leaking.

[Sharu Jiang, 2016/10/20 22:39:06]

Done.

+    subprocess.check_call(['git', 'checkout', head_branch_name],
+                          stdout=dev_null_handle,
+                          stderr=dev_null_handle)