Closed
Bug 968837
Opened 11 years ago
Closed 11 years ago
Move more Mochitests into manifests
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla30
People
(Reporter: ted, Assigned: ted)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
This covers a good chunk of what's left, I was working from the list in bug 920185 comment 1. I need to run this by try after I sort out an issue with one of the other patches in my queue.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
Comment on attachment 8371516 [details] [diff] [review]
Move more Mochitests into manifests
Review of attachment 8371516 [details] [diff] [review]:
-----------------------------------------------------------------
did a drive by here- this is looking pretty good in general- thanks for the work on this ted!
Attachment #8371516 -
Flags: feedback+
Assignee | ||
Comment 3•11 years ago
|
||
A few additions.
Assignee | ||
Updated•11 years ago
|
Attachment #8371516 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
Okay, this covers just about everything. I need to run this past the tryserver still. I compared the contents of $objdir/_tests/testing/mochitest before and after this patch, and on my Linux build the only differences were the added manifests + some support-files that I put into DEFAULT and so got copied even if the tests in that directory were disabled.
jmaher asked (on IRC) about the removal of browser/base/content/test/general/Makefile.in in this patch. It turns out that this Makefile had been previously removed in bug 932147, it must have been reverted in a bad merge:
http://hg.mozilla.org/mozilla-central/rev/dfe65b31de6a
There are two additional patches I'll upload shortly that I wanted separated out for clarity, but they can be folded back in for landing.
Attachment #8373438 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•11 years ago
|
Attachment #8371718 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
This is toolkit/components/places/tests/browser, which has a bunch of files that get installed from subdirectories. It turns out that the MOCHITEST*_FILES rules and the rules in manifests are not compatible in this regard--the former simply copies the leaf filename to the $relativesrcdir target directory, whereas the latter preserves the relative path. I chose to simply hg mv the files from subdirs up a level to avoid having to change test files to match.
Attachment #8373441 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 6•11 years ago
|
||
toolkit/component/places/tests/mochitest had the same problem, applied the same solution.
Attachment #8373442 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•11 years ago
|
Attachment #8373438 -
Flags: review?(Ms2ger) → review?(jmaher)
Assignee | ||
Updated•11 years ago
|
Attachment #8373441 -
Flags: review?(Ms2ger) → review?(jmaher)
Assignee | ||
Updated•11 years ago
|
Attachment #8373442 -
Flags: review?(Ms2ger) → review?(jmaher)
Updated•11 years ago
|
Attachment #8373442 -
Flags: review?(jmaher) → review+
Comment 7•11 years ago
|
||
Comment on attachment 8373441 [details] [diff] [review]
places/tests/browser
oh cool!
Attachment #8373441 -
Flags: review?(jmaher) → review+
Comment 8•11 years ago
|
||
Comment on attachment 8373438 [details] [diff] [review]
Move more Mochitests into manifests
Review of attachment 8373438 [details] [diff] [review]:
-----------------------------------------------------------------
patch in general is great, a few questions
::: content/canvas/test/mochitest.ini
@@ +153,5 @@
> +[test_2d.gradient.radial.outside3.html]
> +skip-if = toolkit != 'cocoa'
> +# These tests only pass on Mac OS X >= 10.5; see bug 450114
> +[test_2d.gradient.radial.touch1.html]
> +disabled = bug 450114
is bug 450114 a comment? I am unfamiliar with the disabled syntax here.
@@ +179,5 @@
> +skip-if = toolkit != 'cocoa'
> +# This test is bogus according to the spec; see bug 407107
> +[test_2d.path.rect.zero.6.html]
> +disabled = bug 407107
> +skip-if = toolkit != 'cocoa'
this appears to be disabled and skipped on cocoa? that is confusing
::: docshell/test/navigation/mochitest.ini
@@ +20,5 @@
> parent.html
>
> [test_bug13871.html]
> +[test_bug270414.html]
> +skip-if = buildapp == "mobile/android"
could we just use toolkit=="android"?
::: toolkit/components/places/tests/Makefile.in
@@ -12,3 @@
> # Simple MochiTests
> MOCHITEST_FILES = \
> mochitest/bug94514-postpage.html \
we still have mochitest and chrome files in here
::: toolkit/components/places/tests/mochitest/prompt_common.js
@@ -1,1 @@
> -
why is this file removed? I believe it is still used!
Attachment #8373438 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 8373438 [details] [diff] [review]
> Move more Mochitests into manifests
>
> Review of attachment 8373438 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> patch in general is great, a few questions
>
> ::: content/canvas/test/mochitest.ini
> @@ +153,5 @@
> > +[test_2d.gradient.radial.outside3.html]
> > +skip-if = toolkit != 'cocoa'
> > +# These tests only pass on Mac OS X >= 10.5; see bug 450114
> > +[test_2d.gradient.radial.touch1.html]
> > +disabled = bug 450114
>
> is bug 450114 a comment? I am unfamiliar with the disabled syntax here.
The value of the disabled key is ignored, so I just used it as a convenient place to put the reason. There are a few instances of this in the tree already, although it's not widespread:
http://mxr.mozilla.org/mozilla-central/search?string=disabled+%3D&find=\.ini&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
I like it better than comments, since it's actually data in the manifest, but we don't have anything comparable for skip-if disablement, which is a bummer.
>
> @@ +179,5 @@
> > +skip-if = toolkit != 'cocoa'
> > +# This test is bogus according to the spec; see bug 407107
> > +[test_2d.path.rect.zero.6.html]
> > +disabled = bug 407107
> > +skip-if = toolkit != 'cocoa'
>
> this appears to be disabled and skipped on cocoa? that is confusing
Hm, I thought this was inside the OSX block in the Makefile and disabled in there, but apparently not. I'll remove the skip-if. I've been trying to port the conditionals from Makefiles as closely as possible, but I must have confused myself.
>
> ::: docshell/test/navigation/mochitest.ini
> @@ +20,5 @@
> > parent.html
> >
> > [test_bug13871.html]
> > +[test_bug270414.html]
> > +skip-if = buildapp == "mobile/android"
>
> could we just use toolkit=="android"?
We could, I've just been porting exactly what the Makefiles say right now. (Even if they're doing silly things.)
>
> ::: toolkit/components/places/tests/Makefile.in
> @@ -12,3 @@
> > # Simple MochiTests
> > MOCHITEST_FILES = \
> > mochitest/bug94514-postpage.html \
>
> we still have mochitest and chrome files in here
Those changes are in attachment 8373442 [details] [diff] [review].
>
> ::: toolkit/components/places/tests/mochitest/prompt_common.js
> @@ -1,1 @@
> > -
>
> why is this file removed? I believe it is still used!
Not AFAICT:
http://mxr.mozilla.org/mozilla-central/search?string=prompt_common.js
There are other files in the tree named prompt_common.js, and those are used:
http://mxr.mozilla.org/mozilla-central/find?text=&kind=text&string=prompt_common.js
Assignee | ||
Comment 10•11 years ago
|
||
I pushed these to try a few days ago, the results look good:
https://tbpl.mozilla.org/?tree=Try&rev=4ded4d2dc418
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Followup bustage fix, wound up conflicting with bug 969021 in a way that didn't break my local Linux build:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2dd850b0ca3
https://hg.mozilla.org/mozilla-central/rev/acefe67f49db
https://hg.mozilla.org/mozilla-central/rev/b2dd850b0ca3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•