bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in mozilla18

Status

Testing
Talos
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emorley, Assigned: jmaher)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sheriff-want])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 660836 [details] [diff] [review]
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! ;-)
Attachment #660836 - Flags: review?(jmaher)
(Reporter)

Comment 1

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

as of bug 790602
(Reporter)

Updated

6 years ago
Blocks: 778688
No longer blocks: 704006
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.
Created attachment 661807 [details] [diff] [review]
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.
Assignee: bmo → jmaher
Attachment #660836 - Attachment is obsolete: true
Attachment #660836 - Flags: review?(jmaher)
Attachment #661807 - Flags: review?(jhammel)

Comment 4

6 years ago
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?
Attachment #661807 - Flags: review?(jhammel) → review-
Created attachment 662158 [details] [diff] [review]
updated to print out talosError (2.0)

this new patch addresses the review comments.
Attachment #661807 - Attachment is obsolete: true
Attachment #662158 - Flags: review?(jhammel)

Comment 6

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

looks good!
Attachment #662158 - Flags: review?(jhammel) → review+
(Reporter)

Comment 7

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

https://hg.mozilla.org/build/talos/rev/caee59c88ac5
(Reporter)

Comment 8

6 years ago
For Talos, do we wait until deployment to close the bug?
we close out the bugs, then deploy when we need a fix or want a fix to land.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Depends on: 793746

Updated

6 years ago
No longer depends on: 793746

Updated

6 years ago
Depends on: 793746
(Reporter)

Updated

6 years ago
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.