Intial checkin of layout test analyzer.

media/tools/layout_tests/layouttest_analyzer_helpers.py

+#!/usr/bin/python
+# Copyright (c) 2011 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.
+
+"""A module for helper functions for the layouttest analyzer."""

[dennis_jeffrey, 2011/08/24 17:40:47]

'Helper functions for the layout test analyzer.'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+import copy
+from datetime import datetime
+from email.mime.multipart import MIMEMultipart
+from email.mime.text import MIMEText
+import os
+import pickle
+import smtplib
+import time
+import urllib
+
+from bug import Bug
+from test_expectations_history import TestExpectationsHistory
+
+
+class AnalyzerResultMap:
+  """A class to deal with joined result produed by the analyzer.
+
+  The join is done between layouttests and the test_expectations object
+  (based on the test expectation file). The instance variable |result_map|
+  contains the following keys: 'whole','skip','nonskip'. The value of 'whole'
+  contains information about all layouttests. The value of 'skip' contains
+  information about skipped layouttests where it has 'SKIP' in its entry in
+  the test expectation file. The value of 'nonskip' contains all information
+  about non skipped layout tests, which are in the test expectation file but
+  no skipped. The information is exactly same as the one parsed by the

[dennis_jeffrey, 2011/08/24 17:40:47]

'no skipped' --> 'not skipped'

[imasaki1, 2011/08/25 23:57:03]

Done.

+  analyzer.
+  """
+
+  def __init__(self, test_info_map):
+    """Initialize the Result based on test_info_map.

[dennis_jeffrey, 2011/08/24 17:40:47]

'Result' --> 'AnalyzerResultMap'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    Test_info_map contains all layouttest information. The job here is to
+    classify them to 'whole', 'skip' or 'nonskip' based on that information.

[dennis_jeffrey, 2011/08/24 17:40:47]

'them to' --> 'them as'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    Args:
+      test_info_map: the result map of layouttests.JoinWithTestExpectation,
+          The key of the map is testname such as 'media/media-foo.html'.
+          The value of the map is a map that contains the following keys: desc
+          (description), te_info (test expectation information), which is
+          a list of test expectation information map. The key of the map is
+          test expectation keywords such as "SKIP".

[dennis_jeffrey, 2011/08/24 17:40:47]

You first mention that the map contains keys "desc" and "te_info".  But in the
next sentence, you say the key of the map is keywords such as "SKIP".  This is a
little confusing; could you clarify the description here?  Also, isn't it
lower-case "skip" instead of capital "SKIP"?

[imasaki1, 2011/08/25 23:57:03]

Added more comments, here.

+    """
+    self.result_map = {}
+    self.result_map['whole'] = {}
+    self.result_map['skip'] = {}
+    self.result_map['nonskip'] = {}
+    if test_info_map is not None:

[dennis_jeffrey, 2011/08/24 17:40:47]

I think we can just use "if test_info_map:"

[imasaki1, 2011/08/25 23:57:03]

Done.

+      for (k, v) in test_info_map.iteritems():
+        self.result_map['whole'][k] = v
+        if 'te_info' in v:
+          skip = False
+          for e in v['te_info']:
+            if 'SKIP' in e:
+              skip = True
+              break

[dennis_jeffrey, 2011/08/24 17:40:47]

I think we can shorten lines 58-62 above using the built-in function any() and
python's list comprehensions, but I leave that as an optional exercise for you
if you want to practice it :-)

[imasaki1, 2011/08/25 23:57:03]

Done.

