Closed Bug 920223 Opened 7 years ago Closed 7 years ago

Mass convert MOCHITEST_* variables to manifests

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(5 files, 2 obsolete files)

The bulk of work for bug 920185 can likely be done automatically using a script that parses Makefile.in. I'm going to attempt an automatic bulk conversion.
No longer blocks: 920185
Blocks: 920185
Note that thanks to bug 912293, most if not all Makefile.in containing MOCHITEST_* variables only contain that, and thanks to bug 917622 the _PART_n stuff is gone too, which makes things simpler.
Depends on: 917622, 912293
This conversion was performed automatically. I'm pretty sure this patch
in isolation will result in failures in dom/tests/mochitest/general because
of missing files defined in chrome tests. Will address in a subsequent
automatic conversion.
Attachment #809488 - Flags: review?(Ms2ger)
Assignee: nobody → gps
Another automatic conversion.
Attachment #809491 - Flags: review?(Ms2ger)
Another completely automated conversion.
Attachment #809497 - Flags: review?(Ms2ger)
I'm on a roll!
Attachment #809505 - Flags: review?(Ms2ger)
I expect this to fail pretty hard.

https://tbpl.mozilla.org/?tree=Try&rev=27f9d990d41d

I diffed the output of _tests before and after this patch set. The following are interesting:

deleted: _tests/testing/mochitest/browser/toolkit/components/places/tests/browser/category-discover.png
deleted: _tests/testing/mochitest/browser/toolkit/components/places/tests/browser/dictionaryGeneric-16.png
deleted: _tests/testing/mochitest/browser/toolkit/components/places/tests/browser/extensionGeneric-16.png
deleted: _tests/testing/mochitest/browser/toolkit/components/places/tests/browser/localeGeneric.png
deleted: _tests/testing/mochitest/browser/toolkit/components/satchel/test/browser/subtst_privbrowsing.html
deleted: _tests/testing/mochitest/chrome/dom/tests/mochitest/general/test_innerScreen.xul
deleted: _tests/testing/mochitest/chrome/dom/tests/mochitest/general/test_offsets.css
deleted: _tests/testing/mochitest/chrome/dom/tests/mochitest/general/test_offsets.js
deleted: _tests/testing/mochitest/chrome/dom/tests/mochitest/general/test_offsets.xul
deleted: _tests/testing/mochitest/chrome/toolkit/content/tests/chrome/popup_shared.js
deleted: _tests/testing/mochitest/chrome/toolkit/content/tests/chrome/tree_shared.js
deleted: _tests/testing/mochitest/chrome/toolkit/mozapps/update/test/chrome/simple.mar
deleted: _tests/testing/mochitest/chrome/toolkit/mozapps/update/test/chrome/simple_no_pib.mar
deleted: _tests/testing/mochitest/tests/dom/devicestorage/ipc/devicestorage_common.js
untracked: _tests/testing/mochitest/browser/toolkit/components/places/tests/browser/colorAnalyzer/

These will likely require some manual correction in a part 5.
There was a weird syntax in dom/tests/mochitest/general/Makefile.in. I
fixed that (in a patch I will soon submit) and regenerated this patch.
Attachment #809547 - Flags: review?(Ms2ger)
Attachment #809488 - Attachment is obsolete: true
Attachment #809488 - Flags: review?(Ms2ger)
Regenerated with syntax issue fixed. This should fix the missing files
issue.
Attachment #809548 - Flags: review?(Ms2ger)
Attachment #809491 - Attachment is obsolete: true
Attachment #809491 - Flags: review?(Ms2ger)
This patch makes the output state of _tests consistent with before the
patchset module some paths for the "color analyzer" test. I worked
around this one issue by simply changing the test to look for files in
their new location. The test passed in a local run.

The reason we had to restore some entries for ../ relative paths is
because emitter.py has an arbitrary restriction where paths outside of
the current directory are ignored. Looks like we'll need to lift this
restriction. But, I'm inclined to punt that to the realm of a followup,
as it requires touching some Python. Plus, I'd like to be sure the new
manifest code sticks before mucking with it too much.

https://tbpl.mozilla.org/?tree=Try&rev=9848df4213c1
Attachment #809552 - Flags: review?(Ms2ger)
I'm sure Ted will have an opinion on this patch set.

Something else I haven't said yet.

I'd eventually like to move the references to the manifest files into parent moz.build files. I figure we can do that as a followup. We can probably even enforce a strictness during moz.build reading: if no Makefile.in exists and the moz.build only defines test manifests, fail (and move the test manifests to a parent directory).
Flags: needinfo?(ted)
Comment on attachment 809548 [details] [diff] [review]
Part 2: Mass convert MOCHITEST_CHROME_FILES to manifests

Review of attachment 809548 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutmemory/tests/Makefile.in
@@ +7,5 @@
>  MOCHITEST_CHROME_FILES += \
>    remote.xul \
>    test_memoryReporters.xul \
>    test_memoryReporters2.xul \
>    $(NULL)

