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.

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.

Talked with media guys, dom/media/tests it is

I will take it. Also see my newsgroup post: https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/P4sE3wYV3Es

This patch moves the tests from /dom/tests/mochitest/media/ to /dom/media/tests/mochitest.

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?

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.

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.

(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.

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

Comment on attachment 676019 [details] [diff] [review] Fix for Makefile.in https://hg.mozilla.org/integration/mozilla-inbound/rev/0cbe3d87fa74