Closed Bug 688762 Opened 13 years ago Closed 13 years ago

add try/except blocks around all harnesses so we always return an error code


(Testing :: General, defect)

Not set


(Not tracked)



(Reporter: jmaher, Assigned: jmaher)



(Keywords: intermittent-failure, Whiteboard: [mobile_unittests][android_tier_1])


(1 file)

currently our test harnesses assume that error are handled properly, but we have ran into many cases where we throw and exception and never catch it.  This causes us to let tegras run for hours :(
this covers a lot of the scenarios we have ran into in automation.  Most likely we will be more Orange than red/purple, but we will REDUCE the amount of idle time we spend on hung tegras.

I still need to test this, but I wanted to get it up to get the ball rolling.
Assignee: nobody → jmaher
Attachment #562046 - Flags: review?(bear)
I tested these locally and it is working as expected.
Attachment #562046 - Flags: review?(bear) → review+
Whiteboard: [orange][mobile_unittests][android_tier_1] → [orange][mobile_unittests][android_tier_1][inbound]
Blocks: 438871
Whiteboard: [orange][mobile_unittests][android_tier_1][inbound] → [orange][mobile_unittests][android_tier_1]
Target Milestone: --- → mozilla9
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 562046 [details] [diff] [review]
wrap try/except around harness code (1.0)

Review of attachment 562046 [details] [diff] [review]:

(I'm just glancing at this, so my comment may be irrelevent, but just in case...)

::: build/mobile/
@@ +709,5 @@
> +    try:
> +      retVal = self.pullFile(remoteFile)
> +    except:
> +      return None
> +      

I would have expected some kind of notice, but I'm not familiar with this file: (bare) 'return None' might be just correct here !?

::: layout/tools/reftest/
@@ +446,5 @@
>  #    manifest = "http://" + options.remoteWebServer + "/reftests/layout/reftests/reftest-sanity/reftest.list"
> +    try:
> +      reftest.runTests(manifest, options)
> +    except:
> +      print "TEST-UNEXPECTED-FAIL | | exception while running reftests"

Middle data (currently empty) used to be something like ''. But maybe you don't do that (anymore) here ?

::: testing/mochitest/
@@ +347,5 @@
> +    try:
> +      retVal = mochitest.runTests(options)
> +    except:
> +      print "TEST-UNEXPECTED-ERROR | | Exception caught while running tests."

Did you actually mean to use 'ERROR' here, whereas it's 'FAIL' everywhere else ?
(Ftr, should a try+catch be added to non-remote harnesses too, fwiw ?)
Version: unspecified → Trunk
for devicemanager.pullFile we need to return None to let other error handling capture it successfully.  We display an error inside of pullFile when we throw an exception.

For "TEST-UNEXPECTED-FAIL | | exception while running reftests", I was just copying other errors.  Maybe we do need to ensure all the errors in the file have the test file specified.

Oh no on the TEST-UNEXPECTED-ERROR;  I will need to clean that up.

As for desktop harnesses, we probably should add try/catch statements.
Comment on attachment 562046 [details] [diff] [review]
wrap try/except around harness code (1.0)

>--- a/build/mobile/
>+++ b/build/mobile/
>+    try:
>+      retVal = self.pullFile(remoteFile)
>+    except:
>+      return None

Not to forget the trailing whitespace here...

>--- a/testing/mochitest/
>+++ b/testing/mochitest/
>+    except:
>+      print "TEST-UNEXPECTED-ERROR | | Exception caught while running tests."
>+      mochitest.stopWebServer(options)
>+      mochitest.stopWebSocketServer(options)
>+      sys.exit(1)

... and here ...

>+    sys.exit(retVal)

... and here.
Whiteboard: [orange][mobile_unittests][android_tier_1] → [mobile_unittests][android_tier_1]
Component: Infrastructure → General
You need to log in before you can comment on or make changes to this bug.