I just looked at this file because it's a directory where I own all the tests and I was curious.  The first assignment of MOCHITEST_CHROME_FILES has been changed, but this conditional update hasn't.  I'm guessing it should be?  And I wonder how many similar cases have been missed...
(In reply to Nicholas Nethercote [:njn] from comment #11)
> ::: toolkit/components/aboutmemory/tests/Makefile.in
> @@ +7,5 @@
> >  MOCHITEST_CHROME_FILES += \
> >    remote.xul \
> >    test_memoryReporters.xul \
> >    test_memoryReporters2.xul \
> >    $(NULL)
> 
> I just looked at this file because it's a directory where I own all the
> tests and I was curious.  The first assignment of MOCHITEST_CHROME_FILES has
> been changed, but this conditional update hasn't.  I'm guessing it should
> be?  And I wonder how many similar cases have been missed...

The automated conversion tool doesn't handle assignments under conditions. There are various reasons for this, including us wanting to manually do that conversion so items under the same conditional are in the same block (otherwise we could get multiple identical condition blocks in the same file, which would decrease readability).
Depends on: 921198
No longer depends on: 921198
Comment on attachment 809547 [details] [diff] [review]
Part 1: Mass convert MOCHITEST_FILES to manifests

Review of attachment 809547 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me, but please double-check before landing that nothing changed in _tests.
Attachment #809547 - Flags: review?(Ms2ger) → review+
Comment on attachment 809548 [details] [diff] [review]
Part 2: Mass convert MOCHITEST_CHROME_FILES to manifests

Review of attachment 809548 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: browser/components/feeds/test/chrome/Makefile.in
@@ +3,5 @@
>  # 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/.
>  
>  # sample_feed.atom was copied from toolkit/components/places/tests/chrome
>  MOCHITEST_FILES	= \

What about this?

::: content/base/test/chrome/Makefile.in
@@ +4,2 @@
>  
>  MOCHITEST_FILES = \

This?

::: toolkit/mozapps/update/test/chrome/Makefile.in
@@ +5,3 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  libs:: 

Check if this bit is relevant
Attachment #809548 - Flags: review?(Ms2ger) → review+
Comment on attachment 809497 [details] [diff] [review]
Part 3: Mass convert MOCHITEST_BROWSER_FILES to manifests

Review of attachment 809497 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: dom/tests/browser/Makefile.in
@@ +5,1 @@
>    $(NULL)

Heh

::: toolkit/components/downloads/test/browser/Makefile.in
@@ +6,2 @@
>  # These are files that need to be loaded via the HTTP proxy server
>  MOCHITEST_FILES = \

?

::: toolkit/components/places/tests/browser/Makefile.in
@@ +6,3 @@
>  # These are files that need to be loaded via the HTTP proxy server
>  # Access them through http://example.com/
>  MOCHITEST_FILES = \

?
Attachment #809497 - Flags: review?(Ms2ger) → review+
Comment on attachment 809505 [details] [diff] [review]
Part 4: Mass convert MOCHITEST_A11Y_FILES to manifests

Review of attachment 809505 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #809505 - Flags: review?(Ms2ger) → review+
Comment on attachment 809552 [details] [diff] [review]
Part 5: Fixup issues from automatic conversions

Review of attachment 809552 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, I guess.
Attachment #809552 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a30d73daf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bff106b704
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b6b152f51a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b02873ee07e
https://hg.mozilla.org/integration/mozilla-inbound/rev/39a308770f17
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0509d0adb3

The review comments noticing stray blocks that should have been converted appear to identify a bug in the converter. It appears that variables immediately after comments are not detected. Likely an off-by-1 error. Oh well. We'll handle these stray blocks in followups.
Status: NEW → ASSIGNED
Flags: needinfo?(ted)
Comment on attachment 809547 [details] [diff] [review]
Part 1: Mass convert MOCHITEST_FILES to manifests

Review of attachment 809547 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/network/tests/Makefile.in
@@ -6,5 @@
> -  test_network_basics.html \
> -  test_tcpsocket_default_permissions.html \
> -  test_tcpsocket_enabled_no_perm.html \
> -  test_tcpsocket_enabled_with_perm.html \
> -  $(NULL)

This enabled all these for B2G (below is an ifdef using "=" not "+="), and a bunch of them are now failing intermittently.
s/intermittently/permanently/

Also, a bunch of other new intermittents (particular on browser-chrome) seem to have cropped up on this push and the ones after, not entirely convinced this isn't to blame.

Backing out for now (I need to head out for a bit shortly, don't have time to sift through more now) - please can we have some more verification we haven't adjusted the lists of tests being run on each platform? (Along the lines of what Ms2ger requested in comment 13).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c35a179f65
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d884bd18873c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8514e27576
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32cccb8b64e8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/deac6e30aaf2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d7645cd0f41f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b832eca64b91
Relanded part 0 because trivial and I don't like patches sitting around.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf49a049d30
Whiteboard: [leave open]
(In reply to Ed Morley [:edmorley UTC+1] from comment #19)
> ::: dom/network/tests/Makefile.in
> @@ -6,5 @@
> > -  test_network_basics.html \
> > -  test_tcpsocket_default_permissions.html \
> > -  test_tcpsocket_enabled_no_perm.html \
> > -  test_tcpsocket_enabled_with_perm.html \
> > -  $(NULL)
> 
> This enabled all these for B2G (below is an ifdef using "=" not "+="), and a
> bunch of them are now failing intermittently.

WTF. I'll just revert this directory.
Landed just part 1. We'll land patches one at a time as a means to isolate fallout. The goal is to get through them all as quickly as possible.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae5aa6fb0db
Blocks: 922517
Depends on: 925766
How come we still have MOCHITEST_FILES in Makefile.ins after this bug?
(In reply to Mike Hommey [:glandium] from comment #26)
> How come we still have MOCHITEST_FILES in Makefile.ins after this bug?

(In reply to Gregory Szorc [:gps] from comment #12)
> The automated conversion tool doesn't handle assignments under conditions.
> There are various reasons for this, including us wanting to manually do that
> conversion so items under the same conditional are in the same block
> (otherwise we could get multiple identical condition blocks in the same
> file, which would decrease readability).
There are many remaining ones without conditionals.
(In reply to Mike Hommey [:glandium] from comment #28)
> There are many remaining ones without conditionals.

It's a bug in the tool that performs the automatic conversion. I think if there was a comment before the variable it gets confused.
Depends on: 939080
Blocks: 1160755
Blocks: 773349
No longer blocks: 1160755
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.