790960 - Talos should catch IOErrors of the form in bug 572127 and output with the TBPL compatible "talosError:" prefix

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Ed Morley [:emorley]]

Attachment: Patch v1

Whilst failures of the type bug 572127 do currently get recognised by TBPL (as of bug ), it is on the timeout, not the initial IOError:

{
NOISE: 
Traceback (most recent call last):
  File "C:\talos-slave\talos-data\talos\bcontroller.py", line 221, in ?
    sys.exit(main())
  File "C:\talos-slave\talos-data\talos\bcontroller.py", line 218, in main
    bcontroller.run()
  File "C:\talos-slave\talos-data\talos\bcontroller.py", line 161, in run
    results_file = open(self.browser_log, "a")
IOError: [Errno 13] Permission denied: 'browser_output.txt'
Failed tdhtmlr: 
		Stopped Fri, 07 Sep 2012 03:45:22
FAIL: Busted: tdhtmlr
FAIL: timeout exceeded
Traceback (most recent call last):
  File "run_tests.py", line 250, in run_tests
    talos_results.add(mytest.runTest(browser_config, test))
  File "C:\talos-slave\talos-data\talos\ttest.py", line 366, in runTest
    raise talosError("timeout exceeded")
talosError: 'timeout exceeded'
}

Note: Python noob so likely broken! ;-)

Comment 1

[Ed Morley [:emorley]]

> Whilst failures of the type bug 572127 do currently get recognised by TBPL
> (as of bug )

as of bug 790602

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 660836 [details] [diff] [review]
Patch v1

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

I am not sure if this is correct because bcontroller is run from a subprocess.Popen(...) which means that the error won't bubble up properly.  With that said, this might give us the correct error message we care about.  I need to test it a bit more.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated to print out talosError (1.0)

small adjustment here.  Since the raised exception doesn't cause the browser to fail and terminate the test, we need to print out a 'talosError:..." message and continue on.

With our current architecture, this seems like the best middle ground to start solidifying our errors until mozprocess comes along.

Comment 4

[Jeff Hammel]

Comment on attachment 661807 [details] [diff] [review]
updated to print out talosError (1.0)

This looks a bit wrong to me.

+from utils import talosError
+

This isn't used.  Also, since we already have utils imported, its probably easier to use utils.talosError versus adding an extra statement for this (if in fact it was used.

-        results_file = open(self.browser_log, "a")
+        try:
+          results_file = open(self.browser_log, "a")
+        except IOError, e:
+          print "talosError: %s" % str(e)
+
         results_file.write("\n__FAILbrowser frozen__FAIL\n")

So there are several things here that trouble me:

1. we print the literal string talosError instead of raising a talosError.  This is bad, IMHO, since if we ever change what talosError does and prints there will be a hidden case where we have to change this string to.

2. It seems like the placement of this try/except clause is a bit strange.  If we fail to open the file, the results_file.write() and .close() calls will *also* fail.

Why not enclose all of this in a try/except block and raise a talosError from that?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated to print out talosError (2.0)

this new patch addresses the review comments.

Comment 6

[Jeff Hammel]

Comment on attachment 662158 [details] [diff] [review]
updated to print out talosError (2.0)

looks good!

Comment 7

[Ed Morley [:emorley]]

Comment on attachment 662158 [details] [diff] [review]
updated to print out talosError (2.0)

https://hg.mozilla.org/build/talos/rev/caee59c88ac5

Comment 8

[Ed Morley [:emorley]]

For Talos, do we wait until deployment to close the bug?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

we close out the bugs, then deploy when we need a fix or want a fix to land.