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

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

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

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 :(

Comment 1

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

Attachment: wrap try/except around harness code (1.0)

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.

Comment 2

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

I tested these locally and it is working as expected.

Comment 3

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

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3995f7c64af8

Comment 4

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/mozilla-central/rev/3995f7c64af8

Comment 5

[Serge Gautherie (:sgautherie)]

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/devicemanagerSUT.py
@@ +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/remotereftest.py
@@ +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 'remotereftest.py'. But maybe you don't do that (anymore) here ?

::: testing/mochitest/runtestsremote.py
@@ +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 ?

Comment 6

[Serge Gautherie (:sgautherie)]

(Ftr, should a try+catch be added to non-remote harnesses too, fwiw ?)

Comment 7

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

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 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 562046 [details] [diff] [review]
wrap try/except around harness code (1.0)

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

Not to forget the trailing whitespace here...

>--- a/testing/mochitest/runtestsremote.py
>+++ b/testing/mochitest/runtestsremote.py
>+    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.