+          if skip:
+            self.result_map['skip'][k] = v
+          else:
+            self.result_map['nonskip'][k] = v
+
+  @staticmethod
+  def GetDiffString(diff_map_element, type_str):
+    """Get difference string out of diff map element

[dennis_jeffrey, 2011/08/24 17:40:47]

What do you mean by a "difference string"?  Also, add a period at the end of the
line.

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    This is used for generating email message.

[dennis_jeffrey, 2011/08/24 17:40:47]

'email message' -->
'email messages'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    Args:
+      diff_map_element: the compared map generated by |CompareResultMaps()|

[dennis_jeffrey, 2011/08/24 17:40:47]

Add a period or comma at the end of this line.  If period, capitalize the first
letter in the next line below.

[imasaki1, 2011/08/25 23:57:03]

Done.

+          also, this is for each test group ('whole', 'skip', 'nonskip')

[dennis_jeffrey, 2011/08/24 17:40:47]

Add period at end of this sentence.

[imasaki1, 2011/08/25 23:57:03]

Done.

+

[dennis_jeffrey, 2011/08/24 17:40:47]

What about the "type_str" arg?

[imasaki1, 2011/08/25 23:57:03]

Done.

+    Return:

[dennis_jeffrey, 2011/08/24 17:40:47]

'Returns:'

[imasaki1, 2011/08/25 23:57:03]

Done.

+      a string including diff information.

[dennis_jeffrey, 2011/08/24 17:40:47]

what do you mean by "diff information"?

[imasaki1, 2011/08/25 23:57:03]

Done.

+    """
+    diff = len(diff_map_element[0]) - len(diff_map_element[1])

[dennis_jeffrey, 2011/08/24 17:40:47]

I thought diff_map_element is a dictionary with keys such as "whole", "skip",
etc.  Is that correct?  If so, then would it be a syntax error to access it as
diff_map_element[0]?

[imasaki1, 2011/08/25 23:57:03]

no. It is a tuple with two lists. I will modify the comments to avoid the
confusion.

[dennis_jeffrey, 2011/08/26 19:01:26]

Got it.  Thanks.

+    if diff == 0:
+      return 'No Change'
+    color = ''
+    if diff > 0 and type_str != 'whole':
+      color = 'red'
+    else:
+      color = 'green'
+    str = '<font color="%s">+%d</font>' % (color, diff)

[dennis_jeffrey, 2011/08/24 17:40:47]

was it intentional to have a '+' within the string here, given that "diff" may
be either a positive or negative value at this point?

[imasaki1, 2011/08/25 23:57:03]

Good catch I will add only when diff is negative. Modify the code.

+    str1 = ''
+    for (name, v) in diff_map_element[0]:
+      str1 += name + ","

[dennis_jeffrey, 2011/08/24 17:40:47]

use single quotes in the string

[imasaki1, 2011/08/25 23:57:03]

Done.

+    str1 = str1[:-1]
+    str2 = ''
+    for (name, v) in diff_map_element[1]:
+      str2 += name + ","
+    str2 = str2[:-1]
+    if str1 or str2:
+      str += ":"
+    if str1:
+      str += '<font color="%s">%s</font> ' % (color, str1)
+    if str2:
+      str += '<font color="%s">%s</font>' % (color, str2)
+    return str
+
+  def ConvertToString(self, prev_time, diff_map, bug_anno_map):
+    """Convert this result to HTML display for email.
+
+    Args:
+      prev_time: the previous time string that are compared against.
+      diff_map: the compared map generated by |CompareResultMaps()|.
+      anno_map: a annotation map where keys are bug names and values are

[dennis_jeffrey, 2011/08/24 17:40:47]

'anno_map' --> 'bug_anno_map'

[imasaki1, 2011/08/25 23:57:03]

Done.

+          annotation for the bug.

[dennis_jeffrey, 2011/08/24 17:40:47]

'annotation' --> 'annotations'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    Returns:
+      a analyzer result string in HTML format.
+    """
+    d = len(self.result_map['whole'].keys()) - (
+        len(self.result_map['skip'].keys()))
+    # Passing rate is calculated like the followings.

[dennis_jeffrey, 2011/08/24 17:40:47]

I think this comment is not necessary; we can see the passing_rate calculated on
the next line below.

[imasaki1, 2011/08/25 23:57:03]

