Closed Bug 803493 Opened 7 years ago Closed 7 years ago

Move WebRTC Mochitests from dom/tests/mochitests/media to /dom/media/tests/mochitest

Categories

(Core :: WebRTC, defect)

18 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

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

Attachments

(2 files)

It would be useful to have the Mochitests right next to the actual code. It's similar to the crashtests which are located under /dom/media/tests/crashtests. Due to that Mochitests are the default test framework those should be moved to /dom/media/tests. Other frameworks will get their own subfolder.

Keep in mind that on the alder project branch there are tests located in /dom/media/tests but those are not on m-c and also shouldn't force us to use another location.

This task aligns to what we are doing recently for other modules too.

Jason, would you like to take that?
Invalid. I thought about this one when I originally was choosing the directory structure, but in reality, nobody was consistent in what they were choosing. I went with sticking the tests in one location mainly cause that mainly separates out digging into understanding the module's development vs. what you are aiming to test. I could technically hear arguments from either side, but I don't think it's worth arguing over this semantic, as that distracts us from what we really should be focusing on.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
You are welcome to join our Automation Development meeting on Monday, when we want to discuss that. But please don't close it as invalid right now.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Talked with media guys, dom/media/tests it is
Whiteboard: [WebRTC], [blocking-webrtc-]
I will take it.

Also see my newsgroup post:
https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/P4sE3wYV3Es
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Attached patch Patch v1Splinter Review
This patch moves the tests from /dom/tests/mochitest/media/ to /dom/media/tests/mochitest.
Attachment #675240 - Flags: review?(rjesup)
Comment on attachment 675240 [details] [diff] [review]
Patch v1

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

Review- only for the TEST_DIRS issue, as I don't think we should suggest starting a trend to stick all test dirs at the top-level directory. I'd also suggest doing a try run to make sure everything is hooked up okay.

::: dom/Makefile.in
@@ +98,5 @@
>  TEST_DIRS += \
>    tests \
>    imptests \
>    bindings/test \
> +  media/tests/mochitest \

I might suggest changing this to the following:

- media in this file
- Add TEST_DIRS with the directory below for followup level in the following Makefile

That will help separate out directory-specific details out a bit more.

::: dom/media/tests/mochitest/head.js
@@ +16,5 @@
> +function runTest(aCallback) {
> +  SimpleTest.waitForExplicitFinish();
> +
> +  SpecialPowers.pushPrefEnv({'set': [['media.peerconnection.enabled', true],
> +                            ['media.navigator.permission.disabled', true]]},

I realized after adding this, but seeing your other patch review that we actually don't need media.navigator.permission.disabled. Do you want to kill that off in this patch or take care of it separately?
Attachment #675240 - Flags: review-
Comment on attachment 675240 [details] [diff] [review]
Patch v1

Switching over to Ted to get additional feedback on such a change. Ted, please also see the latest comment from Jason for that patch.
Attachment #675240 - Flags: review?(rjesup) → review?(ted)
Comment on attachment 675240 [details] [diff] [review]
Patch v1

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

The build config peers are against Makefile proliferation. You don't need to add a second level Makefile that just has TEST_DIRS in it, putting the subdirectory into TEST_DIRS here is just fine.

r=me on the build bits.
Attachment #675240 - Flags: review?(ted) → review+
Attachment #675240 - Flags: review-
https://hg.mozilla.org/mozilla-central/rev/3e02a25a5e7d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc-] → [WebRTC], [blocking-webrtc-], [qa-]
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Comment on attachment 675240 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 675240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The build config peers are against Makefile proliferation. You don't need to
> add a second level Makefile that just has TEST_DIRS in it, putting the
> subdirectory into TEST_DIRS here is just fine.
> 
> r=me on the build bits.

There's a Makefile in dom/media/, though, which would have been a better place for this.
Apologies for that and misreading Jasons comment on it. He was already right with that. Not sure why I missed that we already have a makefile directly under /dom/media there. I will attach a follow-up patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #675240 - Flags: checkin+
This patch fixes the Makefile.in entry for dom/media tests. I also did a try server run to ensure everything is fine - and we are green:

https://tbpl.mozilla.org/?tree=Try&rev=0030b4f76c3c
Attachment #676019 - Flags: review?(rjesup)
Summary: Move WebRTC Mochitests from dom/tests/mochitests/media to /dom/media/tests → Move WebRTC Mochitests from dom/tests/mochitests/media to /dom/media/tests/mochitest
Attachment #676019 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/0cbe3d87fa74
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.