Refactor/Simplify the test expectation code in media/tools/layout_tests/

media/tools/layout_tests/test_expectations.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 TestExpectionsManager.
+"""A Module for the Webkit layout test test expectation related class."""
 
-TestExpectaionManager manages data for Webkit layout tests.
-"""
+import re
+import urllib2
 
-import csv
-import re
-import time
-import urllib
-import pysvn
+# Default Webkit SVN location for chromium test expectation file.
+# TODO(imasaki) : support multiple test expectations files.
+DEFAULT_TEST_EXPECTATION_LOCATION = (
+    'http://svn.webkit.org/repository/webkit/trunk/'
+    'LayoutTests/platform/chromium/test_expectations.txt')
 
-from csv_utils import CsvUtils
-from layout_test_test_case import LayoutTestCaseManager
-from test_case_patterns import TestCasePatterns
+# The following is from test expectation syntax. The detail can be found in
+# http://www.chromium.org/developers/testing/
+# webkit-layout-tests#TOC-Test-Expectations
+# <decision> ::== [SKIP] [WONTFIX] [SLOW]
+DECISION_NAMES = ['SKIP', 'WONTFIX', 'SLOW']
+# <platform> ::== [GPU] [CPU] [WIN] [LINUX] [MAC]
+PLATFORM_NAMES = ['GPU', 'CPU', 'WIN', 'LINUX', 'MAC']
+# <config> ::== RELEASE | DEBUG
+CONFIG_NAMES = ['RELEASE', 'DEBUG']
+# <EXPECTATION_NAMES> ::== \
+#   [FAIL] [PASS] [CRASH] [TIMEOUT] [IMAGE] [TEXT] [IMAGE+TEXT]
+EXPECTATION_NAMES = ['FAIL', 'PASS', 'CRASH',
+                     'TIMEOUT', 'IMAGE', 'TEXT',
+                     'IMAGE+TEXT']
 
 
-class TestExpectationsManager(object):
-    """This class manages test expectations data for Webkit layout tests.
+class TestExpectations(object):
+  """A class to model the content of test expectation file for analysis.
 
-    The detail of the test expectation file can be found in
-    http://trac.webkit.org/wiki/TestExpectations.
+  The raw test expectation file can be found in
+  |DEFAULT_TEST_EXPECTATION_LOCATION|.
+  It is necessary to parse this file and store meaningful information for
+  the analysis (joining with existing layout tests using a test name).
+  Instance variable |all_test_expectation_info| is used.
+  A test name such as 'media/video-source-type.html' is used for the key
+  to store information. However, a test name can appear multiple times in
+  the test expectation file. So, the map should keep all the occurrences
+  information. For example, the current test expectation file has the following
+  two entries:
+  BUGWK58587 LINUX DEBUG GPU : media/video-zoom.html = IMAGE
+  BUGCR86714 MAC GPU : media/video-zoom.html = CRASH IMAGE
+  In this case, all_test_expectation_info['media/video-zoom.html'] will have
+  a list with two elements, each of which is the map of the test expectation
+  information. In other words,this is the generated map after parsing both
+  lines.
+    {'media/video-zoom.html': [{'LINUX':True, 'DEBUG':True ....},

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

Are the values always True/False?  If so ISTM you'd want a list/vector, not a
dict?

[imasaki1, 2011/08/20 08:25:36]

This map also contains 'Bugs' which holds a list of bugs. I prefer map since I
can look them up directly.

+                               {'MAC':True, 'GPU':True     ....}]
+  """
 
-    This class does the following:
-    (1) get test expectation file from WebKit subversion source
-        repository using pysvn.
-    (2) parse the file and generate CSV entries.
-    (3) create hyperlinks where appropriate (e.g., test case name).
+  @staticmethod
+  def GetAllDataNames():
+    """get all names relating to test expectation data.

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

All comments should start with capital letter (please make a pass over the CL)

[imasaki1, 2011/08/20 08:25:36]

Done.

+
+    Returns:
+      a list of all names (keywords) used in the test expectation files.
     """
