829734 - Don't use talosError for crashes, to avoid doubling up with PROCESS-CRASH in TBPL's annotated summary

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Ed Morley [:emorley]]

Now that Talos correctly gives us PROCESS-CRASH lines, we should replace the:
raise talosError("stack found after process termination (%s)" % cleanup_result)
and
raise talosError("crash during run (stack found)")

...with generic exceptions that will not cause additional redundant lines in TBPL's annotated summary.

Comment 1

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

what adds to the redundant lines, the text 'Error' or some variation?

Comment 2

[Ed Morley [:emorley]]

"/^talosError:/"

https://hg.mozilla.org/webtools/tbpl/file/1c6ee7b76fed/php/inc/GeneralErrorFilter.php#l38

Comment 3

[Jeff Hammel]

Forgive me for being slow, but is this different from bug 795006 ?

Comment 4

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

this is for not printing additional lines in the output that will show up in the tbpl helper text.

Comment 5

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

Attachment: prevent talos from printing talosError after finding a crash and continue on (1.0)

this patch does two things:
1) removes the messages about finding crashes, we already print information about that
2) prevents raising exceptions and reporting results for a given test which fails, allowing future tests to continue on.

Comment 6

[Ed Morley [:emorley]]

Comment on attachment 702824 [details] [diff] [review]
prevent talos from printing talosError after finding a crash and continue on (1.0)

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

This will achieve just what I wanted thank you :-)

Just f- for fixing us not missing crashes in an earlier cycle.

::: talos/ttest.py
@@ +156,4 @@
>                  print "Remote Device Error: Error getting crash minidumps from device"
>                  raise
>  
> +        found = False

check_for_crashes only ever returns true or false (even if we hit an exception), so this first assignment is redundant, unless I'm missing something?

@@ +374,5 @@
>                      time.sleep(browser_config['browser_wait'])
>  
>                  #clean up any stray browser processes
> +                found = False
> +                found = self.cleanupAndCheckForCrashes(browser_config, profile_dir, test_config['name'])

This is inside a loop, so at the moment this will just return whether the final cycle crashed, so could hide earlier failures.

@@ +390,4 @@
>              test_results.all_counter_results.extend([{key: value} for key, value in global_counters.items()])
>  
>              # return results
> +            if not found:

Could we s/found/found_crash/ (or similar) throughout?
Otherwise for this block particularly, at first glance it seems like found is actually referring to "found results".

Comment 7

[Jeff Hammel]

Comment on attachment 702824 [details] [diff] [review]
prevent talos from printing talosError after finding a crash and continue on (1.0)

So....I don't really like this pattern.  Effectively we're faking an exception.  IMHO we should do one of a few things:

- subclass talosError -> talosCrash ; probably add a comment that the only reason we're doing this is for TBPL error matching.  then raise this error.  This might work.  (Or alternatively, make `class talosCrash(Exception)` without subclassing.)

- sys.exit(some_nonzero_code) somewhere directly in ttest.py.  I don't really like this but it seems like a lesser evil than now having a True/False return setting just for crashes.

Also, what :edmorley said

Comment 8

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

Attachment: updated to throw exception on crash (1.0)

updated patch to have a talosCrash class, as well as allowing tests to continue if we detect a crash.

Comment 9

[Jeff Hammel]

Comment on attachment 703885 [details] [diff] [review]
updated to throw exception on crash (1.0)

+                except talosError:
+                    pass

'twould be nice to document why we can just pass here.  That said, its a straight copy+paste of the original code.

+# Define this as an exception type where we want to report a crash
+# and stay compatible with tbpl while allowing us to continue on
+class talosCrash(Exception):
+  def __init__(self, msg):
+    self.msg = msg
+  def __str__(self):
+    return repr(self.msg)
+

A few things I would do different here:

- the pattern used in talosError is just plain wrong; just inheriting from Exception is enough.  no need to keep track of self.msg or roll your own string method. (If my cobwebby memory serves me correctly, this was necessary in, say, python 2.2.)

- move the comment to a docstring

class talosCrash(Exception):
    """exception type where we want to report a crash
    and stay compatible with tbpl while allowing us to continue on"""
    # I might also add the bug URL too

Overall, though, looks great!

Comment 10

[Jeff Hammel]

filed issue wrt talosError: https://bugzilla.mozilla.org/show_bug.cgi?id=832422

Comment 11

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

http://hg.mozilla.org/build/talos/rev/666e0eda4edb

Comment 12

[Jeff Hammel]

bad indentation:

https://tbpl.mozilla.org/php/getParsedLog.php?id=18936285&tree=Mozilla-Inbound&full=1

Comment 13

[Jeff Hammel]

Attachment: correct to horrible 2 space indentation

Comment 14

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

Comment on attachment 704064 [details] [diff] [review]
correct to horrible 2 space indentation

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

Aye!

Comment 15

[Jeff Hammel]

Going to push as things are busted anyway (wfm locally): http://hg.mozilla.org/build/talos/rev/9253fd09cda9

Comment 16

[Jeff Hammel]

(Incidentally, this is the reason i often don't test on try until after a r+)