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

RESOLVED FIXED in mozilla30

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: gps, Assigned: ted)

Tracking

(Blocks 1 bug, {meta})

Trunk
mozilla30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Depends on: 920223
No longer depends on: 920223
Depends on: 920223
Depends on: 920201
Depends on: 921987
Depends on: 923273
Depends on: 924160
Depends on: 924209
Depends on: 924463
Depends on: 924523
Depends on: 925887
Depends on: 932147
Blocks: 938019
Depends on: 939048
Depends on: 939080
Depends on: 939271
Depends on: 939518
Depends on: 939522
Depends on: 947770
Depends on: 948167
Depends on: 948258
Depends on: 948801
Depends on: 956862
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'.
Depends on: 968351
Depends on: 968403
Depends on: 968837
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.
Depends on: 971025
Depends on: 972510
Depends on: 770938
Depends on: 975455
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.
Depends on: 976528
Depends on: 977699
This is nearly done. The dependent bugs have patches that will move all remaining mochitests to install manifests.
Assignee: nobody → ted
Blocks: 980015
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?
\o/
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I'll give it another sanity check, but the analysis from comment 9 is pretty thorough.
\o/
Target Milestone: --- → mozilla30
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.