Closed Bug 817709 Opened 7 years ago Closed 7 years ago

Global flag to conditionally enable leaking mochitests

Categories

(Core :: WebRTC, defect)

20 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])

Attachments

(2 files, 3 obsolete files)

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.
This can only be done for mochitests. Crashtests we will have to enable all at once.
Summary: Global flag to enable leaking mochitests and crashtests → Global flag to conditionally enable leaking mochitests
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #688170 - Flags: review?(rjesup)
Once bug 812648 has been fixed we can remove this flag.
Depends on: 812648
Blocks: 817971
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?
Attachment #688170 - Flags: review?(rjesup) → review+
(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.
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 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.
Attachment #688170 - Flags: review-
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.
Attachment #688170 - Flags: review-
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 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.
Attachment #688170 - Flags: checkin+
Whiteboard: [WebRTC] [blocking-webrtc-]
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.
Attached patch patch v1.1Splinter Review
New patch with only an updated comment. I will carry over the last review from Randel and get it landed on inbound.
Attachment #688170 - Attachment is obsolete: true
Attachment #688755 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/256dd5fd9338
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Bustage fix (obsolete) — Splinter Review
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.
Attachment #689630 - Flags: review?(mbanner)
Attachment #689630 - Attachment is obsolete: true
Attachment #689630 - Flags: review?(mbanner)
Attached patch Bustage fix v1.1 (obsolete) — Splinter Review
Updated patch now with no other patches applied to the queue. Sorry.
Attachment #689633 - Flags: review?(mbanner)
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.
Attachment #689633 - Flags: review?(mbanner) → review+
Attached patch Bustage fix v2Splinter Review
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.
Attachment #689633 - Attachment is obsolete: true
Attachment #689649 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f43810048b24
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.