Fix Talos TinderboxPrint output so TBPL's MachineResult.js::_getTalosResults doesn't have to un-mangle it

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
(Phil Ringnalda (:philor) from bug 790963 comment #6)
> I really think the right thing to do is to fix Talos (have you looked at the
> graphserver URLs we get for the suites where we get both a link for the
> number, and a link for "(details)"? and the reason that we have this special
> FAIL: processing, to strip out a TinderboxPrint:<br>? and the way the
> useless repeated details links come in a <p style="font-size:smaller;">?),
> but I don't think that we'll lose anything by not using the generic
> _getScrapeResults even when there's no graphserver URL, because I don't
> think there's anything interesting TinderboxPrinted in those cases.

eg:

{
Completed test tresize: 
		Stopped Thu, 27 Sep 2012 01:05:51
RETURN: cycle time: 00:18:15<br>
talos-r3-xp-051: 
		Stopped Thu, 27 Sep 2012 01:05:51
NOISE: Outputting talos results => {'results_urls': ['http://graphs.mozilla.org/server/collect.cgi'], 'datazilla_urls': ['https://datazilla.mozilla.org/talos']}
Generating results file: tdhtmlr: 
		Started Thu, 27 Sep 2012 01:05:51
Generating results file: tdhtmlr: 
		Stopped Thu, 27 Sep 2012 01:05:51
Generating results file: tresize: 
		Started Thu, 27 Sep 2012 01:05:51
Generating results file: tresize: 
		Stopped Thu, 27 Sep 2012 01:05:51
NOISE: Posting result 0 of 2 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 1 of 2 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
RETURN:<br>
RETURN:<a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint: 619.03</a><br>
RETURN:<a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize: 13.0</a><br>
RETURN:<p style="font-size:smaller;">Details:<br>| <a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint</a> | <a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize</a> |</p>
NOISE: Outputting datazilla results to https://datazilla.mozilla.org/talos
NOISE: datazilla: https//datazilla.mozilla.org/talos; oauth=False
program finished with exit code 0
elapsedTime=1099.938000
TinderboxPrint:<a href = "http://hg.mozilla.org/integration/mozilla-inbound/rev/157f3efdee37">rev:157f3efdee37</a>

TinderboxPrint: cycle time: 00:18:15<br>

TinderboxPrint:<br>

TinderboxPrint:<a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint: 619.03</a><br>

TinderboxPrint:<a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize: 13.0</a><br>

TinderboxPrint:<p style="font-size:smaller;">Details:<br>| <a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint</a> | <a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize</a> |</p>
}
(Assignee)

Comment 1

6 years ago
Output of https://tbpl.mozilla.org/php/getLogExcerpt.php?id=15584544&type=tinderbox_print:
{
 s: talos-r3-xp-051
<a href = "http://hg.mozilla.org/integration/mozilla-inbound/rev/157f3efdee37">rev:157f3efdee37</a>
 cycle time: 00:18:15<br>
<br>
<a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint: 619.03</a><br>
<a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize: 13.0</a><br>
<p style="font-size:smaller;">Details:<br>| <a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint</a> | <a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize</a> |</p>
}
(Assignee)

Comment 2

6 years ago
Example failure case:
{
Traceback (most recent call last):
  File "run_tests.py", line 250, in run_tests
getting files in '/mnt/sdcard/tests/profile/minidumps/'
Failed tp4m: 
		Stopped Thu, 27 Sep 2012 01:09:59
    talos_results.add(mytest.runTest(browser_config, test))
  File "/builds/tegra-259/talos-data/talos/ttest.py", line 378, in runTest
    test_results.add(browser_log_filename, counter_results=counter_results)
  File "/builds/tegra-259/talos-data/talos/results.py", line 120, in add
    browserLog = BrowserLogResults(filename=results, counter_results=counter_results, global_counters=self.global_counters)
  File "/builds/tegra-259/talos-data/talos/results.py", line 319, in __init__
    self.parse()
  File "/builds/tegra-259/talos-data/talos/results.py", line 356, in parse
    self.error("Could not find %s in browser output: (tokens: %s)" % (attr, tokens))
  File "/builds/tegra-259/talos-data/talos/results.py", line 331, in error
    raise utils.talosError(message)
talosError: "Could not find beforeLaunchTime in browser output: (tokens: ('__startBeforeLaunchTimestamp', '__endBeforeLaunchTimestamp')) [browser_output.txt]"
Traceback (most recent call last):
  File "run_tests.py", line 298, in <module>
FAIL: Busted: tp4m
FAIL: Could not find beforeLaunchTime in browser output: (tokens: ('__startBeforeLaunchTimestamp', '__endBeforeLaunchTimestamp')) [browser_output.txt]
    main()
  File "run_tests.py", line 295, in main
    run_tests(parser)
  File "run_tests.py", line 259, in run_tests
    raise e
utils.talosError: "Could not find beforeLaunchTime in browser output: (tokens: ('__startBeforeLaunchTimestamp', '__endBeforeLaunchTimestamp')) [browser_output.txt]"
program finished with exit code 1
}
(Assignee)

Comment 3

6 years ago
Created attachment 665345 [details] [diff] [review]
Don't output source revision

Is already in the buildbot log; TBPL just has to strip it out of the TinderboxPrint output.
Attachment #665345 - Flags: review?(jhammel)
(Assignee)

Comment 4

6 years ago
Created attachment 665347 [details] [diff] [review]
Don't TinderboxPrint cycle time

...just show it in the log. (Buildbot logs already show time per build step; TBPL just strips this entry out).
Attachment #665347 - Flags: review?(jhammel)
(Assignee)

Comment 5

6 years ago
Created attachment 665365 [details] [diff] [review]
Clean up graphserver links

The output we're getting at the moment is of the form:
{
RETURN:<br>
RETURN:<a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint: 619.03</a><br>
RETURN:<a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize: 13.0</a><br>
RETURN:<p style="font-size:smaller;">Details:<br>| <a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint</a> | <a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize</a> |</p>
}

Which after a lot of head scratching reading results_from_graph() - which is sadly less than clear - boils down to:

{
'RETURN:<br>' +
first_results + 
'\nRETURN:<p style="font-size:smaller;">Details:<br>' + 
last_results + 
'|</p>'
}

...where first_results is a newline separated list of lines for which we were able to find a linkvalue (ie: test score used as the link text for the URL).
...and last_results is a pipe separated list of lines for which we were not able to find a linkvalue.

At the moment last_results is just duplicating first_results, but without being able to see the graphserver response, I've no idea if this is expected/intended.

Therefore this patch just ditches the idea of first_results/last_results (as well as stopping adding random '<br/>'s all over the place), and just shows results that have a linkvalue. It also (IMO) makes results_from_graph()a lot more grokable.
Attachment #665365 - Flags: review?(jhammel)
(Assignee)

Comment 6

6 years ago
Comment on attachment 665365 [details] [diff] [review]
Clean up graphserver links

After this patch, the output in comment 5, becomes:

{
RETURN:<a href='http://graphs.mozilla.org/graph.html#tests=[[220,131,1]]'>tdhtmlr_paint: 619.03</a>
RETURN:<a href='http://graphs.mozilla.org/graph.html#tests=[[254,131,1]]'>tresize: 13.0</a>
}
(Assignee)

Comment 7

6 years ago
Created attachment 665373 [details] [diff] [review]
Don't also output talosErrors as 'FAIL:'

TBPL now recognises talosError, so the "FAIL:" output here is redundant, and is one of the contributing factors in the mess that is comment 2.
Attachment #665373 - Flags: review?(jhammel)
(Assignee)

Updated

6 years ago
Blocks: 794895
(Assignee)

Updated

6 years ago
Depends on: 794901

Comment 8

6 years ago
Comment on attachment 665345 [details] [diff] [review]
Don't output source revision

+  if not ('repository' in browser_config) and ('sourcestamp' in browser_config):

Since I'm one of those fools that never remembers order of operations, I'd prefer

if not ('repository' in browser_config and 'sourcestamp' in browser_config):

or if you want to be even more verbose:

if not (('repository' in browser_config) and ('sourcestamp' in browser_config)):

I'm going to give this to jmaher to review.  I have no objections, but I also have no idea why this code is there o_O
Attachment #665345 - Flags: review?(jmaher)
Attachment #665345 - Flags: review?(jhammel)
Attachment #665345 - Flags: review+

Comment 9

6 years ago
Comment on attachment 665347 [details] [diff] [review]
Don't TinderboxPrint cycle time

As long as we're cleaning up, can we format this in a more consistent way?

+  print "cycle time: %s" % elapsed
Attachment #665347 - Flags: review?(jhammel) → review+

Comment 10

6 years ago
Comment on attachment 665365 [details] [diff] [review]
Clean up graphserver links

This is fine by me.  As a standard disclaimer, this is integration code that I'm not overly fond of being in talos anyway.  So if it doesn't cause any problem with TBPL, I'm all for it
Attachment #665365 - Flags: review?(jhammel) → review+

Comment 11

6 years ago
Comment on attachment 665373 [details] [diff] [review]
Don't also output talosErrors as 'FAIL:'

Yep, wfm
Attachment #665373 - Flags: review?(jhammel) → review+
Comment on attachment 665345 [details] [diff] [review]
Don't output source revision

Review of attachment 665345 [details] [diff] [review]:
-----------------------------------------------------------------

as far as I know this is good.  we should really run this on try server.
Attachment #665345 - Flags: review?(jmaher) → review+
(Assignee)

Comment 15

6 years ago
I don't understand this failure:

{
Traceback (most recent call last):
  File "remotePerfConfigurator.py", line 243, in <module>
    sys.exit(main())
  File "remotePerfConfigurator.py", line 239, in main
    conf.parse_args(args)
  File "/builds/tegra-095/talos-data/talos/PerfConfigurator.py", line 398, in parse_args
    options, args = Configuration.parse_args(self, *args, **kwargs)
  File "/builds/tegra-095/talos-data/talos/configuration.py", line 466, in parse_args
    self(*config)
  File "/builds/tegra-095/talos-data/talos/configuration.py", line 372, in __call__
    self.validate()
  File "remotePerfConfigurator.py", line 120, in validate
    self._setupRemote(deviceip, deviceport)
  File "remotePerfConfigurator.py", line 197, in _setupRemote
    raise pc.ConfigurationError("Unable to connect to remote device '%s'" % deviceip)
PerfConfigurator.ConfigurationError: Unable to connect to remote device '10.250.49.83'
program finished with exit code 1
elapsedTime=0.419459
========= Finished 'python remotePerfConfigurator.py ...' failed (results: 2, elapsed: 0 secs) (at 2012-09-27 12:47:46.626564) =========
}
(Assignee)

Comment 16

6 years ago
Or more: This failure doesn't seem like it could be related, but yet it is occurring on every android talos run on my push and none others I can see on Try; so must be due to my push somehow.
This error is during confuguration when we connect to the device and setup it up.  We also need to pull information like the deviceroot from the device.
(Assignee)

Updated

6 years ago
Blocks: 794901
No longer depends on: 794901
(Assignee)

Comment 18

6 years ago
Comment on attachment 665345 [details] [diff] [review]
Don't output source revision

Review of attachment 665345 [details] [diff] [review]:
-----------------------------------------------------------------

::: talos/run_tests.py
@@ +51,5 @@
>      if not browser_config.get('browser_name'):
>        browser_config['browser_name'] = config.get('App', 'Name')
>      if not browser_config.get('browser_version'):
>        browser_config['browser_version'] = config.get('App', 'Version')
> +  if not ('repository' in browser_config) and ('sourcestamp' in browser_config):

Bah, this should be:
if not ((foo) and (bar)):

...missing some parentheses.
(Assignee)

Comment 19

6 years ago
Created attachment 668008 [details] [diff] [review]
Don't output source revision (v2)

Fix parentheses.
Attachment #665345 - Attachment is obsolete: true
Attachment #668008 - Flags: review?(jhammel)

Comment 20

6 years ago
Comment on attachment 668008 [details] [diff] [review]
Don't output source revision (v2)

wfm
Attachment #668008 - Flags: review?(jhammel) → review+
(Assignee)

Updated

6 years ago
Depends on: 797936
(Assignee)

Updated

6 years ago
Depends on: 801633
You need to log in before you can comment on or make changes to this bug.