Removed. Also, this part is replaced by function call since it is used when
generating graph as well.

+    passing_rate = 100 - len(self.result_map['nonskip'].keys()) * 100 / d
+    str = ('<b>Statistics (Diff Compared to %s):</b><ul>'
+           '<li>The number of tests: %d (%s)</li>'
+           '<li>The number of failing skipped tests: %d (%s)</li>'
+           '<li>The number of failing non-skipped tests: %d (%s)</li>'
+           '<li>Passing rate: %d %%</li></ul>') % (
+        prev_time,
+        len(self.result_map['whole'].keys()),
+        AnalyzerResultMap.GetDiffString(diff_map['whole'], 'whole'),
+        len(self.result_map['skip'].keys()),
+        AnalyzerResultMap.GetDiffString(diff_map['skip'], 'skip'),
+        len(self.result_map['nonskip'].keys()),
+        AnalyzerResultMap.GetDiffString(diff_map['nonskip'], 'nonskip'),
+        passing_rate)
+    str += '<b>Current issues about failing non-skipped tests:</b>'
+    for (bug_txt, test_info_list) in (
+        self.GetListOfBugsForNonSkippedTests().iteritems()):
+      if not bug_txt in bug_anno_map:
+        bug_anno_map[bug_txt] = '<font color="red">Needs investigation!</font>'
+      str += '<ul>%s (%s)' % (Bug(bug_txt).ToString(), bug_anno_map[bug_txt])

[dennis_jeffrey, 2011/08/24 17:40:47]

Do you use a Bug object anywhere else?  If so, that's ok.  If not, you should
consider removing Bug as a class if its only purpose it to have its ToString()
function called.

[imasaki1, 2011/08/25 23:57:03]

I am planning to implement several more functions in Bug class (you can see my
TODO in the class). So, I prefer to keep as it is.

+      for test_info in test_info_list:
+        (test_name, te_info) = test_info
+        gpu_link = ''
+        if 'GPU' in te_info:
+          gpu_link = 'group=%40ToT%20GPU%20Mesa%20-%20chromium.org&'
+        dashboard_link = ('http://test-results.appspot.com/dashboards/'
+                  'flakiness_dashboard.html#%stests='
+                  '%s' % (gpu_link, test_name))

[dennis_jeffrey, 2011/08/24 17:40:47]

nit: indent the above 2 lines some more so they line up underneath the start of
the string in line 146 above.

[imasaki1, 2011/08/25 23:57:03]

Done.

+        str += '<li><a href="%s">%s</a> (%s) </li>' % (
+            dashboard_link, test_name, ' '.join(te_info.keys()))
+      str += '</ul>\n'
+    return str
+
+  def CompareResultMaps(self, result_map2):

[dennis_jeffrey, 2011/08/24 17:40:47]

How about 'other_result_map' instead of 'result_map2'?  Having the '2' in the
name suggests there may be another variable called 'result_map1', which is not
exactly the case here.

Also, this function is meant to compare the result_map of the current object to
the other, specified result map, so how about changing the function name to
"CompareToOtherResultMap"?

[imasaki1, 2011/08/25 23:57:03]

Done.

