817709 - Global flag to conditionally enable leaking mochitests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

To be able to run our existent Mochitests even on mozilla-central we have to find an easy way to get them enabled. As a discussion on IRC has been shown the best way for now would be a conditional flag everyone could set in the .mozconfig file.

In our case it will be off by default on mozilla-central but enabled on the alder branch.

A patch should be up in a bit. Just running tests on mozilla-central to ensure the added code works as expected.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This can only be done for mochitests. Crashtests we will have to enable all at once.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This enables mochitests on mozilla-central if the following line gets added to your .mozconfig. By default this feature is disabled.

export MOZ_WEBRTC_LEAKING_TESTS=1

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Once bug 812648 has been fixed we can remove this flag.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 688170 [details] [diff] [review]
Patch v1

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

r+ if the tests are uncommented, unless I'm missing something

::: dom/media/tests/mochitest/Makefile.in
@@ +20,5 @@
> +MOCHITEST_FILES += \
> +# Bug 814718 prevents us from runnig the following tests:
> +#  test_getUserMedia_basicVideo.html \
> +#  test_getUserMedia_basicAudio.html \
> +#  test_getUserMedia_basicVideoAudio.html \

If these are inside an ifdef, shouldn't they be uncommented?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Randell Jesup [:jesup] from comment #4)
> If these are inside an ifdef, shouldn't they be uncommented?

No we can't enable them yet because they are blocked on bug 814718. I hope that this dependency can be fixed soon. Randell, any recommendation who could work on that? I would appreciate your feedback over on the other bug.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 688170 [details] [diff] [review]
Patch v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/991f2e95d735

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Eric, once landed you can run Mochitests for WebRTC:

1. Put 'export MOZ_WEBRTC_LEAKING_TESTS=1' in your .mozconfig
2. Re-configure and build
3. Run 'TEST_PATH=dom/media/tests/mochitest/ make -C ${OBJ_DIR} mochitest-plain'

Comment 8

[Jason Smith [:jsmith]]

Comment on attachment 688170 [details] [diff] [review]
Patch v1

r- - the only thing should be commented out here is the basic video test. The other two ran fine and did not hit bug 814718.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 688170 [details] [diff] [review]
Patch v1

This patch will not change any state of tests. You originally have disabled all of those tests so it should have been corrected on your initial patch. But it's clearly not a change for this bug. So please file a new bug to get those enabled. But as far as I can remember from my testing yesterday all of those tests are failing due to the other bug.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not sure why the flag has been removed. I will try to find the tryserver run. If we can really enable those tests we can push a follow-up once tested. Let me check that.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 688170 [details] [diff] [review]
Patch v1

I have backed out the patch for now. As best we push a try server build again from the alder branch with all the tests enabled. So we can easily see which ones we can take.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I triggered another try server build yesterday:
https://tbpl.mozilla.org/?tree=Try&rev=19e9c6b652d5

So the test failures for all tests happen on "Android 2.2 NoIon opt". As far as I can see we disabled our tests for Android on bug 781534. So not sure why those are getting run there. 

116 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_getUserMedia_basicAudio.html | Test timed out.
119 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_getUserMedia_basicVideo.html | Test timed out.
122 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_getUserMedia_basicVideoAudio.html | Test timed out.

I will file a separate bug on it and update the comments in the patch. That means I'm not going to change my patch in terms of enabling the other two tests.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v1.1

New patch with only an updated comment. I will carry over the last review from Randel and get it landed on inbound.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 688755 [details] [diff] [review]
patch v1.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/256dd5fd9338

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/256dd5fd9338

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So this was actually a faulty patch because it will cause a build failure due to the move of the commented out lines into the inclusion statement. You will never stop learning. Thanks for the hint Mark!

I will come up with a bustage fix for it in a bit.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bustage fix

This moves the comment back to the origin location with two differences:

* The shared module will be included by default because it doesn't leak and other tests could already make use of.

* I haven't put the bug for leaks in the comment again because that can change. Whenever the dependencies are fixed we will have to re-test for leaks. If leaks exist we simply add the test to the ifdef'ed part.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bustage fix v1.1

Updated patch now with no other patches applied to the queue. Sorry.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 689633 [details] [diff] [review]
Bustage fix v1.1

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

::: dom/media/tests/mochitest/Makefile.in
@@ -21,5 @@
> -# Bug 814718 and bug 818466 prevent us from running the following tests:
> -#  test_getUserMedia_basicVideo.html \
> -#  test_getUserMedia_basicAudio.html \
> -#  test_getUserMedia_basicVideoAudio.html \
> -#  mediaStreamPlayback.js \

Changes look fine, though I think you can leave mediaStreamPlayback.js not commented out ready for when the bugs are fixed to run the tests.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bustage fix v2

Right. The merge conflict I got due to the change based on the applied mq patch was not fully resolved. mediaStreamPlayback.js should have been added to MOCHITEST_FILES.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 689649 [details] [diff] [review]
Bustage fix v2

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

Comment 22

[(no longer active)]

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