Closed Bug 920185 Opened 7 years ago Closed 7 years ago
[meta] Move MOCHITEST
_*_FILES variables out of Makefile .in / convert all tests to use manifests
43.00 KB, text/plain
Bug 901990 landed support for mochitest manifests. We should move all the make logic defining tests into manifests. We can do this incrementally. Please file bugs under here.
I looked through the outstanding users; a list of issues blocking the conversion in each of those follows. In most cases, we need to add more things to mozinfo, need to support manifests with only support files, or fix bug 939080. Also note that there's various more or less manual installation rules in various places not mentioned below, that also affect the contents of _tests. accessible/tests/mochitest/events/Makefile.in ifeq (,$(filter Linux WINNT,$(OS_ARCH))) accessible/tests/mochitest/Makefile.in $(topsrcdir)/content/media/test/bug461281.ogg \ browser/base/content/test/general/Makefile.in MOZ_WIDGET_GTK, MOZ_DATA_REPORTING, MOZ_CRASHREPORTER browser/components/preferences/in-content/tests/Makefile.in MOZ_SERVICES_HEALTHREPORT browser/components/preferences/tests/Makefile.in MOZ_SERVICES_HEALTHREPORT content/base/test/chrome/Makefile.in only support files to be installed into the mochitest-plain location content/base/test/Makefile.in MOZ_CHILD_PERMISSIONS content/canvas/test/Makefile.in MOZ_WIDGET_TOOLKIT checks content/media/webaudio/test/blink/Makefile.in only support files docshell/test/chrome/Makefile.in only support files docshell/test/navigation/Makefile.in MOZ_BUILD_APP dom/browser-element/mochitest/Makefile.in $(topsrcdir)/browser/base/content/test/general/audio.ogg \ MOZ_ANDROID_OMTC dom/devicestorage/ipc/Makefile.in only support files dom/permission/tests/Makefile.in MOZ_B2G* dom/tests/browser/Makefile.in MOZ_B2G only support files dom/tests/mochitest/ajax/lib/Makefile.in only support files dom/tests/mochitest/chrome/Makefile.in OS_ARCH dom/tests/mochitest/crypto/Makefile.in MOZ_DISABLE_CRYPTOLEGACY editor/libeditor/text/tests/Makefile.in MOZ_WIDGET_GTK layout/generic/test/Makefile.in $(srcdir)/../../reftests/backgrounds/blue-32x32.png \ layout/reftests/fonts/Makefile.in relativesrcdir = fonts layout/reftests/fonts/mplus/Makefile.in relativesrcdir = fonts/mplus layout/style/test/chrome/Makefile.in only support files layout/style/test/Makefile.in build-generated file security/manager/ssl/tests/mochitest/bugs/Makefile.in MOZ_DISABLE_CRYPTOLEGACY toolkit/components/downloads/test/browser/Makefile.in only support files toolkit/components/places/tests/browser/Makefile.in only support files toolkit/components/places/tests/chrome/Makefile.in only support files toolkit/components/places/tests/Makefile.in only support files toolkit/components/places/tests/mochitest/bug_411966/Makefile.in only support files toolkit/components/places/tests/mochitest/bug_461710/Makefile.in only support files toolkit/components/satchel/test/browser/Makefile.in MOCHITEST_BROWSER_FILES := ../subtst_privbrowsing.html toolkit/content/tests/chrome/Makefile.in MOCHITEST_CHROME_FILES := \ ../widgets/popup_shared.js \ ../widgets/tree_shared.js \ $(SHULL) toolkit/content/tests/widgets/Makefile.in $(topsrcdir)/content/media/test/audio.wav \ toolkit/devtools/apps/tests/Makefile.in only support files toolkit/mozapps/downloads/tests/chrome/Makefile.in OS_ARCH toolkit/mozapps/extensions/test/browser/Makefile.in double installation toolkit/mozapps/plugins/tests/Makefile.in only support files
(In reply to :Ms2ger from comment #1) > accessible/tests/mochitest/events/Makefile.in > ifeq (,$(filter Linux WINNT,$(OS_ARCH))) This is just: skip-if = os == 'win' || os == 'linux' # and oh boy is that easier to read! > browser/base/content/test/general/Makefile.in > MOZ_WIDGET_GTK, MOZ_DATA_REPORTING, MOZ_CRASHREPORTER we have 'toolkit', 'datareporting' and 'crashreporter' in mozinfo: https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/buildsystem/mozinfo.html#mozinfo-attributes > browser/components/preferences/in-content/tests/Makefile.in > MOZ_SERVICES_HEALTHREPORT > browser/components/preferences/tests/Makefile.in > MOZ_SERVICES_HEALTHREPORT This needs added. > content/base/test/Makefile.in > MOZ_CHILD_PERMISSIONS As does this. > content/canvas/test/Makefile.in > MOZ_WIDGET_TOOLKIT checks Use toolkit per above. > docshell/test/navigation/Makefile.in > MOZ_BUILD_APP We have appname which is MOZ_APP_NAME, maybe close enough? > dom/browser-element/mochitest/Makefile.in > $(topsrcdir)/browser/base/content/test/general/audio.ogg \ > MOZ_ANDROID_OMTC Needs added. > dom/permission/tests/Makefile.in > MOZ_B2G* > dom/tests/browser/Makefile.in > MOZ_B2G os == 'b2g'? > dom/tests/mochitest/chrome/Makefile.in > OS_ARCH Just use 'os'. > dom/tests/mochitest/crypto/Makefile.in > MOZ_DISABLE_CRYPTOLEGACY I guess needs added. > editor/libeditor/text/tests/Makefile.in > MOZ_WIDGET_GTK 'toolkit'.
We added MOZ_BUILD_APP but forgot to add it to the docs: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mozinfo.py#54
bug 968837 covers a bunch of these: (In reply to :Ms2ger from comment #1) > accessible/tests/mochitest/events/Makefile.in > browser/base/content/test/general/Makefile.in > content/base/test/chrome/Makefile.in > content/canvas/test/Makefile.in > content/media/webaudio/test/blink/Makefile.in > docshell/test/chrome/Makefile.in > docshell/test/navigation/Makefile.in > dom/tests/browser/Makefile.in > dom/tests/mochitest/ajax/lib/Makefile.in > dom/tests/mochitest/chrome/Makefile.in > editor/libeditor/text/tests/Makefile.in > layout/style/test/chrome/Makefile.in > toolkit/components/downloads/test/browser/Makefile.in > toolkit/components/places/tests/browser/Makefile.in > toolkit/components/places/tests/chrome/Makefile.in > toolkit/components/places/tests/Makefile.in > toolkit/components/places/tests/mochitest/bug_411966/Makefile.in > toolkit/components/places/tests/mochitest/bug_461710/Makefile.in > toolkit/devtools/apps/tests/Makefile.in > toolkit/mozapps/downloads/tests/chrome/Makefile.in > toolkit/mozapps/plugins/tests/Makefile.in Not covered: > accessible/tests/mochitest/Makefile.in > $(topsrcdir)/content/media/test/bug461281.ogg \ > browser/components/preferences/in-content/tests/Makefile.in > MOZ_SERVICES_HEALTHREPORT > browser/components/preferences/tests/Makefile.in > MOZ_SERVICES_HEALTHREPORT > content/base/test/Makefile.in > MOZ_CHILD_PERMISSIONS > dom/browser-element/mochitest/Makefile.in > $(topsrcdir)/browser/base/content/test/general/audio.ogg \ > MOZ_ANDROID_OMTC > dom/devicestorage/ipc/Makefile.in > only support files > dom/permission/tests/Makefile.in > MOZ_B2G* > dom/tests/mochitest/crypto/Makefile.in > MOZ_DISABLE_CRYPTOLEGACY > layout/generic/test/Makefile.in > $(srcdir)/../../reftests/backgrounds/blue-32x32.png \ > layout/reftests/fonts/Makefile.in > relativesrcdir = fonts > layout/reftests/fonts/mplus/Makefile.in > relativesrcdir = fonts/mplus > layout/style/test/Makefile.in > build-generated file > security/manager/ssl/tests/mochitest/bugs/Makefile.in > MOZ_DISABLE_CRYPTOLEGACY > toolkit/components/satchel/test/browser/Makefile.in > MOCHITEST_BROWSER_FILES := ../subtst_privbrowsing.html > toolkit/content/tests/chrome/Makefile.in > MOCHITEST_CHROME_FILES := \ > ../widgets/popup_shared.js \ > ../widgets/tree_shared.js \ > $(SHULL) > toolkit/content/tests/widgets/Makefile.in > $(topsrcdir)/content/media/test/audio.wav \ > toolkit/mozapps/extensions/test/browser/Makefile.in > double installation The main things we need to account for to fix the rest: * Installing files from different directories (bug 939080), should be simple, I'll tackle that after getting this round of tests finished * Those few missing conditionals we need in mozinfo (gps said he was adding most of them in a patch on a bug whose number I've forgotten) * A handful of preprocessed/build-generated files and the silliness in bug 948801
I'm updating mozinfo in bug 958561. That should hopefully land very soon now. Be on the lookup for clobbers required when you touch mozinfo.py since mozinfo.json is only regenerated at configure time. Bug 968245 also has a review up to fix this.
With my patches from bug 968403 and bug 975455 applied, we're down to just 9 Makefiles using MOCHITEST_.*FILES: $ find . -name "Makefile.in" | xargs grep -l "MOCHITEST_.*FILES" ./testing/profiles/Makefile.in ./layout/style/test/Makefile.in ./build/mobile/robocop/Makefile.in ./toolkit/mozapps/extensions/test/browser/Makefile.in ./toolkit/components/ctypes/tests/Makefile.in ./content/base/test/Makefile.in ./dom/permission/tests/Makefile.in ./dom/tests/mochitest/crypto/Makefile.in ./security/manager/ssl/tests/mochitest/bugs/Makefile.in There are almost certainly still a few Makefiles using custom rules to install tests as well. I'll have to hack something up to compare the contents of the install manifest to what's actually installed in test directories to find all of those.
Great work, Ted! I've noticed my no-op build speeds have gotten a little faster - presumably due to avoiding excessive I/O due to increased install manifest usage / decreased nsinstall usage. On the flip side, the _tests install manifest is now a very long pole. We should consider splitting that up or making install manifest processing use multiple threads for simultaneous I/O operations. Follow ups.
This is more thorough. It also shows that we don't currently have a way to specify test harness files via moz.build, which ought to be simple to fix.
This is nearly done. The dependent bugs have patches that will move all remaining mochitests to install manifests.
Assignee: nobody → ted
The last bug standing here is bug 948801. Currently that patch causes failures on Try, so I need to sort that out before landing it.
all dependencies done, are we sure?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I'll give it another sanity check, but the analysis from comment 9 is pretty thorough.
Updated list of all files in $objdir/_tests that weren't installed via manifest. There are no tests in there, so we're done. Followups will help us get rid of the rest of the things in there, but that's just nice for build speed, it doesn't affect test running.
Attachment #8380844 - Attachment is obsolete: true
oh, we have overlooked the robocop files: http://dxr.mozilla.org/mozilla-central/source/build/mobile/robocop/moz.build http://dxr.mozilla.org/mozilla-central/source/build/mobile/robocop/Makefile.in so in android.json we have an excluded path of 'robocop' which ignores this for regular mochitest, but now that we have removed that we are getting test_bug720538.html as a failing test (which is disabled for robocop anyway). This test only matches because it is test_*.html, the other robocop tests are test* <- no underscore. I thought a simple solution would be to create a mochitest.ini or reference the existing robocop.ini in the build system, unfortunately that won't work as we are not using this. We could rename the test file, it isn't being used :) I think we need to fix this build issue in general. Open to thoughts
Let's file a followup on that. It seems significantly different from the other Mochitest work.
You need to log in before you can comment on or make changes to this bug.