+    """Compare this result map with the other to see if any difference.

[dennis_jeffrey, 2011/08/24 17:40:47]

'any difference' --> 'there are any differences'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    The comparison is done for layouttests which belong to 'whole', 'skip',
+    or 'nonskip'.
+
+    Args:
+      result_map2: another result map to be compared againt this result.

[dennis_jeffrey, 2011/08/24 17:40:47]

'this result' --> 'the result map of the current object'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    Returns:
+      a comp_result_map, which contains 'whole', 'skip' and 'nonskip' as keys.

[dennis_jeffrey, 2011/08/24 17:40:47]

We shouldn't say 'comp_result_map' in the description here, since that just
happens to be the name of the variable used by this function.  The description
here should be understandable even if someone cannot look into the code itself. 
Is there a way to describe 'comp_result_map' more generally, rather than
referring to the variable name here?

[imasaki1, 2011/08/25 23:57:03]

Modified comment here.

+           The values are the result of |GetDiffBetweenMaps()| that have two
+           tuples, where one is for a list of current tests diff and the other

[dennis_jeffrey, 2011/08/24 17:40:47]

'tests diff' --> 'test diffs'

[imasaki1, 2011/08/25 23:57:03]

Deleted.

+           one is for a list of previous test diff.

[dennis_jeffrey, 2011/08/24 17:40:47]

'diff' --> 'diffs'

[imasaki1, 2011/08/25 23:57:03]

Deleted

+           For example (test expectaion information is omitted for simplicity),
+             comp_result_map['whole'][0] = ['foo1.html']
+             comp_result_map['whole'][1] = ['foo2.html']
+           This means that current result has 'foo1.html' but not in previous
+           result. This also means the previous result has 'foo2.html' but not
+           current one.

[dennis_jeffrey, 2011/08/24 17:40:47]

Should we add a comment somewhere explicitly saying that index 0 of the tuple
corresponds to the "current" result, and index 1 of the tuple corresponds to the
"previous" result?

[imasaki1, 2011/08/25 23:57:03]

Done.

+    """
+    comp_result_map = {}
+    for name in ['whole', 'skip', 'nonskip']:
+      if name == 'nonskip':
+        # Look into expectation to get diff only for non-skipped tests.
+        lookintoTestExpectaionInfo = True

[dennis_jeffrey, 2011/08/24 17:40:47]

'lookintoTestExpectaionInfo' -->
'lookIntoTestExpectationInfo'

[imasaki1, 2011/08/25 23:57:03]

Done.

+      else:
+        #  Otherwise, only test names are compared to get diff.
+        lookintoTestExpectaionInfo = False
+      comp_result_map[name] = GetDiffBetweenMaps(
+         self.result_map[name], result_map2.result_map[name],
+         lookintoTestExpectaionInfo)
+    return comp_result_map
+
+  @staticmethod
+  def Load(file_path):
+    """Load the object from |file_path| using pickle library.
+
+    Args:
+      file_path: the file path to be read the result from.

[dennis_jeffrey, 2011/08/24 17:40:47]

'the string path to the file from which to read the result'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    Returns:
+       a AnalyzerResultMap object read from |file_path|.
+    """
+    file_object = open(file_path)
+    analyzer_result_map = pickle.load(file_object)
+    file_object.close()
+    return analyzer_result_map
+
+  def Save(self, file_path):
+    """Save the object to |file_path| using pickle library.
+
+    Args:
+       file_path: the file path to be read the result from.

[dennis_jeffrey, 2011/08/24 17:40:47]

'the string path to the file in which to store the result'

[imasaki1, 2011/08/25 23:57:03]

Done.

+    """
+    file_object = open(file_path, "wb")

[dennis_jeffrey, 2011/08/24 17:40:47]

use single quotes in strings

[imasaki1, 2011/08/25 23:57:03]

Done.

+    pickle.dump(self, file_object)
+    file_object.close()
+
+  def GetListOfBugsForNonSkippedTests(self):
+    """Get a lit of bugs for non-skipped layout tessts.

[dennis_jeffrey, 2011/08/24 17:40:47]

'lit' --> 'list'

'tessts' --> 'tests'

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+    This is used for generating email content.

[dennis_jeffrey, 2011/08/24 17:40:47]

Add a "Returns:" section to this docstring

[imasaki1, 2011/08/25 23:57:03]

Done.

+    """
+    bug_map = {}
+    for (name, v) in self.result_map['nonskip'].iteritems():
+        for te_info in v['te_info']:

[dennis_jeffrey, 2011/08/24 17:40:47]

only indent lines 220-228 by 2 spaces underneath line 219, not 4.

[imasaki1, 2011/08/25 23:57:03]