+    return DECISION_NAMES + PLATFORM_NAMES + CONFIG_NAMES + EXPECTATION_NAMES
 
-    DEFAULT_TEST_EXPECTATION_DIR = (
-        'http://svn.webkit.org/repository/webkit/trunk/'
-        'LayoutTests/platform/chromium/')
+  def __init__(self, url=DEFAULT_TEST_EXPECTATION_LOCATION):
+    """read the test expectation file from the specified URL and parse it.
 
-    DEFAULT_TEST_EXPECTATION_LOCATION = (
-        'http://svn.webkit.org/repository/webkit/trunk/'
-        'LayoutTests/platform/chromium/test_expectations.txt')
+    All the parsed information is stored in the instance variable
+    |all_test_expectation_info|, which is the map where its key is test name
+    and its value is a list of the test expectation entries information map
+    such as 'SKIP' or 'Comments' or 'Bugs'.
 
-    CHROME_BUG_URL = 'http://code.google.com/p/chromium/issues/detail?id='
+    Args:
+      url: A URL string for the test expectation file.
+    """
+    self.all_test_expectation_info = {}
+    resp = urllib2.urlopen(url)
+    if resp.code != 200:
+      raise NameError('Test expectation file does not exist in %s' % source)
+    # Start parsing each line.
+    lines = resp.read().split('\n')
+    comments = ''
+    for line in lines:
+      if line.startswith('//'):
+        # Comments can be Multiple lines.
+        comments += line.replace('//', '')
+      elif not line:
+        comments = ''
+      else:
+        test_expectation_info = self.ParseLine(line, comments)
+        testname = TestExpectations.ExtractTestName(line)
+        if not testname in self.all_test_expectation_info:
+          self.all_test_expectation_info[testname] = []
+        # This is a list for multiple entries.
+        self.all_test_expectation_info[testname].append(test_expectation_info)
 
-    WEBKIT_BUG_URL = 'https://bugs.webkit.org/show_bug.cgi?id='
+  @staticmethod
+  def ExtractTestName(line):
+    """extract either a test name or a directory name from each line.
 
-    FLAKINESS_DASHBOARD_LINK = (
-        'http://test-results.appspot.com/dashboards/'
-        'flakiness_dashboard.html#tests=')
+    Args:
+      line: each line in the test expectation file.
 
-    TEST_CASE_PATTERNS = TestCasePatterns()
+    Returns:
+      a test name or directory name string. Returns '' if no matching.
+    """
+    # First try to find test case where ending with .html.
+    matches = re.search(r':\s+(\S+.html)', line)
+    # Next try to find directory
+    if matches:
+      return matches.group(1)
+    matches = re.search(r':\s+(\S+)', line)
+    if matches:
+      return matches.group(1)
+    else:
+      return ''

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

raise?

[imasaki1, 2011/08/20 08:25:36]

Done.

[Ami GONE FROM CHROMIUM, 2011/08/20 17:52:23]

I meant raise a sensible exception.  Doesn't this trigger a TypeError b/c
NoneType is not a valid thing to raise?

[imasaki1, 2011/08/20 18:07:38]

Done. Also added unit test for this case.

 
-    OTHER_FIELD_NAMES = ['TestCase', 'Media', 'Flaky', 'WebKitD', 'Bug',
-                         'Test']
+  @staticmethod
+  def ParseLine(line, comments):

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

|comments| should be |comment_prefix| or something like that, since it holds the
comments from previous lines; this line may have yet more comments that should
be appended to the group comment.

[imasaki1, 2011/08/20 08:25:36]

Done.

