As a prerequisite for manifests, we need to reduce our usage of Makefiles for defining tests and logic. In order to reduce our dependency on makefiles, we need to ensure that we have all the test files for a given harness (mochitest, chrome, browser) in their own subfolders. Once this is done, we can start to change the makefile system to copy the entire subdirectory and use manifests to provide the conditional logic.

For the record these 23 patches fix all directories that have >1 harness type (i.e. mochitest + browser-chrome, or mochitest + chrome) in a single directory/Makefile.in by putting them in their own harness subdirs: before: rootdir/test rootdir/test/unit after: rootdir/test/mochitest rootdir/test/browser rootdir/test/chrome rootdir/test/unit Ideally we would have all tests under a /test/ directory, but for sake of getting things done, this only touches the directories which have >1 harness in them. If desired, I can go through in future patches and adjust the remaining directories, likewise I could put the crashtest/reftest files inside the /test/ folder as well. The only tests remaining are all the tests which live under the dom/ top level folder in the tree. As it stands, all of the tests pass on try server for all platforms (opt, debug) for all mochitests/chrome/browser-chrome tests.

Comment on attachment 726099 [details] [diff] [review] 1) content/canvas/test Review of attachment 726099 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/mochitest/Makefile.in @@ +197,5 @@ > + > +# This test is bogus according to the spec; see bug 407107 > +# test_2d.path.rect.zero.6.html > + > +# split up into groups to work around command-line length limits This comment doesn't seem to make any sense.

Comment on attachment 726104 [details] [diff] [review] 3) content/html/content Review of attachment 726104 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/test/mochitest/moz.build @@ +10,5 @@ > +# We can't have index.html in this directory because it would prevent > +# running the tests here. > +TEST_DIRS += ['bug649134'] > + > +TEST_DIRS += ['forms'] Just collapse these two assignments into one.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25) > Comment on attachment 726099 [details] [diff] [review] > 1) content/canvas/test > > Review of attachment 726099 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/test/mochitest/Makefile.in > @@ +197,5 @@ > > + > > +# This test is bogus according to the spec; see bug 407107 > > +# test_2d.path.rect.zero.6.html > > + > > +# split up into groups to work around command-line length limits > > This comment doesn't seem to make any sense. It's ancient. I'll fix it up when I get around to getting my patches to these tests reviewed, but I'd rather not see more bitrot here :)

I wrote bug 852416 as a general tracking bug for mochitests on manifests. If there is a better tracking bug, please substitute for this and update its dependency tree

Comment on attachment 726106 [details] [diff] [review] 5) media/webaudio Review of attachment 726106 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/mochitest/Makefile.in @@ +37,5 @@ > + allowed.sjs \ > + can_play_type_ogg.js \ > + can_play_type_wave.js \ > + can_play_type_webm.js \ > + can_play_type_dash.js \ Can you fix the indentation while you're here?

Had I known you were doing this, I would have proposed moving the MOCHITEST_* variables over to moz.build files. We're just going to rewrite all this stuff in the near future anyway. Well, at least we have a tool for automatically moving Makefile variables to moz.build files to make it easier :)

Comment on attachment 726109 [details] [diff] [review] 7) browser/ Review of attachment 726109 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/shell/test/browser/Makefile.in @@ +11,5 @@ > +include $(DEPTH)/config/autoconf.mk > + > +MOCHITEST_BROWSER_FILES = browser_420786.js \ > + browser_633221.js \ > + $(NULL) Can you indent these properly while you're here?

Furthermore, my plan all along has been for the set of test files to live in moz.build (or at least have moz.build files point to manifests defining these lists of files) and then eliminate a whole slew of leaf Makefile.in/moz.build files from the tree. After all, the moz.build/Makefile.in in the test directories are essentially empty and just cause make recursion overhead. We should move as much data as possible up to parent directories and eliminate the overhead.

Comment on attachment 726112 [details] [diff] [review] 10) editor/ Review of attachment 726112 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/html/tests/mochitest/Makefile.in @@ +93,5 @@ > + ../data/cfhtml-firefox.txt \ > + ../data/cfhtml-ie.txt \ > + ../data/cfhtml-ooo.txt \ > + ../data/cfhtml-nocontext.txt \ > + $(NULL) These are going to wreak havoc with your "just copy the dirs wholesale" idea aren't they?

(In reply to Gregory Szorc [:gps] from comment #32) > Furthermore, my plan all along has been for the set of test files to live in > moz.build (or at least have moz.build files point to manifests defining > these lists of files) and then eliminate a whole slew of leaf > Makefile.in/moz.build files from the tree. After all, the > moz.build/Makefile.in in the test directories are essentially empty and just > cause make recursion overhead. We should move as much data as possible up to > parent directories and eliminate the overhead. I think this is a noble goal, and I don't think it conflicts with jmaher's desired end goal here. This is merely transitional. Having the tests live in foo/tests/mochitest is good for keeping things logically separated. When everything is in moz.build files, presumably you could just have foo/moz.build say MOCHITEST_DIRS += ["tests/mochitest"].

> ::: editor/libeditor/html/tests/mochitest/Makefile.in > @@ +93,5 @@ > > + ../data/cfhtml-firefox.txt \ > > + ../data/cfhtml-ie.txt \ > > + ../data/cfhtml-ooo.txt \ > > + ../data/cfhtml-nocontext.txt \ > > + $(NULL) > > These are going to wreak havoc with your "just copy the dirs wholesale" idea > aren't they? yes, there are a few places this happens. We can either copy the data, put it in a common folder (i.e. testing/mochitest/dynamic|static) or we could reference it from the http://mochi.test:8888/tests/... path to the mochitest-plain directory.

Comment on attachment 726115 [details] [diff] [review] 13) js/ Review of attachment 726115 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/jsd/test/mochitest/Makefile.in @@ +16,5 @@ > + > +MOCHITEST_FILES = test_bug507448.html bug507448.js \ > + test_bug617870-callhooks.html test-bug617870-callhooks.js jsd-test.js \ > + test_bug638178-execlines.html test-bug638178-execlines.js \ > + $(NULL) Can you fix the indentation here? Drives me nuts.

