framework: refactor subtest handling

The old design of TestResult and TestrunResult has two related limitations
that prevent us from relaxing process isolation more than what is done for
shader tests:

1. A TestResult can only have subtests one level below its 'root' group.

2. If one TestResult with subtests covers a group, then no other
   TestResult can have tests in the same group.

This patch relaxes both constraints in the following way:

- The conceptual specialness of subtests is reduced. A TestResult is
  simply a container for one or more tests that happen to have been
  tested as part of the same execution (so the tests share pid and
  stdout/stderr).

- TestResult now has a 'root' attribute. Without subtests, this is just
  the name of the test. With subtests, it is the common prefix of all
  subtests. Subtest names are relative to the root, and can be multiple
  levels deep.

- TestrunResult stores a plain list of TestResults in the 'results'
  attribute. For test lookups, the '_tests' dictionary caches the mapping
  of tests to result object.

The users of those classes are adjusted accordingly, especially for
summary generation.

This patch does not yet change the on-disk format of results.

TODO:
- unit tests need to be adjusted
- junit backend needs to be adjusted

6 files changed, 130 insertions, 69 deletions

diff --git a/framework/grouptools.py b/framework/grouptools.py
index f28241d3c..2ee74ed9f 100644
--- a/framework/grouptools.py
+++ b/framework/grouptools.py
@@ -153,6 +153,24 @@ def split(group):
     return group.split(SEPARATOR)
 
 
+def relative(root, test):
+    """Return the subgroup/subtest path of test relative to root.
+
+    Satisfies join(root, relative(root, test)) == test.
+
+    If root == test, return ''.
+
+    """
+    assert test.startswith(root), (test, root)
+    if test == root:
+        return ''
+
+    assert test[len(root)] == SEPARATOR
+    assert len(test) >= len(root) + 2
+
+    return test[len(root) + 1:]
+
+
 def from_path(path):
     """Create a group from a path.
 
diff --git a/framework/results.py b/framework/results.py
index 99dd3735b..a44029f3e 100644
--- a/framework/results.py
+++ b/framework/results.py
@@ -148,11 +148,12 @@ class TestResult(object):
     """An object represting the result of a single test."""
     __slots__ = ['returncode', '_err', '_out', 'time', 'command', 'traceback',
                  'environment', 'subtests', 'dmesg', '__result', 'images',
-                 'exception', 'pid']
+                 'exception', 'pid', 'root']
     err = StringDescriptor('_err')
     out = StringDescriptor('_out')
 
     def __init__(self, result=None):
+        self.root = ''
         self.returncode = None
         self.time = TimeAttribute()
         self.command = str()
@@ -190,6 +191,24 @@ class TestResult(object):
         except exceptions.PiglitInternalError as e:
             raise exceptions.PiglitFatalError(str(e))
 
+    def get_result(self, key):
+        """Return the result of a (sub-)test covered by this object.
+
+        KeyError is raised if the test key is not covered.
+
+        """
+        relative = grouptools.relative(self.root, key)
+
+        if relative == '':
+            if self.subtests:
+                raise KeyError(key)
+            return self.result
+
+        try:
+            return self.subtests[relative]
+        except KeyError:
+            raise KeyError(key)
+
     def to_json(self):
         """Return the TestResult as a json serializable object."""
         obj = {
@@ -275,6 +294,10 @@ class Totals(dict):
                 return True
         return False
 
+    def add(self, other):
+        for each in status.ALL:
+            self[each] += other[each]
+
     def to_json(self):
         """Convert totals to a json object."""
         result = copy.copy(self)
@@ -301,57 +324,76 @@ class TestrunResult(object):
         self.clinfo = None
         self.lspci = None
         self.time_elapsed = TimeAttribute()
-        self.tests = collections.OrderedDict()
+        self.results = []
+        self._tests = {}
         self.totals = collections.defaultdict(Totals)
 
+    def get_testresult(self, key):
+        """Get the TestResult object that contains the test.
+
+        If the test does not exist, return None.
+
+        The returned object may contain the given test only as a subtest.
+
+        Arguments:
+        key -- the key name of the test whose TestResult object is returned
+
+        """
+        return self._tests.get(key)
+
     def get_result(self, key):
-        """Get the result of a test or subtest.
+        """Get the result of a test.
 
-        If neither a test nor a subtest instance exist, then raise the original
-        KeyError generated from looking up <key> in the tests attribute. It is
-        the job of the caller to handle this error.
+        KeyError is raised when the test does not exist.
 
         Arguments:
         key -- the key name of the test to return
 
         """
-        try:
-            return self.tests[key].result
-        except KeyError as e:
-            name, test = grouptools.splitname(key)
-            try:
-                return self.tests[name].subtests[test]
-            except KeyError:
-                raise e
+        return self._tests[key].get_result(key)
+
+    @property
+    def testnames(self):
+        """Return an iterable of all test names.
+        """
+        return six.iterkeys(self._tests)
 
     def calculate_group_totals(self):
         """Calculate the number of passes, fails, etc at each level."""
