Mass convert MOCHITEST_* variables to manifests

RESOLVED FIXED in mozilla27

Status

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

Assignee

Description

6 years ago
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.

Updated

6 years ago
No longer blocks: 920185
Assignee

Updated

6 years ago
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
Assignee

Comment 2

6 years ago
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

Updated

6 years ago
Assignee: nobody → gps
Assignee

Comment 3

6 years ago
Another automatic conversion.
Attachment #809491 - Flags: review?(Ms2ger)
Assignee

Comment 4

6 years ago
Another completely automated conversion.
Attachment #809497 - Flags: review?(Ms2ger)
Assignee

Comment 5

6 years ago
I'm on a roll!
Attachment #809505 - Flags: review?(Ms2ger)
Assignee

Comment 6

6 years ago
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.
Assignee

Comment 7

6 years ago
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)
Assignee

Updated

6 years ago
Attachment #809488 - Attachment is obsolete: true
Attachment #809488 - Flags: review?(Ms2ger)
Assignee

Comment 8

6 years ago
Regenerated with syntax issue fixed. This should fix the missing files
issue.
Attachment #809548 - Flags: review?(Ms2ger)
Assignee

Updated

6 years ago
Attachment #809491 - Attachment is obsolete: true
Attachment #809491 - Flags: review?(Ms2ger)
Assignee

Comment 9

6 years ago
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)
Assignee

Comment 10

6 years ago
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...
Assignee

Comment 12

6 years ago
(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).
Assignee

Updated

6 years ago
Depends on: 921198
Assignee

Updated

6 years ago
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+
Assignee

Comment 18

6 years ago
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
Assignee

Comment 21

6 years ago
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]
Assignee

Comment 22

6 years ago
(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.
Assignee

Comment 23

6 years ago
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
Assignee

Updated

6 years ago
Blocks: 922517
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.
Assignee

Comment 29

6 years ago
(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

Updated

4 years ago
Blocks: 1160755

Updated

4 years ago
Blocks: 773349

Updated

4 years ago
No longer blocks: 1160755

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.