Done.

+          main_te_info = {}
+          for k in te_info.keys():
+            if k != 'Comments' and k != 'Bugs':
+              main_te_info[k] = True
+          for bug in te_info['Bugs']:
+            if bug not in bug_map:
+              bug_map[bug] = []
+            bug_map[bug].append((name, main_te_info))
+    return bug_map
+
+
+def SendStatusEmail(prev_time, analyzer_result_map, prev_analyzer_result_map,
+                    bug_anno_map, receiver_email_address):
+  """Send status email.
+
+  Args:
+    prev_time: the date string such as '2011-10-09-11'. This format has been
+        used in this analyzer.
+    analyzer_result_map: current analyzer result.
+    prev_analyzer_result_map: previous analyzer reusult, which is read from
+         a file.
+    bug_anno_map: bug annotation map where bug name and annotations are
+         stored.
+    receiver_email_address: reciever's email address.

[dennis_jeffrey, 2011/08/24 17:40:47]

reciever's -->
receiver's

[imasaki1, 2011/08/25 23:57:03]

Done.

+  """
+  diff_map = analyzer_result_map.CompareResultMaps(prev_analyzer_result_map)
+  str = analyzer_result_map.ConvertToString(prev_time, diff_map, bug_anno_map)
+  # Add diff info about skipped/non-skipped test.
+  prev_time = datetime.strptime(prev_time, "%Y-%m-%d-%H")

[dennis_jeffrey, 2011/08/24 17:40:47]

use single quotes in strings

[imasaki1, 2011/08/25 23:57:03]

Done.

+  prev_time = time.mktime(prev_time.timetuple())
+  testname_map = {}
+  for (k, v) in diff_map['skip'][0]:
+    testname_map[k] = True
+  for (k, v) in diff_map['skip'][1]:
+    testname_map[k] = True
+  for (k, v) in diff_map['nonskip'][0]:
+    testname_map[k] = True
+  for (k, v) in diff_map['nonskip'][1]:
+    testname_map[k] = True

[dennis_jeffrey, 2011/08/24 17:40:47]

How about something like this?

for type in ['skip', 'nonskip']:
  for i in range(2):
    for (k, _) in diff_map[type][i]:
      testname_map[k] = True

[imasaki1, 2011/08/25 23:57:03]

Done.

+  now = time.time()
+
+  rev_infos = TestExpectationsHistory.GetDiffBetweenTimes(now, prev_time,
+                                                          testname_map.keys())
+  if len(rev_infos) > 0:

[dennis_jeffrey, 2011/08/24 17:40:47]

if rev_infos:

[imasaki1, 2011/08/25 23:57:03]

Done.

+    str += '<br><b>Revision Information:</b>'
+  for rev_info in rev_infos:
+    (old_rev, new_rev, author, date, message, target_lines) = rev_info
+    l = urllib.unquote('http://trac.webkit.org/changeset?new=%d%40trunk%2F'
+                       'LayoutTests%2Fplatform%2Fchromium%2F'
+                       'test_expectations.txt&old=%d%40trunk%2FLayoutTests%2F'
+                       'platform%2Fchromium%2Ftest_expectations.txt')
+    link = l % (new_rev, old_rev)

[dennis_jeffrey, 2011/08/24 17:40:47]

Maybe combine the above 2 statements so that we don't need to define the 'l'
variable?

[imasaki1, 2011/08/25 23:57:03]

Done.

