920185 - [meta] Move MOCHITEST_*_FILES variables out of Makefile.in / convert all tests to use manifests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 2

[(not currently active) Ted Mielczarek]

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

Comment 3

[(not currently active) Ted Mielczarek]

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

Comment 4

[Gregory Szorc [:gps]]

Fixed the docs: https://hg.mozilla.org/integration/mozilla-inbound/rev/580eefd59837

Comment 5

[(not currently active) Ted Mielczarek]

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

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: Full list of files in $objdir/_tests not covered by a manifest

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.

Comment 10

[(not currently active) Ted Mielczarek]

This is nearly done. The dependent bugs have patches that will move all remaining mochitests to install manifests.

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

all dependencies done, are we sure?

Comment 13

[(not currently active) Ted Mielczarek]

\o/

Comment 14

[(not currently active) Ted Mielczarek]

I'll give it another sanity check, but the analysis from comment 9 is pretty thorough.

Comment 15

[Gregory Szorc [:gps]]

\o/

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: Full list of files in $objdir/_tests not covered by a manifest

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.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 18

[(not currently active) Ted Mielczarek]

Let's file a followup on that. It seems significantly different from the other Mochitest work.