Comment on attachment 726117 [details] [diff] [review] 15) layout/forms/test Review of attachment 726117 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/test/chrome/Makefile.in @@ +16,5 @@ > + test_bug536567_perwindowpb.html \ > + bug536567_subframe.html \ > + test_bug665540.html \ > + bug665540_window.xul \ > + $(NULL) Can you fix this indentation while you're here? ::: layout/forms/test/mochitest/Makefile.in @@ +25,5 @@ > + test_bug476308.html \ > + test_bug477531.html \ > + test_bug477700.html \ > + bug477700_subframe.html \ > + test_textarea_resize.html \ and here.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #37) > Comment on attachment 726117 [details] [diff] [review] > 15) layout/forms/test > > Review of attachment 726117 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/forms/test/chrome/Makefile.in > @@ +16,5 @@ > > + test_bug536567_perwindowpb.html \ > > + bug536567_subframe.html \ > > + test_bug665540.html \ > > + bug665540_window.xul \ > > + $(NULL) > > Can you fix this indentation while you're here? > > ::: layout/forms/test/mochitest/Makefile.in > @@ +25,5 @@ > > + test_bug476308.html \ > > + test_bug477531.html \ > > + test_bug477700.html \ > > + bug477700_subframe.html \ > > + test_textarea_resize.html \ > > and here. Note that it's intentional; the 'bug's are lined up.

Comment on attachment 726120 [details] [diff] [review] 18) layout/style/test Review of attachment 726120 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/test/mochitest/Makefile.in @@ +229,5 @@ > + $(topsrcdir)/layout/reftests/svg/pseudo-classes-02-ref.svg \ > + $(topsrcdir)/layout/reftests/svg/as-image/lime100x100.svg \ > + $(topsrcdir)/layout/reftests/svg/as-image/svg-image-visited-1-helper.svg \ > + $(topsrcdir)/layout/reftests/svg/as-image/svg-image-visited-2-helper.svg \ > + $(NULL) Wow, this is horrible. @@ +247,5 @@ > + > +GARBAGE += css_properties.js > + > +libs:: $(_VISITED_REFTEST_FILES) > + $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)/css-visited/ As a followup we could use INSTALL_TARGETS here.

Comment on attachment 726121 [details] [diff] [review] 19) layout/xul/test Review of attachment 726121 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/xul/base/test/chrome/Makefile.in @@ +19,5 @@ > + test_resizer.xul \ > + window_resizer.xul \ > + window_resizer_element.xul \ > + test_windowminmaxsize.xul \ > + $(NULL) Fix the indentation? ::: layout/xul/base/test/mochitest/Makefile.in @@ +13,5 @@ > + > +MOCHITEST_FILES = test_bug511075.html \ > + test_splitter.xul \ > + test_resizer_incontent.xul \ > + $(NULL) Also here.

Comment on attachment 726122 [details] [diff] [review] 20) security/ Review of attachment 726122 [details] [diff] [review]: ----------------------------------------------------------------- Otherwise OK, but I don't want a stub Makefile/moz.build that does nothing but list child DIRS. ::: security/manager/ssl/tests/moz.build @@ +3,4 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +TEST_DIRS += ['browser', 'chrome', 'mochitest'] Don't make chrome/{Makefile.in,moz.build}, just list 'chrome/bugs' and 'chrome/stricttransportsecurity' directly here.

Comment on attachment 726125 [details] [diff] [review] 22) widget/ Review of attachment 726125 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/tests/chrome/Makefile.in @@ +7,5 @@ > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ > +relativesrcdir = @relativesrcdir@ > +FAIL_ON_WARNINGS = 1 This doesn't do anything useful in a directory without C++ source.

(In reply to :Ms2ger from comment #38) > Note that it's intentional; the 'bug's are lined up. I submit that that is horrible.

Comment on attachment 726123 [details] [diff] [review] 21) toolkit/ Review of attachment 726123 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/passwordmgr/test/chrome/Makefile.in @@ +14,5 @@ > +MOCHITEST_CHROME_FILES += \ > + test_privbrowsing_perwindowpb.html \ > + ../mochitest/notification_common.js \ > + ../mochitest/pwmgr_common.js \ > + ../mochitest/formsubmit.sjs \ We're going to have to figure something out here. :-/ Maybe we should bite the bullet and figure out how to unify mochitest-plain and mochitest-chrome.

updated patch as per r- comments. Works great locally, final sanity on try server underway.

Comment on attachment 735902 [details] [diff] [review] 20) security/ (2.0) Review of attachment 735902 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/moz.build @@ +4,4 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +TEST_DIRS += ['browser', 'chrome/bugs', 'chrome/stricttransportsecurity', 'mochitest'] nit: might be nicer to do TEST_DIRS += [ 'browser', ... ]

Comment on attachment 726112 [details] [diff] [review] 10) editor/ I'd rather if you did not do this at all, but I definitely don't want this change in the editor component. This makes some things harder than necessary (for example, changing the test type from mochitest-plain=>chrome and vice versa), and also makes it cumbersome to share resource files between different test files.

Also, I don't believe that there is any good reason why we could not parse the manifest files you're mentioning in comment 0 at build time and do the right thing. Whether or not the files are in a single directory should not make things much harder.