+    str += '<ul><a href="%s">%s->%s</a>\n' % (link, old_rev, new_rev)
+    str += '<li>%s</li>\n' % author
+    str += '<li>%s</li>\n<ul>' % date
+    for line in target_lines:
+      str += '<li>%s</li>\n' % line
+    str += '</ul></ul>'
+  localtime = time.asctime(time.localtime(time.time()))
+  # TODO(imasaki): remove my name from here.
+  SendEmail('imasaki@chromium.org', 'Kenji Imasaki',
+            [receiver_email_address], ['Layout Test Analyzer Result'],
+            'Layout Test Analyzer Result : ' + localtime, str)
+
+
+def SendEmail(sender_email_address, sender_name, receivers_email_addresses,

[dennis_jeffrey, 2011/08/24 17:40:47]

This seems to only be called at line 281 above.  Do we need this as a separate
function, or can we put it all in the function above?

[imasaki1, 2011/08/25 23:57:03]

I prefer to separate pure email function and status email function. So, later
on, we can use just SendEmail function only (if we want to). 

+              receivers_names, subject, message):
+    """Send email using localhost's mail server.
+
+    Args:
+        sender_email_address: sender's email address.
+        sender_name: sender's name.
+        receivers_email_addresses: receiver's email addresses.
+        receivers_names:  receiver's names.
+        subject: subject string.
+        message: email message.
+    """
+    whole_message = ''.join([
+        'From: %s<%s>\n' % (sender_name, sender_email_address),
+        'To: %s<%s>\n' % (receivers_names[0],
+                          receivers_email_addresses[0]),
+        'Subject: %s\n' % subject, message])
+
+    try:
+        html_top = """

[dennis_jeffrey, 2011/08/24 17:40:47]

only indent all the lines within the 'try' by 2 spaces, not 4.

[imasaki1, 2011/08/25 23:57:03]

Done.

+             <html>
+             <head></head>
+             <body>
+             """
+        html_bot = """
+             </body>
+            </html>
+        """
+        html = html_top + message + html_bot
+        msg = MIMEMultipart('alternative')
+        msg['Subject'] = subject
+        msg['From'] = sender_email_address
+        msg['To'] = receivers_email_addresses[0]
+        part1 = MIMEText(html, 'html')
+        smtpObj = smtplib.SMTP('localhost')
+        msg.attach(part1)
+        smtpObj.sendmail(sender_email_address,
+                          receivers_email_addresses,
+                          msg.as_string())

[dennis_jeffrey, 2011/08/24 17:40:47]

nit: indent the above 2 lines by 1 fewer space each

[imasaki1, 2011/08/25 23:57:03]

Done.

+        print 'Successfully sent email'
+    except smtplib.SMTPException:
+        print 'Error: unable to send email'

[dennis_jeffrey, 2011/08/24 17:40:47]

only indent line by 2 spaces, not 4

[dennis_jeffrey, 2011/08/24 17:40:47]

Maybe we should also capture the exception message and print that out here too

[imasaki1, 2011/08/25 23:57:03]

Done.

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+
+def FindLatestTime(time_list):
+  """Find latest time from |time_list|.
+
+  The current status is compared to the status of the latest file in
+  |RESULT_DIR|.
+
+  Args:
+    time_list: a list of time string in the form of '2011-10-23-23'
+
+  Returns:
+     a string representing latest time among the time_list.

[dennis_jeffrey, 2011/08/24 17:40:47]

add this: 'or None if |time_list| is empty.'

[imasaki1, 2011/08/25 23:57:03]

Done.

+  """
+  latest_date = None
+  for t in time_list:
+    item_date = datetime.strptime(t, "%Y-%m-%d-%H")
+    if latest_date == None or latest_date < item_date:
+      latest_date = item_date
+  return latest_date.strftime("%Y-%m-%d-%H")

[dennis_jeffrey, 2011/08/24 17:40:47]

Be careful: if time_list is empty, lastest_date will be None here, leading to a
problem at this line.  We should probably add a check to prevent against this
case.

[imasaki1, 2011/08/25 23:57:03]

Done.

+
+
+def FindLatestResult(result_dir):
+  """Find the latest result in |result_dir| and read and return them.
+
+  This is used for comparison of analyzer result between current analyzer
+  and most known latest result.
+
+  Args:
+    result_dir: the result directory.
+
+  Returns:
+    a tuple of filename (latest_time) of the and the latest analyzer result.
+  """
+  dirList = os.listdir(result_dir)
+  file_name = FindLatestTime(dirList)