-        for name, result in six.iteritems(self.tests):
+        worklist = collections.defaultdict(Totals)
+
+        for result in self.results:
             # If there are subtests treat the test as if it is a group instead
             # of a test.
             if result.subtests:
-                for res in six.itervalues(result.subtests):
+                for subtest, res in six.iteritems(result.subtests):
                     res = str(res)
-                    temp = name
-
-                    self.totals[temp][res] += 1
-                    while temp:
-                        temp = grouptools.groupname(temp)
-                        self.totals[temp][res] += 1
-                    self.totals['root'][res] += 1
+                    name = grouptools.groupname(grouptools.join(result.root, subtest))
+                    worklist[name][res] += 1
             else:
                 res = str(result.result)
-                while name:
+                name = grouptools.groupname(result.root)
+                worklist[name][res] += 1
+
+        while worklist:
+            # Sorting the keys in reverse orders means that sub-groups appear
+            # before their parent group.
+            for name in sorted(six.iterkeys(worklist), reverse=True):
+                total = worklist.pop(name)
+
+                if name:
+                    self.totals[name].add(total)
                     name = grouptools.groupname(name)
-                    self.totals[name][res] += 1
-                self.totals['root'][res] += 1
+                    worklist[name].add(total)
+                else:
+                    self.totals['root'].add(total)
 
     def to_json(self):
         if not self.totals:
             self.calculate_group_totals()
         rep = copy.copy(self.__dict__)
-        rep['tests'] = collections.OrderedDict((k, t.to_json())
-                       for k, t in six.iteritems(self.tests))
+        rep['tests'] = collections.OrderedDict((t.root, t.to_json())
+                       for t in self.results)
         rep['__type__'] = 'TestrunResult'
         return rep
 
@@ -379,8 +421,22 @@ class TestrunResult(object):
         if 'time_elapsed' in dict_:
             setattr(res, 'time_elapsed',
                     TimeAttribute.from_dict(dict_['time_elapsed']))
-        res.tests = collections.OrderedDict((n, TestResult.from_dict(t))
-                    for n, t in six.iteritems(dict_['tests']))
+
+        for root, result_dict in six.iteritems(dict_['tests']):
+            result = TestResult.from_dict(result_dict)
+            result.root = root
+            res.results.append(result)
+
+            if result.subtests:
+                for subtest in six.iterkeys(result.subtests):
+                    fullname = grouptools.join(root, subtest)
+                    assert fullname not in res._tests
+                    res._tests[fullname] = result
+            else:
+                if result.root in res._tests:
+                    # This can happen with old resumed test results.
+                    print('Warning: duplicate results for {}'.format(result.root))
+                res._tests[result.root] = result
 
         if not 'totals' in dict_ and not _no_totals:
             res.calculate_group_totals()
diff --git a/framework/summary/common.py b/framework/summary/common.py
index b5e38b4f1..594eedc45 100644
--- a/framework/summary/common.py
+++ b/framework/summary/common.py
@@ -97,12 +97,7 @@ class Names(object):
         """A set of all tests in all runs."""
         all_ = set()
         for res in self.__results:
-            for key, value in six.iteritems(res.tests):
-                if not value.subtests:
-                    all_.add(key)
-                else:
-                    for subt in six.iterkeys(value.subtests):
-                        all_.add(grouptools.join(key, subt))
+            all_.update(res.testnames)
         return all_
 
     @lazy_property
diff --git a/framework/summary/html_.py b/framework/summary/html_.py
index 512b42c24..a269ea802 100644
--- a/framework/summary/html_.py
+++ b/framework/summary/html_.py
@@ -41,7 +41,7 @@ import six
 # the module
 from framework import backends, exceptions, core
 
-from .common import Results, escape_filename, escape_pathname
+from .common import Results, escape_pathname
 from .feature import FeatResults
 
 __all__ = [
@@ -77,7 +77,7 @@ def _copy_static_files(destination):
                 os.path.join(destination, "result.css"))
 
 
-def _make_testrun_info(results, destination, exclude=None):
+def _make_testrun_info(results, destination, result_html_map, exclude=None):
     """Create the pages for each results file."""
     exclude = exclude or {}
     result_css = os.path.join(destination, "result.css")
@@ -106,11 +106,14 @@ def _make_testrun_info(results, destination, exclude=None):
                 lspci=each.lspci))
 
         # Then build the individual test results
-        for key, value in six.iteritems(each.tests):
-            html_path = os.path.join(destination, name,
-                                     escape_filename(key + ".html"))
+        for idx, value in enumerate(each.results):
+            html_relative_path = os.path.join(name,
+                                     "{}.html".format(idx))
+            html_path = os.path.join(destination, html_relative_path)
             temp_path = os.path.dirname(html_path)
 
+            result_html_map[value] = html_relative_path
+
             if value.result not in exclude:
                 core.check_dir(temp_path)
 
