[LNT] Missing "Produced by" field on Run causes UI to crash

[leandron]

Summary

When a given run contains a "producer" value (usually an URL pointing to build systems), the run details page can crash when displaying this run in cases when the previous or baseline runs do not contain a valid value for the "producer" field.

It is possible to see the following error message:

...
File "/sandbox/lnt/server/ui/filters.py", line 47, in filter_producerAsHTML
  if not producer:
jinja2.exceptions.UndefinedError: 'dict object' has no attribute 'producer'

This change prevents the UI to crash when "producer" is not found. It also adds a test change to capture this situation.

[kristof.beyls]

tests/server/ui/V4Pages.py

291–292 ↗

(On Diff #105229)

Ideally, the test would also check that the outputted producer information is as expected, not just that the HTML return code is as expected.
You can find examples of how that's done in this file; just look for how check_nr_machines_reported is used & defined. check_table_content is another example.

Do you think it would be possible to improve the test case like this without too much effort?

[cmatthews]

LGTM, besides Kristof's point about checking the output in the test.

[leandron]

• Improve testing output to check the generated labels
• Small change to escape a bare & that crashes xml.etree.ElementTree
• sync with current SVN rev.

[leandron]

Adding context.

[kristof.beyls]

Mostly looks good, just 2 minor remarks.

tests/server/ui/V4Pages.py

143 ↗

(On Diff #106188)

I was confused by the name "check_table_row_content", not sure what this function was doing based on the name.
After reading the implementation, I guess "check_row_is_in_table" is a better name?

168 ↗

(On Diff #106188)

Isn't table_header always "Produced by" for check_producer_label?
If so, wouldn't it be better to make the signature of check_producer_label(client, url, label)?

[leandron]

I'll submit a new version including these fixes.

tests/server/ui/V4Pages.py

143 ↗

(On Diff #106188)

Yes. I'll rename.

168 ↗

(On Diff #106188)

Makes sense. I'll change.

[leandron]

Applying code review suggestions.

[kristof.beyls]

LGTM!

lnt/trunk/lnt/server/ui/templates/reporting/runs.html

<table style="{{ styles['table'] }}" class="{{ classes['table'] }}">
   {% for title, r in (('Current', run), ('Previous', compare_to), ('Baseline', baseline)) %}
     {% if r %}
     <tr id="run-table-{{ title }}">
       <td style="{{ styles['td'] }}"><a href="{{ ts_url }}/{{ r.id }}">{{ title }}</a></td>
       <td style="{{ styles['td'] }}"><a href="{{ ts_url }}/order/{{ r.order.id }}">{{ r.order.llvm_project_revision }}</a></td>
       <td style="{{ styles['td'] }}"><span class="utctime">{{ r.start_time.isoformat() }}</span></td>
       <td style="{{ styles['td'] }}">{{ r.end_time - r.start_time }}</td>
       {% if show_producers %}
-        <td style="{{ styles['td'] }}">{{ r.parameters.producer|producerAsHTML }}</td>
+        <td style="{{ styles['td'] }}">
+          {{ r.parameters.producer|producerAsHTML|safe if 'producer' in r.parameters else "" }}
+        </td>
       {% endif %}
     </tr>
     {% else %}
     <tr id="run-table-{{ title }}">
       <td style="{{ styles['td'] }}" colspan="5">No {{ title }} Run</td>
     </tr>
     {% endif %}
   {% endfor %}

lnt/trunk/lnt/server/ui/util.py

 def renderProducerAsHTML(producer):
     # If the string looks like a buildbot link, render it prettily.
     m = re.match(r'http://(.*)/builders/(.*)/builds/(\d+)', producer)
     if m:
         url = m.group(1)
         builder = m.group(2)
         build = m.group(3)
 
-        png_url = 'http://%(url)s/png?builder=%(builder)s&number=%(build)s' % locals()
-        img = '<img src="%(png_url)s">' % locals()
+        png_url = 'http://%(url)s/png?builder=%(builder)s&amp;number=%(build)s' % locals()
+        img = '<img src="%(png_url)s" />' % locals()
         return '<a href="%(producer)s">%(builder)s #%(build)s %(img)s</a>' % locals()
 
     elif producer.startswith('http://'):
         return '<a href="' + producer + '">Producer</a>'
 
     else:
         return producer
 

lnt/trunk/tests/server/ui/Inputs/V4Pages_extra_records.sql

 -- check that a regression on consecutive runs more than 1 day apart can be detected:
 INSERT INTO "NT_Test" VALUES(4,'test1'); -- ID 4
 INSERT INTO "NT_Test" VALUES(5,'test2'); -- ID 5
 INSERT INTO "NT_Order" ("NextOrder", "PreviousOrder", "llvm_project_revision")
  VALUES(NULL,NULL,'152292'); -- ID 5
 INSERT INTO "NT_Run" ("MachineID", "OrderID", "ImportedFrom", "StartTime",
                       "EndTime", "SimpleRunID", "Parameters")
  VALUES(2,5,'run5.json','2012-05-01 16:28:23.000000',
-        '2012-05-01 16:28:58.000000',NULL,'[]'); -- ID 5
+        '2012-05-01 16:28:58.000000',NULL,
+        '[["producer", "http://buildbot.server.url/builders/some-builder/builds/987"]]'); -- ID 5
 INSERT INTO "NT_Sample" ("RunID", "TestID", "compile_status",
                          "execution_status", "compile_time", "execution_time",
                          "score", "mem_bytes")
  VALUES(5,4,0,0,0.001,1.0,NULL,NULL); -- ID 5: passing result
 INSERT INTO "NT_Sample" ("RunID", "TestID", "compile_status",
                          "execution_status", "compile_time", "execution_time",
                          "score", "mem_bytes")
  VALUES(5,5,0,1,0.001,1.0,NULL,NULL); -- ID 6: failing result

  VALUES(NULL,NULL,'152294'); -- ID 6
 INSERT INTO "NT_Order" ("NextOrder", "PreviousOrder", "llvm_project_revision")
  VALUES(NULL,NULL,'152295'); -- ID 7
 INSERT INTO "NT_Order" ("NextOrder", "PreviousOrder", "llvm_project_revision")
  VALUES(NULL,NULL,'152296'); -- ID 8
 INSERT INTO "NT_Run" ("MachineID", "OrderID", "ImportedFrom", "StartTime",
                       "EndTime", "SimpleRunID", "Parameters")
  VALUES(2,6,'run7.json','2012-05-10 16:28:23.000000',
-        '2012-05-10 16:28:58.000000',NULL,'[]'); -- ID 7
+        '2012-05-10 16:28:58.000000',NULL,
+        '[["producer", "http://my.build.server/buildResult"]]'); -- ID 7
 INSERT INTO "NT_Run" ("MachineID", "OrderID", "ImportedFrom", "StartTime",
                       "EndTime", "SimpleRunID", "Parameters")
  VALUES(2,7,'run8.json','2012-05-11 16:28:23.000000',
         '2012-05-11 16:28:58.000000',NULL,'[]'); -- ID 8
 INSERT INTO "NT_Run" ("MachineID", "OrderID", "ImportedFrom", "StartTime",
                       "EndTime", "SimpleRunID", "Parameters")
  VALUES(2,8,'run9.json','2012-05-12 16:28:23.000000',
         '2012-05-12 16:28:58.000000',NULL,'[]'); -- ID 9

lnt/trunk/tests/server/ui/V4Pages.py

try:
         parser.entity.update((x, unichr(i)) for x, i in name2codepoint.iteritems())
         tree = ET.fromstring(html_string, parser=parser)
     except:
        dump_html(html_string)
        raise
     return tree
 
 
+def find_table_by_thead_content(tree, table_head_contents):
+    all_tables = tree.findall(".//thead/..")
+    found_header = False
+    for table in all_tables:
+        for child in table.findall('./thead/tr/th'):
+            if child.text == table_head_contents:
+                return table
+    return None
+
+
 def find_table_with_heading(tree, table_heading):
     table_parent_elements = tree.findall(".//table/..")
     found_header = False
     for parent in table_parent_elements:
         for child in parent.findall('*'):
             if found_header:
                 if child.tag == "table":
                     return child
def get_table_by_header(client, url, table_header):
     return table
 
 
 def get_results_table(client, url, fieldname):
     table_header = "Result Table (%s)" % fieldname
     return get_table_by_header(client, url, table_header)
 
 
-def check_table_content(table, expected_content):
-    body_content = [[convert_html_to_text(cell).strip()
-                     for cell in row.findall("./td")]
-                    for row in table.findall("./tbody/tr")]
+def get_table_body_content(table):
+    return [[convert_html_to_text(cell).strip()
+
+
+def check_row_is_in_table(table, expected_row_content):
+    body_content = get_table_body_content(table)
+    assert expected_row_content in body_content, \
+        "Expected row content %s not found in %s" % \
+        (expected_row_content, body_content)
+
+
+def check_table_content(table, expected_content):
+    body_content = get_table_body_content(table)
     assert expected_content == body_content, \
         "Expected table content %s, found %s" % \
         (expected_content, body_content)
 
 
 def check_body_result_table(client, url, fieldname, expected_content):
     table = get_results_table(client, url, fieldname)
     check_table_content(table, expected_content)
 
 
 def check_body_nr_tests_table(client, url, expected_content):
     table_header = "Number of Tests Seen"
     table = get_table_by_header(client, url, table_header)
     check_table_content(table, expected_content)
 
 
+def check_producer_label(client, url, label):
+    table_header = "Produced by"
+    resp = check_code(client, url)
+    tree = get_xml_tree(resp.data)
+    table = find_table_by_thead_content(tree, table_header)
+    check_row_is_in_table(table, label)
+
+
 def get_sparkline(table, testname, machinename):
     body_content = [[cell
                      for cell in row.findall("./td")]
                     for row in table.findall("./tbody/tr")]
     txt_body_content = [[convert_html_to_text(cell).strip()
                          for cell in row.findall("./td")]
                         for row in table.findall("./tbody/tr")]
     cur_test_name = ""
check_code(client, '/v4/nts/9999/report',
                expected_code=HTTP_NOT_FOUND)
     check_code(client, '/v4/nts/9999/text_report',
                expected_code=HTTP_NOT_FOUND)
 
     # Get a graph page. This has been changed to redirect.
     check_redirect(client, '/v4/nts/1/graph?test.3=2',
                    'v4/nts/graph\?plot\.0=1\.3\.2&highlight_run=1$')
 
+    # Get a run that contains generic producer information
+    check_producer_label(client, '/v4/nts/7',
+                         ['Current', '152293', '2012-05-10T16:28:23',
+                          '0:00:35', 'Producer'])
+
+    # Get a run that contains Buildbot producer information
+    check_producer_label(client, '/v4/nts/7',
+                         ['Previous', '152292', '2012-05-01T16:28:23',
+                          '0:00:35', 'some-builder #987'])
+
     # Get the new graph page.
     check_code(client, '/v4/nts/graph?plot.0=1.3.2')
     # Don't crash when requesting non-existing data
     check_code(client, '/v4/nts/graph?plot.9999=1.3.2')
     check_code(client, '/v4/nts/graph?plot.0=9999.3.2',
                expected_code=HTTP_NOT_FOUND)
     check_code(client, '/v4/nts/graph?plot.0=1.9999.2',
                expected_code=HTTP_NOT_FOUND)