+    """Parse each line in test expectation and update test expectation info.
 
-    # The following is from test expectation syntax.
-    # BUG[0-9]+ [SKIP] [WONTFIX] [SLOW]
-    DECISION_COLUMN_NAMES = ['SKIP', 'SLOW']
-    # <platform> ::== [GPU] [CPU] [WIN] [LINUX] [MAC]
-    PLATFORM_COLUMN_NAMES = ['GPU', 'CPU', 'WIN', 'LINUX', 'MAC']
-    #<config> ::== RELEASE | DEBUG
-    CONFIG_COLUMN_NAMES = ['RELEASE', 'DEBUG']
-    # <EXPECTATION_COLUMN_NAMES> ::== \
-    #   [FAIL] [PASS] [CRASH] [TIMEOUT] [IMAGE] [TEXT] [IMAGE+TEXT]
-    EXPECTATION_COLUMN_NAMES = ['FAIL', 'PASS', 'CRASH',
-                                'TIMEOUT', 'IMAGE', 'TEXT',
-                                'IMAGE+TEXT']
+    This looks for each name in |GetAllDataNames()| in line and store in the
+    map and returns it. Also, store comments and bugs in the map.
 
-    # These are coming from metadata in comments in the test expectation file.
-    COMMENT_COLUMN_NAMES = ['UNIMPLEMENTED', 'KNOWNISSUE', 'TESTISSUE',
-                            'WONTFIX']
+    Args:
+      line: each line in the test expectation. For example,
+          "BUGCR86714 MAC GPU : media/video-zoom.html = CRASH IMAGE"
+      comments: comments in the test expectation. Usually, it exist just before
+          the entry.
 
-    RAW_COMMENT_COLUMN_NAME = 'COMMENTS'
-
-    def __init__(self):
-        """Initialize the test cases."""
-        self.testcases = []
-
-    def get_column_indexes(self, column_names):
-        """Get column indexes for given column names.
-
-        Args:
-            column_names: a list of column names.
-
-        Returns:
-            a list of indexes for the given column names.
-        """
-        all_field_names = self.get_all_column_names(True, True)
-        return [all_field_names.index(
-            field_name) for field_name in column_names]
-
-    def get_all_column_names(self, include_other_fields, include_comments):
-        """Get all column names that are used in CSV file.
-
-        Args:
-            include_other_fields: a boolean that indicates the result should
-                include other column names (defined in OTHER_COLUMNS_NAMES).
-            include_comments: a boolean that indicates the result should
-                include comment-related names.
-
-        Returns:
-            a list that contains column names.
-        """
-        return_list = (
-            self.DECISION_COLUMN_NAMES + self.PLATFORM_COLUMN_NAMES +
-            self.CONFIG_COLUMN_NAMES + self.EXPECTATION_COLUMN_NAMES)
-        if include_other_fields:
-            return_list = self.OTHER_FIELD_NAMES + return_list
-        if include_comments:
-            return_list.extend(self.COMMENT_COLUMN_NAMES)
-            return_list.append(self.RAW_COMMENT_COLUMN_NAME)
-        return return_list
-
-    def get_test_case_element(self, test_case, column_names):
-        """Get test case elements.
-
-        A test case is a collection of test case elements.
-
-        Args:
-            test_case: test case data that contains all test case elements.
-            column_names: column names for specifying test case elements.
-
-        Returns:
-            A list of test case elements for the given column names.
-        """
-        field_indexes = self.get_column_indexes(column_names)
-        test_case = self.get_test_case_by_name(test_case)
-        if test_case is None:
-            return []
-        return [test_case[fid] for fid in field_indexes]
-
-    def get_test_case_by_name(self, target):
-        """Get test case object by test case name.
-
-        Args:
-            target: test case name.
-
-        Returns:
-            A test case with the test case name or None if the test case
-                cannot be found.
-        """
-        for testcase in self.testcases:
-            # Test case name is stored in the first column.
-            if testcase[0] == target:
-                return testcase
-        return None
-
-    def get_all_test_case_names(self):
-        """Get all test case names.
-
-        Returns:
-            A list of test case names.
-        """
-        return [testcase[0] for testcase in self.testcases]
-
-    def generate_link_for_bug(self, bug):
-        """Generate link for a bug.
-
-        Parse the bug description accordingly. The bug description can be like
-        the following: BUGWK1234, BUGCR1234, BUGFOO.
-
-        Args:
-            bug: A string bug description
-
-        Returns:
-            A string that represents a bug link. Returns an empty
-                string if match is not found.
-        """
-        pattern_for_webkit_bug = 'BUGWK(\d+)'
-        match = re.search(pattern_for_webkit_bug, bug)
-        if match is not None:
-            return self.WEBKIT_BUG_URL + match.group(1)
-        pattern_for_chrome_bug = 'BUGCR(\d+)'
-        match = re.search(pattern_for_chrome_bug, bug)
-        if match is not None:
-            return self.CHROME_BUG_URL + match.group(1)
-        pattern_for_other_bug = 'BUG(\S+)'
-        match = re.search(pattern_for_other_bug, bug)
-        if match is not None:
-            return 'mailto:%s@chromium.org' % match.group(1).lower()
-        return ''
-
-    def generate_link_for_dashboard(self, testcase_name, webkit):
-        """Generate link for flakiness dashboard.
-
-        Args:
-            testcase_name: a string test case name.
-            webkit: a boolean indicating whether to use the webkit dashboard.
-
-        Returns:
-            A string link to the flakiness dashboard.
-        """
-        url = self.FLAKINESS_DASHBOARD_LINK + urllib.quote(testcase_name)
-        if webkit:
-            url += '&master=webkit.org'
-        return url
-
-    def parse_line(self, line, previous_comment_info_list, test_case_patterns,
-                   media_test_cases_only, writer):
-        """Parse each line in test_expectations.txt.
-
-        The format of each line is as follows:
-            BUG[0-9]+ [SKIP] [WONTFIX] [SLOW] [<platform>] [<config>]
-              : <url> = <EXPCTATION>
-        The example is:
-                BUG123 BUG345 MAC : media/hoge.html
-                WONTFIX SKIP BUG19635 : media/restore-from-page-cache.html
-                    = TIMEOUT
-
-        Args:
-          line: a text for each line in the test expectation file.
-          previous_comment_info_list: a list of comments in previous lines.
-          test_case_patterns: a string for test case pattern.
-          media_test_cases_only: a boolean for media test case only.
-          writer: writer for intermediate results.
-        """
-        bugs = re.findall(r'BUG\w+', line)
-        testcases = re.search(r':\s+(\S+.[html|xml|svg|js])', line)
-        if testcases is not None:
-            testcase = testcases.group(1)
-            data = []
-            data.append(testcase)
-
-            if LayoutTestCaseManager.check_test_case_matches_pattern(
-                testcase, test_case_patterns):
-                data.append('Y')
-                media_test_case = True
-            else:
-                data.append('N')
-                media_test_case = False
-
-            dlink = self.generate_link_for_dashboard(testcase, False)
-            # Generate dashboard link.
-            data.append(
-                CsvUtils.generate_hyperlink_in_csv(dlink, 'Y', ''))
-            dlink = self.generate_link_for_dashboard(testcase, True)
-            # Generate dashboard link.
-            data.append(
-                CsvUtils.generate_hyperlink_in_csv(dlink, 'Y', ''))
-            for bug in bugs:
-                blink = self.generate_link_for_bug(bug)
-                data.append(
-                    CsvUtils.generate_hyperlink_in_csv(blink, bug, ''))
-            # Fill the gap for test case with less bug/test.
-            for i in range(2 - len(bugs)):
-                data.append('')
-
-            # Fill all data with 'N' (default).
-            for field_name in self.get_all_column_names(False, False):
-                if field_name in line:
-                    data.append('Y')
-                else:
-                    data.append('N')
-            # Comments will be accumulated in previous comments.
-            # This is for multiple-line comment.
-            for previous_comment_info in previous_comment_info_list:
-                data.append(previous_comment_info)
-            # Write only media related test case if specified.
-            if (media_test_case and media_test_cases_only
-                or (not media_test_cases_only)):
-                writer.writerow(data)
-                self.testcases.append(data)
-
-    def process_comments(self, comments):
-        """Process comments in the test expectations file.
-
-        Comments may contain special keywords such as UNIMPLEMENTED,
-        KNOWNISSUE, TESTISSUE (in TestExpectationsManager.COMMENT_COLUMN_NAMES)
-        and may be multiple lines.
-
-        Args:
-            comments: A raw comment from the test expectation file.
-               It is above test case name.
-
-        Returns:
-            A list of 'Y' or 'N' whether the comments contain
-            each column name in TestExpectationsManager.COMMENT_COLUMN_NAMES
-            in the comments.
-        """
-        return_list = []
-        for ccn in TestExpectationsManager.COMMENT_COLUMN_NAMES:
-            if ccn in comments:
-                return_list.append('Y')
-            else:
-                return_list.append('N')
-        return_list.append(comments)
-        return return_list
-
-    def get_and_save_content(self, location, output):
-        """Simply get test expectation from the specified location and save it.
-
-        Args:
-            location: SVN location of the test expectation file.
-            output: an output file path including file name.
-        """
-        client = pysvn.Client()
-        file_object = open(output, 'w')
-        file_object.write(client.cat(location))
-        file_object.close()
-
-    def get_and_save_content_media_only(self, location, output):
-        """Simply get test expectation from the specified location.
-
-        It also saves it (media only).
-
-        Args:
-            location: SVN location of the test expectation file.
-            output: an output file path including file name.
-        """
-        file_object = file(location, 'r')
-        text_list = list(file_object)
-        output_text = ''
-        for txt in text_list:
-            if txt.startswith('//'):
-                output_text += txt
-            else:
-                for pattern in (
-                    self.TEST_CASE_PATTERNS.get_test_case_pattern(
-                        'media')):
-                    if re.search(pattern, txt) is not None:
-                        output_text += txt
-                        break
-        file_object.close()
-        # Save output.
-        file_object = open(output, 'w')
-        file_object.write(output_text)
-        file_object.close()
-
-    def get_and_parse_content(self, location, output, media_test_case_only):
-        """Get test_expectations.txt and parse the content.
-
-        The comments are parsed as well since it contains keywords.
-
-        Args:
-            location: SVN location of the test expectation file.
-            output: an output file path including file name.
-            media_test_case_only: A boolean indicating whether media
-                test cases should only be processed.
-        """
-        if location.startswith('http'):
-            # Get from SVN.
-            client = pysvn.Client()
-            # Check out the current version of the pysvn project.
-            txt = client.cat(location)
-        else:
-            if location.endswith('.csv'):
-                # No parsing in the case of CSV file.
-                # Direct reading.
-                file_object = file(location, 'r')
-                self.testcases = list(csv.reader(file_object))
-                file_object.close()
-                return
-            file_object = open(location, 'r')
-            txt = file_object.read()
-            file_object.close()
-
-        file_object = file(output, 'wb')
-        writer = csv.writer(file_object)
-        writer.writerow(self.get_all_column_names(True, True))
-        lines = txt.split('\n')
-        process = False
-        previous_comment = ''
-        previous_comment_list = []
-        for line in lines:
-            if line.isspace() or str is '':
-                continue
-            line.strip()
-            if line.startswith('//'):
-                # There are comments.
-                line = line.replace('//', '')
-                if process is True:
-                    previous_comment = line
-                else:
-                    previous_comment = previous_comment + '\n' + line
-                previous_comment_list = self.process_comments(previous_comment)
-                process = False
-            else:
-                self.parse_line(
-                    line, previous_comment_list,
-                    self.TEST_CASE_PATTERNS.get_test_case_pattern('media'),
-                    media_test_case_only, writer)
-                process = True
-        file_object.close()
-
-    def get_te_diff_between_times(self, te_location, start, end, patterns,
-                                  change, checkchange):
-        """Get test expectation diff output for a given time period.
-
-        Args:
-            te_location: SVN location of the test expectation file.
-            start: a date object for the start of the time period.
-            end: a date object for the end of the time period.
-            patterns: test case name patterns of the test cases that
-                we are interested in. This could be test case name
-                as it is in the case of exact matching.
-            change: the number of change in occurrence of test case in
-                test expectation.
-            checkchange: a boolean to indicate if we want to check the
-                change in occurrence as specified in |change| argument.
-
-        Returns:
-            a list of tuples (old_revision, new_revision,
-                diff_line, author, date, commit_message) that matches
-                the condition.
-        """
-        client = pysvn.Client()
-        client.checkout(te_location, 'tmp', recurse=False)
-        logs = client.log('tmp/test_expectations.txt',
-                          revision_start=pysvn.Revision(
-                              pysvn.opt_revision_kind.date, start),
-                          revision_end=pysvn.Revision(
-                              pysvn.opt_revision_kind.date, end))
-        result_list = []
-        for i in xrange(len(logs) - 1):
-            # PySVN.log returns logs in reverse chronological order.
-            new_rev = logs[i].revision.number
-            old_rev = logs[i + 1].revision.number
-            # Getting information about new revision.
-            author = logs[i].author
-            date = logs[i].date
-            message = logs[i].message
-            text = client.diff('/tmp', 'tmp/test_expectations.txt',
-                               revision1=pysvn.Revision(
-                                   pysvn.opt_revision_kind.number, old_rev),
-                               revision2=pysvn.Revision(
-                                   pysvn.opt_revision_kind.number, new_rev))
-            lines = text.split('\n')
-            for line in lines:
-                for pattern in patterns:
-                    matches = re.findall(pattern, line)
-                    if matches:
-                        if checkchange:
-                            if ((line[0] == '+' and change > 0) or
-                                (line[0] == '-' and change < 0)):
-                                result_list.append((old_rev, new_rev, line,
-                                                    author, date, message))
-                        else:
-                            if line[0] == '+' or line[0] == '-':
-                                result_list.append((old_rev, new_rev, line,
-                                                    author, date, message))
-        return result_list
+    Returns:
+      a map containing the test expectation entries an and comments and bugs.
+    """
+    test_expectation_info = {}
+    for name in TestExpectations.GetAllDataNames():
+      if not line:

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