@@ -118,7 +121,6 @@ def _make_testrun_info(results, destination, exclude=None):
                     with open(html_path, 'wb') as out:
                         out.write(_TEMPLATES.get_template(
                             'test_result.mako').render(
-                                testname=key,
                                 value=value,
                                 css=os.path.relpath(result_css, temp_path),
                                 index=os.path.relpath(index, temp_path)))
@@ -126,7 +128,7 @@ def _make_testrun_info(results, destination, exclude=None):
                     traceback.print_exc()
 
 
-def _make_comparison_pages(results, destination, exclude):
+def _make_comparison_pages(results, destination, result_html_map, exclude):
     """Create the pages of comparisons."""
     pages = frozenset(['changes', 'problems', 'skips', 'fixes',
                        'regressions', 'enabled', 'disabled'])
@@ -137,6 +139,7 @@ def _make_comparison_pages(results, destination, exclude):
     with open(os.path.join(destination, "index.html"), 'wb') as out:
         out.write(_TEMPLATES.get_template('index.mako').render(
             results=results,
+            result_html_map=result_html_map,
             page='all',
             pages=pages,
             exclude=exclude))
@@ -148,6 +151,7 @@ def _make_comparison_pages(results, destination, exclude):
             if sum(getattr(results.counts, page)) > 0:
                 out.write(_TEMPLATES.get_template('index.mako').render(
                     results=results,
+                    result_html_map=result_html_map,
                     pages=pages,
                     page=page,
                     exclude=exclude))
@@ -179,8 +183,9 @@ def html(results, destination, exclude):
     results = Results([backends.load(i) for i in results])
 
     _copy_static_files(destination)
-    _make_testrun_info(results, destination, exclude)
-    _make_comparison_pages(results, destination, exclude)
+    result_html_map = {}
+    _make_testrun_info(results, destination, result_html_map, exclude)
+    _make_comparison_pages(results, destination, result_html_map, exclude)
 
 
 def feat(results, destination, feat_desc):
@@ -189,5 +194,6 @@ def feat(results, destination, feat_desc):
     feat_res = FeatResults([backends.load(i) for i in results], feat_desc)
 
     _copy_static_files(destination)
-    _make_testrun_info(feat_res, destination)
+    result_html_map = {}
+    _make_testrun_info(feat_res, result_html_map, destination)
     _make_feature_info(feat_res, destination)
diff --git a/templates/index.mako b/templates/index.mako
index 49a8b1c86..1f263ab42 100644
--- a/templates/index.mako
+++ b/templates/index.mako
@@ -42,11 +42,6 @@
       return '{}/{}'.format(num, den)
 
 
-  def escape_filename(key):
-      """Avoid reserved characters in filenames."""
-      return re.sub(r'[<>:"|?*#]', '_', key)
-
-
   def escape_pathname(key):
       """ Remove / and \\ from names """
       return re.sub(r'[/\\]', '_', key)
@@ -145,7 +140,7 @@
             </tr><tr>
           % endfor
         % endif
-        
+
         <td>
           <div class="group" style="margin-left: ${depth * 1.75}em">
             ${grouptools.testname(test)}
@@ -153,30 +148,21 @@
         </td>
         % for res in results.results:
           <%
-            # Get the raw result, if it's none check to see if it's a subtest, if that's still None
-            # then declare it not run
+            # Get the raw result
             # This very intentionally uses posix path, we're generating urls, and while
             # some windows based browsers support \\ as a url separator, *nix systems do not,
             # which would make a result generated on windows non-portable
-            raw = res.tests.get(test)
+            raw = res.get_testresult(test)
             if raw is not None:
-              result = raw.result
-              href = normalize_href(posixpath.join(escape_pathname(res.name),
-                                                   escape_filename(test)))
+              result = raw.get_result(test)
+              href = normalize_href(result_html_map[raw])
             else:
-              raw = res.tests.get(grouptools.groupname(test))
-              name = grouptools.testname(test)
-              if raw is not None and name in raw.subtests:
-                result = raw.subtests[name]
-                href = normalize_href(posixpath.join(escape_pathname(res.name),
-                                                     escape_filename(grouptools.groupname(test))))
-              else:
-                result = status.NOTRUN
+              result = status.NOTRUN
             del raw  # we don't need this, so don't let it leak
           %>
           <td class="${str(result)}">
           % if str(result) not in exclude and result is not status.NOTRUN:
-            <a href="${href}.html">
+            <a href="${href}">
               ${str(result)}
             </a>
           % else:
diff --git a/templates/test_result.mako b/templates/test_result.mako
index ff08797bc..7cd70891e 100644
--- a/templates/test_result.mako
+++ b/templates/test_result.mako
@@ -4,11 +4,11 @@
 <html xmlns="http://www.w3.org/1999/xhtml">
   <head>
     <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
-    <title>${testname} - Details</title>
+    <title>${value.root} - Details</title>
     <link rel="stylesheet" href="${css}" type="text/css" />
   </head>
   <body>
-    <h1>Results for ${testname}</h1>
+    <h1>Results for ${value.root}</h1>
     <h2>Overview</h2>
     <div>
       <p><b>Result:</b> ${value.result}</p>