Address review + limit each builder to 70 displayed "build dots"

Author: encukou

master/custom/release_dashboard.py

@@ -33,7 +33,7 @@
 
 
 def _gimme_error(func):
-    """Decorator to turn AttributeError into a different Exception
+    """Debug decorator to turn AttributeError into a different Exception
 
     jinja2 tends to swallow AttributeError or report it in some place it
     didn't happen. When that's a problem, use this decorator to get
@@ -43,9 +43,11 @@ def decorated(*args, **kwargs):
         try:
             return func(*args, **kwargs)
         except AttributeError as e:
-            raise Exception(f'your error: {e!r}')
+            raise _WrappedAttributeError(f'your error: {e!r}')
     return decorated
 
+class _WrappedAttributeError(Exception): pass
+
 
 class DashboardObject:
     """Base wrapper for a dashboard object.
@@ -307,13 +309,10 @@ def problems(self):
 
     @cached_property
     def featured_problem(self):
-       try:
         try:
             return self.problems[0]
         except IndexError:
             return NoProblem()
-       except AttributeError:
-           raise SystemError
 
     def get_grouped_problems(self):
         def key(problem):
@@ -411,18 +410,35 @@ def css_color_class(self):
 
     @cached_property
     def junit_results(self):
-        filepath = (
-            self._root._app.test_result_dir
-            / self.builder.branch.tag
-            / self.builder["name"]
-            / f'build_{self["number"]}.xml'
-        )
+        if not self._root._app.test_result_dir:
+            return None
+
         try:
-            file = filepath.open()
-        except OSError:
+            filepath = (
+                self._root._app.test_result_dir
+                / self.builder.branch.tag
+                / self.builder["name"]
+                / f'build_{self["number"]}.xml'
+            ).resolve()
+
+            # Ensure path doesn't escape test_result_dir
+            if not filepath.is_relative_to(self._root._app.test_result_dir):
+                return None
+
+            if not filepath.is_file():
+                return None
+
+            with filepath.open() as file:
+                etree = ElementTree.parse(file)
+
+        # We don't have a logger set up, this returns None on common failures
+        # (meaning failures won't show on the dashboard).
+        # TODO: set up monitoring and log failures (in the whole method).
+        except OSError as e:
+            return None
+        except ElementTree.ParseError as e:
             return None
-        with file:
-            etree = ElementTree.parse(file)
+
         result = JunitResult(self, {})
         for element in etree.iterfind('.//error/..'):
             result.add(element)
@@ -449,6 +465,22 @@ def __init__(self, *args):
         self.error_types = set()
 
     def add(self, element):
+        """Add errors from a XML element.
+
+        JunitResult are arranged in a tree, grouped by test modules, classes
+        and methods (i.e. dot-separated parts of the test name).
+
+        JunitError instances are added to the lowest level of the tree.
+        They're deduplicated, because we re-run failing tests and often
+        get two copies of the same error (with the same traceback).
+
+        Exception type names are added to *all* levels of the tree:
+        if the details of a test module/class/methods aren't expanded,
+        the dashboard shows exception types from all the hidden failures.
+        """
+        # Gather all the errors (as dicts), and their exception types
+        # (as strings), from *element*.
+        # Usually there's only one error per element.
         errors = []
         error_types = set()
         for error_elem in element.iterfind('error'):
@@ -458,13 +490,18 @@ def add(self, element):
             })
             errors.append(new_error)
             error_types.add(new_error["type"])
+
+        # Find/add the leaf JunitResult, updating result.error_types for each
+        # Result along the way
         result = self
         name_parts = element.attrib.get('name', '??').split('.')
         if name_parts[0] == 'test':
             name_parts.pop(0)
         for part in name_parts:
             result.error_types.update(error_types)
             result = result.contents.setdefault(part, JunitResult(self, {}))
+
+        # Add error details to the leaf
         result.error_types.update(error_types)
         for error in errors:
             if error not in result.errors:
@@ -665,7 +702,7 @@ def __init__(self, test_result_dir=None):
         self.flask_app.jinja_env.add_extension('jinja2.ext.loopcontrols')
         self.flask_app.jinja_env.undefined = jinja2.StrictUndefined
 
-        self.test_result_dir = Path(test_result_dir)
+        self.test_result_dir = Path(test_result_dir).resolve()
 
         @self.flask_app.route('/')
         @self.flask_app.route("/index.html")

master/custom/templates/releasedashboard.html

@@ -196,7 +196,7 @@ <h5>
                                         </div>
                                     {% endif %}
                                     <div class="build-dots">
-                                        {% for build in builder.builds %}
+                                        {% for build in builder.builds[:70] %}
                                             {{ build_dot(build) }}
                                         {% endfor %}
                                     </div>