Isn't this guarded against before calling this function?
(even if it wasn't, you wouldn't want to be checking this inside the for-name
loop)

[imasaki1, 2011/08/20 08:25:36]

Deleted

+        # Clear the comments if there is space (this is typical the case).

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

s/typical/typically/

[imasaki1, 2011/08/20 08:25:36]

Done.

+        comments = ''
+      if name in line:
+        test_expectation_info[name] = True

[Ami GONE FROM CHROMIUM, 2011/08/20 04:16:50]

This is going to incorrectly count names appearing in comments as being
non-comments.
I think you need to strip off comments *first* in this function, and append them
to comments.

[imasaki1, 2011/08/20 08:25:36]

Comments are dealt in __init__ function. So, the line here should not be
comments.

[Ami GONE FROM CHROMIUM, 2011/08/20 17:52:23]

I was referring to line-ending comments, not multi-line ones.
They're apparently rare but exi$ grep // test_expectations.txt |grep -v '^//'
BUGCR30536 SLOW LINUX WIN : http/tests/misc/acid3.html = FAIL // See "SVG Tests"
section too
BUGWK54322 SNOWLEOPARD : http/tests/misc/acid3.html = IMAGE // See "SVG tests"
section too

[imasaki1, 2011/08/20 18:07:38]

Modified... Added unit test for this.

+    # Store comments.
+    test_expectation_info['Comments'] = comments
+    # Store bugs.
+    bugs = re.findall(r'BUG\w+', line)
+    if len(bugs) > 0:
+      test_expectation_info['Bugs'] = bugs
+    return test_expectation_info