1372981 - Limit the number of times reftest retries the harness after a crash

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Andrew Halberstadt [:ahal]]

Bug 1344991 implemented a neat new feature that tries to resume the test run in the event a test crashed. However, sometimes a developer might do a try push with a very crashy Firefox. In this case, the reftest harness will crash at every test and keep restarting.

This takes a very long time and makes the logs enormous (~250MB) due to all the crash stacks. We should put a limit on the number of crashes the harness will attempt to recover from before giving up. I arbitrarily propose 4 (though would be fine with a higher number too).

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1372981 - Limit reftest to 4 attempts at recovering after a crash,

Review commit: https://reviewboard.mozilla.org/r/149130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149130/

Comment 2

[Cameron McCormack (:heycam)]

Nice, this will help with a problem I had recently where I'd do a test run with |--debugger=rr|, but because of restarting after a crash, it would start recording a subsequent rr session, when really I just needed the first one.

Comment 3

[Andrew Halberstadt [:ahal]]

Good point. Should we automatically set --max-retries=0 if a debugger is attached?

Comment 4

[Cameron McCormack (:heycam)]

That sounds good to me.  Although perhaps I should just be using --run-until-failure, which hopefully stops after the first crash too.

Comment 5

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

Comment on attachment 8877696 [details]
Bug 1372981 - Limit reftest to 4 attempts at recovering after a crash,

https://reviewboard.mozilla.org/r/149130/#review155692

::: layout/tools/reftest/runreftest.py:706
(Diff revision 1)
>                  self.log.info("Process mode: {}".format('e10s' if options.e10s else 'non-e10s'))
>                  mozleak.process_leak_log(self.leakLogFile,
>                                           leak_thresholds=options.leakThresholds,
>                                           stack_fixer=get_stack_fixer_function(options.utilityPath,
>                                                                                options.symbolsPath))
> -                self.cleanup(profileDir)
> +                if status == 0:

do we not need to cleanup anymore?

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8877696 [details]
Bug 1372981 - Limit reftest to 4 attempts at recovering after a crash,

https://reviewboard.mozilla.org/r/149130/#review155692

> do we not need to cleanup anymore?

We do. If you expand the source down a bit (probably outside of this diff), we're calling self.cleanup() in a finally: block. So before this change, cleanup was getting called twice.

Comment 7

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

Comment on attachment 8877696 [details]
Bug 1372981 - Limit reftest to 4 attempts at recovering after a crash,

https://reviewboard.mozilla.org/r/149130/#review155728

thanks for pointing that out, r+!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1990807be524
Limit reftest to 4 attempts at recovering after a crash, r=jmaher

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1990807be524