Closed Bug 1162003 Opened 4 years ago Closed 2 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox40 affected, firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- affected
firefox43 --- fixed

People

(Reporter: jmaher, Assigned: kaustabh93)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 8 obsolete files)

No description provided.
Depends on: 1162008
Depends on: 1162020
Summary: tracking but to get mochitest-plain running in run-by-dir mode → tracking bug to get mochitest-plain running in run-by-dir mode
Depends on: 1162025
Depends on: 1165339
Depends on: 1165343
Attached patch current WIP (obsolete) — Splinter Review
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).
Depends on: 1179782
Depends on: 1180351
Depends on: 1180388
Depends on: 1181577
Depends on: 1076833
Depends on: 1182467
Attached patch rbd_plain.patch (obsolete) — Splinter Review
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?
Attached patch rbd_plain.patch (obsolete) — Splinter Review
Attachment #8645120 - Attachment is obsolete: true
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?
Attached patch rbd_plain.patch (obsolete) — Splinter Review
Attachment #8645777 - Attachment is obsolete: true
Here's a push to try to test the patch : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7238f809f5b2
Assignee: nobody → kaustabh93
the try run had a little issue- I am sure it is something small.
Attachment #8606405 - Attachment is obsolete: true
Attachment #8646407 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/1306c43f4996
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 950636
Depends on: 1199778
Depends on: 1199912
Depends on: 1202564
Depends on: 1202565
Depends on: 1202567
Depends on: 1196084
Depends on: 1202570
Depends on: 1202683
Attached patch rbd_plain.patch (obsolete) — Splinter Review
Attachment #8646407 - Attachment is obsolete: true
Comment on attachment 8658213 [details] [diff] [review]
rbd_plain.patch

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

thanks, this is looking good!
Attachment #8658213 - Flags: review+
Attached patch rbd_plain.patch (obsolete) — Splinter Review
Attachment #8658213 - Attachment is obsolete: true
Attached patch rbd_plain.patch (obsolete) — Splinter Review
Attachment #8659341 - Attachment is obsolete: true
Comment on attachment 8659347 [details] [diff] [review]
rbd_plain.patch

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

Thanks Kaustabh!
Attachment #8659347 - Flags: review+
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.
Keywords: checkin-needed
Keywords: leave-open
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?
Flags: needinfo?(kaustabh93)
Flags: needinfo?(jmaher)
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
Flags: needinfo?(kaustabh93)
Flags: needinfo?(jmaher)
Attached patch rbd_plain.patchSplinter Review
Attachment #8659347 - Attachment is obsolete: true
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.
Attachment #8665957 - Flags: review+
Keywords: checkin-needed
Thanks for re-enabling the webaudio tests.
Have owners of the other tests disabled been needinfo'ed?
Flags: needinfo?(kaustabh93)
Flags: needinfo?(jmaher)
Flags: needinfo?(jmaher)
Keywords: checkin-needed
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!
Flags: needinfo?(kaustabh93)
No longer depends on: 1179826
Attached patch rbd_asan.patchSplinter Review
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.
Attachment #8672675 - Flags: review?(jgriffin)
Comment on attachment 8672675 [details] [diff] [review]
rbd_asan.patch

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

\o/
Attachment #8672675 - Flags: review?(jgriffin) → review+
Status: REOPENED → RESOLVED
Closed: 4 years ago2 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.