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

RESOLVED FIXED in Firefox 56

Status

Testing
Reftest
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
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.
(Assignee)

Comment 3

a year ago
Good point. Should we automatically set --max-retries=0 if a debugger is attached?
That sounds good to me.  Although perhaps I should just be using --run-until-failure, which hopefully stops after the first crash too.
(Assignee)

Updated

a year ago
Attachment #8877696 - Flags: review?(shing.lyu) → review?(jmaher)

Comment 5

a year ago
mozreview-review
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?
Attachment #8877696 - Flags: review?(jmaher) → review-
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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+!
Attachment #8877696 - Flags: review- → review+

Comment 8

a year ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1990807be524
Limit reftest to 4 attempts at recovering after a crash, r=jmaher

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1990807be524
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.