[dennis_jeffrey, 2011/08/24 17:40:47]

Why not remove FindLatestTime and just put that code inside this function, since
it's only used here anyway?

[imasaki1, 2011/08/25 23:57:03]

I used this in the unit test. This needs to be seperate function to be unit
tested.

+  file_path = os.path.join(result_dir, file_name)
+  return (file_name, AnalyzerResultMap.Load(file_path))
+
+
+def GetTestExpectationDiffBetweenLists(list1, list2):
+  """Get test expectation diff between lists.
+
+  Args:
+    list1: a list of test expectation information.
+    list2: a list of test expectation information.
+
+  Returns:
+    a list of the difference between test expectation information.

[dennis_jeffrey, 2011/08/24 17:40:47]

'difference between test' -->
'differences between the test'

[imasaki1, 2011/08/25 23:57:03]

Done.

+  """
+  result_list = []
+  for l1 in list1:
+    found = False
+    for l2 in list2:
+      if l1 == l2:
+        found = True
+        break
+    if not found:
+      result_list.append(l1)
+  return result_list

[dennis_jeffrey, 2011/08/24 17:40:47]

This function seems to only identify the elements in list1 that are also not in
list2.  If there are any elements in list2 that are not in list1, I think we'll
miss those.  Is that intentional?  If so, we should make sure to describe this
in the docstring, and even possibly change the name of the function to make this
clear.

[imasaki1, 2011/08/25 23:57:03]

Deleted.

+
+
+def GetDiffBetweenMaps(map1, map2, lookintoTestExpectaionInfo=False):

[dennis_jeffrey, 2011/08/24 17:40:47]

'lookintoTestExpectaionInfo' -->
'lookIntoTestExpectationInfo'

[imasaki1, 2011/08/25 23:57:03]

Done.

+  """Get difference between maps.
+
+  Args:
+    map1: analyzer result map to be compared.
+    map2: analyzer result map to be compared.
+    lookintoTestExpectaionInfo: a boolean to indicate whether you compare

[dennis_jeffrey, 2011/08/24 17:40:47]

'a boolean to indicate whether to compare test expectation information in
addition to just the test case names'

[imasaki1, 2011/08/25 23:57:03]

Done.

+        test expetation information as well as testnames.
+
+  Returns:
+    a tuple of |name1_list| and |name2_list|. |Name1_list| contains all test
+        name and the test expectation information in |map1| but not in |map2|.
+        |Name2_list| contains all test name and the test expectation
+        information in |map2| but not in |map1|.
+  """
+  name1_list = []
+  # Compare map1 with map2.
+  for (name, v1) in map1.iteritems():
+    if name in map2:
+      if lookintoTestExpectaionInfo and 'te_info' in v1:
+          te_diff = GetTestExpectationDiffBetweenLists(v1['te_info'],
+                                                      map2[name]['te_info'])
+          if te_diff:
+            name1_list.append((name, te_diff))
+    else:
+      name1_list.append((name, v1))
+
+  name2_list = []
+  # Compare map1 with map2.
+  for (name, v2) in map2.iteritems():
+    if name in map1:
+      if lookintoTestExpectaionInfo and 'te_info' in v2:
+      # Look into te_info.
+        te_diff = GetTestExpectationDiffBetweenLists(v2['te_info'],
+                                                    map1[name]['te_info'])
+        if te_diff:
+          name2_list.append((name, te_diff))
+    else:
+      name2_list.append((name, v2))

[dennis_jeffrey, 2011/08/24 17:40:47]

There's a big chunk of code in this function that's almost duplicated twice: is
there a way you can write this code just once and use it twice?  Lines 405-415
and 417-428 are very similar to each other.

[imasaki1, 2011/08/25 23:57:03]

Done.

+  return (name1_list, name2_list)