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

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emorley, Assigned: jmaher)

Tracking

({sheriffing-P2})

Trunk
sheriffing-P2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
what adds to the redundant lines, the text 'Error' or some variation?

Comment 3

5 years ago
Forgive me for being slow, but is this different from bug 795006 ?
this is for not printing additional lines in the output that will show up in the tbpl helper text.
Created attachment 702824 [details] [diff] [review]
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.
Assignee: emorley → jmaher
Attachment #702824 - Flags: review?(jhammel)
Attachment #702824 - Flags: feedback?(emorley)
(Reporter)

Comment 6

5 years ago
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".
Attachment #702824 - Flags: feedback?(emorley) → feedback-

Comment 7

5 years ago
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
Attachment #702824 - Flags: review?(jhammel) → review-
Created attachment 703885 [details] [diff] [review]
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.
Attachment #702824 - Attachment is obsolete: true
Attachment #703885 - Flags: review?(jhammel)
Attachment #703885 - Flags: feedback?(emorley)

Comment 9

5 years ago
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!
Attachment #703885 - Flags: review?(jhammel) → review+
http://hg.mozilla.org/build/talos/rev/666e0eda4edb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 12

5 years ago
bad indentation:

https://tbpl.mozilla.org/php/getParsedLog.php?id=18936285&tree=Mozilla-Inbound&full=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 13

5 years ago
Created attachment 704064 [details] [diff] [review]
correct to horrible 2 space indentation
Attachment #704064 - Flags: review?(jmaher)
Comment on attachment 704064 [details] [diff] [review]
correct to horrible 2 space indentation

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

Aye!
Attachment #704064 - Flags: review?(jmaher) → review+

Comment 15

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

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 16

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

Updated

5 years ago
Attachment #703885 - Flags: feedback?(emorley) → feedback+
(Reporter)

Updated

5 years ago
Depends on: 832442
You need to log in before you can comment on or make changes to this bug.