with the above patch, we still need to figure out which of the 8 tests in dom/media/webspeech/recognition/test are causing the crash(es).

Comment on attachment 8645120 [details] [diff] [review] rbd_plain.patch Review of attachment 8645120 [details] [diff] [review]: ----------------------------------------------------------------- a few small nits, address it or explain it and I will probably r+ :) ::: layout/style/test/mochitest.ini @@ +279,5 @@ > skip-if = buildapp == 'b2g' || toolkit == 'android' #TIMED_OUT # b2g(bug 870262, :visited support) b2g-debug(bug 870262, :visited support) b2g-desktop(bug 870262, :visited support) > [test_visited_pref.html] > skip-if = buildapp == 'b2g' || toolkit == 'android' #TIMED_OUT # b2g(bug 870262, :visited support) b2g-debug(bug 870262, :visited support) b2g-desktop(bug 870262, :visited support) > [test_visited_reftests.html] > +skip-if = buildapp == 'b2g' || toolkit == 'android' #TIMED_OUT # b2g(bug 870262, :visited support) b2g-debug(bug 870262, :visited support) b2g-desktop(bug 870262, :visited support I don't see what changed here, maybe a newline or a whitespace? ::: testing/mochitest/runtests.py @@ +2586,5 @@ > runner = Mochitest(logger_options) > + if runner.getTestFlavor(options) == 'browser-chrome' or runner.getTestFlavor(options) == 'mochitest': > + options.runByDir = True > + > + if (mozinfo.info['debug'] or mozinfo.info['asan']) and runner.getTestFlavor(options) != 'browser-chrome' or mozinfo.info.get('buildapp') == 'mulet': I would prefer to split this logic up a bit: runbydir = false if browserchrome: runbydir=true if mochitest && not debug: runbydir = true if asan || mulet: runbydir = false this seems a bit easier to follow and adjust without accidentally messing it up ::: testing/mochitest/runtestsb2g.py @@ +473,5 @@ > > if (options is None): > sys.exit(1) > > + options.runByDir = False Why do we need this twice in this file?

Comment on attachment 8645777 [details] [diff] [review] rbd_plain.patch Review of attachment 8645777 [details] [diff] [review]: ----------------------------------------------------------------- I think this is good, thanks for having bugs and a path here. Address the comments below and do a final try push for -p all -b do -u mochitests; and quite possibly we will be ready to land ::: testing/mochitest/runtests.py @@ +2589,5 @@ > + > + if runner.getTestFlavor(options) == 'browser-chrome': > + options.runByDir = True > + > + if runner.getTestFlavor(options) == 'mochitest' and !mozinfo.info['debug'] and !mozinfo.info['asan']: nit: "if runner" should have 1 space between. @@ +2594,5 @@ > + options.runByDir = True > + > + if mozinfo.info.get('buildapp') == 'mulet': > + options.runByDir = False > + I believe we have another definition of runbydir=true in runTests(); can we remove that or put this there?

Here's a push to try to test the patch : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7238f809f5b2

the try run had a little issue- I am sure it is something small.

BTW, I had to fix up the commit message for this when I pushed it. Try to use ones that describe what the patch is doing rather than something ambiguous or one that only restates the problem being solved. https://developer.mozilla.org/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

forgot to mark leave-open! This is live, we still have debug tests to sort out, I am fine leaving ASAN off the list for this bug.

Comment on attachment 8658213 [details] [diff] [review] rbd_plain.patch Review of attachment 8658213 [details] [diff] [review]: ----------------------------------------------------------------- thanks, this is looking good!

Comment on attachment 8659347 [details] [diff] [review] rbd_plain.patch Review of attachment 8659347 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Kaustabh!

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=857213947b09 with so many closures and delays, this takes 24+ hours to really get data, so we disabled osx+debug in the final patch that is r+ now.

Comment on attachment 8659347 [details] [diff] [review] rbd_plain.patch Test owners need to be needinfo'd when their tests are disabled. Can you ensure this has happened for all the tests disabled here, please? >+skip-if = os == 'win' && debug #Bug 1202564 > [test_waveShaperNoCurve.html] >+skip-if = os == 'win' && debug #Bug 1202565 > [test_waveShaperPassThrough.html] >+skip-if = os == 'win' && debug #Bug 1196084 > [test_waveShaperInvalidLengthCurve.html] >+skip-if = os == 'win' && debug #Bug 1202567 I didn't find anything in these bugs to indicate that they were disabled. Bug 1187204 was already fixed. Was it still necessary to disable these tests?

we can retest with these enabled today. Windows takes a while to get test results on try, so it will probably be Friday before the answer comes in.

Karl, thanks for calling this out, these tests work great with runbydir, we will have a patch in the short term to re-enable these! https://treeherder.mozilla.org/#/jobs?repo=try&revision=c82ee20d103e

Comment on attachment 8665957 [details] [diff] [review] rbd_plain.patch Review of attachment 8665957 [details] [diff] [review]: ----------------------------------------------------------------- Great work Kaustabh! Turning on tests is a great thing, and making run-by-dir more consistent is even better.

Thanks for re-enabling the webaudio tests. Have owners of the other tests disabled been needinfo'ed?

I have cleaned up a few bugs, 1 to retest, a few needinfo's and 1 closed as it is working. :karlt, thanks for making sure we are closing the loop!

tested on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6342832f8d9a this is looking good- would like to get this in before things change. This will get us to 100% runbydir for mochitest plain/chrome/browser-chrome. Not necessarily a11y, jetpack or other tests using mochitest.

Comment on attachment 8672675 [details] [diff] [review] rbd_asan.patch Review of attachment 8672675 [details] [diff] [review]: ----------------------------------------------------------------- \o/

Removing leave-open keyword from resolved bugs, per :sylvestre.