920223 - Mass convert MOCHITEST_* variables to manifests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Mike Hommey [:glandium]]

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 1: Mass convert MOCHITEST_FILES to manifests

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.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 2: Mass convert MOCHITEST_CHROME_FILES to manifests

Another automatic conversion.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 3: Mass convert MOCHITEST_BROWSER_FILES to manifests

Another completely automated conversion.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 4: Mass convert MOCHITEST_A11Y_FILES to manifests

I'm on a roll!

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Part 1: Mass convert MOCHITEST_FILES to manifests

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.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Part 2: Mass convert MOCHITEST_CHROME_FILES to manifests

Regenerated with syntax issue fixed. This should fix the missing files
issue.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Part 5: Fixup issues from automatic conversions

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

Comment 10

[Gregory Szorc [:gps]]

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).

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Gregory Szorc [:gps]]

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

Comment 13

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

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.

Comment 14

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

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

Comment 15

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

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 = \

?

Comment 16

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

Comment on attachment 809505 [details] [diff] [review]
Part 4: Mass convert MOCHITEST_A11Y_FILES to manifests

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

lgtm

Comment 17

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

Comment on attachment 809552 [details] [diff] [review]
Part 5: Fixup issues from automatic conversions

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

Alright, I guess.

Comment 18

[Gregory Szorc [:gps]]

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.

Comment 19

[Ed Morley [:emorley]]

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.

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Gregory Szorc [:gps]]

Relanded part 0 because trivial and I don't like patches sitting around.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf49a049d30

Comment 22

[Gregory Szorc [:gps]]

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

Comment 23

[Gregory Szorc [:gps]]

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

Comment 24

[Gregory Szorc [:gps]]

Try came back happy. edmorley attributes the "other" failures to weirdness and said it was OK to land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef9d1885496
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe8ab10bee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fad2cd8390b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf84234319a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dcf49a049d30
https://hg.mozilla.org/mozilla-central/rev/4ae5aa6fb0db
https://hg.mozilla.org/mozilla-central/rev/3ef9d1885496
https://hg.mozilla.org/mozilla-central/rev/9fe8ab10bee7
https://hg.mozilla.org/mozilla-central/rev/1fad2cd8390b
https://hg.mozilla.org/mozilla-central/rev/8bf84234319a

Comment 26

[Mike Hommey [:glandium]]

How come we still have MOCHITEST_FILES in Makefile.ins after this bug?

Comment 27

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

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

Comment 28

[Mike Hommey [:glandium]]

There are many remaining ones without conditionals.

Comment 29

[Gregory Szorc [:gps]]

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