1162003 - tracking bug to get mochitest-plain running in run-by-dir mode

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Comment 1

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

Attachment: current WIP

Comment 2

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

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 3

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 4

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

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 5

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 6

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

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?

Comment 7

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 8

[Kaustabh Datta Choudhury]

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

Comment 9

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

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

Comment 10

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15da056ced92

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1306c43f4996

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1306c43f4996

Comment 14

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

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 15

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 16

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

Comment on attachment 8658213 [details] [diff] [review]
rbd_plain.patch

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

thanks, this is looking good!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06bb0c1766f1

Comment 18

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 19

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 20

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

Comment on attachment 8659347 [details] [diff] [review]
rbd_plain.patch

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

Thanks Kaustabh!

Comment 21

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

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 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09b298831189

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09b298831189

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81fca7e4e6ef

Comment 25

[Karl Tomlinson (:karlt)]

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?

Comment 26

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

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.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81fca7e4e6ef

Comment 28

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

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 29

[Kaustabh Datta Choudhury]

Attachment: rbd_plain.patch

Comment 30

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

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.

Comment 31

[Karl Tomlinson (:karlt)]

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

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9b9304f10c

Comment 33

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

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!

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b9b9304f10c

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c2fc6ea271

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3c2fc6ea271

Comment 37

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

Attachment: rbd_asan.patch

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 38

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8672675 [details] [diff] [review]
rbd_asan.patch

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

\o/

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c053c40c80a

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c053c40c80a

Comment 41

[Firefox Bug Husbandry Bot]

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