Closed Bug 368994 Opened 13 years ago Closed 3 years ago

move mochitest tests near the code they test in the tree

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a2

People

(Reporter: sayrer, Assigned: sayrer)

Details

Attachments

(3 files, 18 obsolete files)

45.84 KB, patch
Details | Diff | Splinter Review
5.61 KB, text/plain
Details
138.17 KB, patch
sayrer
: review+
Details | Diff | Splinter Review
I'll move them all first, and then cvs delete in testing/mochitest/tests when complete.
Attachment #253631 - Attachment is obsolete: true
Attachment #253632 - Flags: review?(bzbarsky)
Attached patch now with index.html removals (obsolete) — Splinter Review
Attachment #253632 - Attachment is obsolete: true
Attachment #253633 - Flags: review?(bzbarsky)
Attachment #253632 - Flags: review?(bzbarsky)
Comment on attachment 253633 [details] [diff] [review]
now with index.html removals

Looks ok.  The test for bug 1682 is incorrect, but you're just moving it anyway, I guess....
Attachment #253633 - Flags: review?(bzbarsky) → review+
Attached patch second batch (obsolete) — Splinter Review
Attachment #253633 - Attachment is obsolete: true
Attachment #253662 - Flags: review?
Attached patch second batch (obsolete) — Splinter Review
Attachment #253662 - Attachment is obsolete: true
Attachment #253662 - Flags: review?
Attachment #253663 - Flags: review?(jonas)
Comment on attachment 253663 [details] [diff] [review]
second batch

r/sr=sicking assuming this is just moving the files.
Attachment #253663 - Flags: superreview+
Attachment #253663 - Flags: review?(jonas)
Attachment #253663 - Flags: review+
Attached patch move bug 100533 (obsolete) — Splinter Review
Attached patch move bug 100533 (obsolete) — Splinter Review
Attachment #253880 - Attachment is obsolete: true
Attachment #253881 - Flags: review?(bzbarsky)
Comment on attachment 253881 [details] [diff] [review]
move bug 100533

r=bzbarsky
Attachment #253881 - Flags: review?(bzbarsky) → review+
Attached patch third batch (obsolete) — Splinter Review
I had to comment out one test line in test_bug277724.html--will file a follow up
Attached patch third batch (obsolete) — Splinter Review
Attachment #253962 - Attachment is obsolete: true
Attachment #253963 - Flags: review?(bzbarsky)
Comment on attachment 253963 [details] [diff] [review]
third batch

>Index: content/html/content/test/Makefile.in
>+		test_bug218277.html \
>+		test_bug237071.html \
>+		test_bug238409.html \

So what are our criteria for categorizing?  There's no content/html/content code involved in bug 218277 or bug 238409, really....

>Index: content/html/document/test/Makefile.in
>+		test_bug277724.html \
>+		bug277724_iframe1.html \
>+		bug277724_iframe2.xhtml \

This test, on the other hand, is all about content/html/content stuff.
> bug 218277 or bug 238409, really....

ok, will move to base.

> test_bug277724.html

it had changes to the content sink, so I figured it was document/ stuff.

> ok, will move to base.

Sounds good.

> it had changes to the content sink

Those were pretty incidental.
Attached patch third batch (obsolete) — Splinter Review
Attachment #253963 - Attachment is obsolete: true
Attachment #253963 - Flags: review?(bzbarsky)
Attachment #253995 - Flags: review?(bzbarsky)
Comment on attachment 253995 [details] [diff] [review]
third batch

Looks great!
Attachment #253995 - Flags: review?(bzbarsky) → review+
Attached patch fourth batch (obsolete) — Splinter Review
Attached patch fourth batch (obsolete) — Splinter Review
Attachment #254069 - Attachment is obsolete: true
Attachment #254070 - Flags: review?(bzbarsky)
Comment on attachment 254070 [details] [diff] [review]
fourth batch

r=bzbarsky
Attachment #254070 - Flags: review?(bzbarsky) → review+
Attached patch fifth batch (obsolete) — Splinter Review
Attachment #255811 - Flags: review?(dbaron)
Comment on attachment 255811 [details] [diff] [review]
fifth batch

Seems like the test for bug 302186 should also be in layout/style/test, not content/html/content/test.    And you're removing the files from their old locations, right?  r=dbaron with those changes (or if you convince me otherwise)
Attachment #255811 - Flags: review?(dbaron) → review+
(In reply to comment #22)
> (From update of attachment 255811 [details] [diff] [review])
> Seems like the test for bug 302186 should also be in layout/style/test, not
> content/html/content/test.    And you're removing the files from their old
> locations, right?  r=dbaron with those changes (or if you convince me
> otherwise)

the patch in bug 302186 is almost entirely composed of code in content/html.
Attached patch sixth batch (obsolete) — Splinter Review
Attached patch sixth batch (obsolete) — Splinter Review
Comment on attachment 260390 [details] [diff] [review]
sixth batch

I changed test_bug359657.html to waitForExplicitFinish.
Attachment #260390 - Flags: review?(bzbarsky)
Attachment #260389 - Attachment is obsolete: true
Comment on attachment 260390 [details] [diff] [review]
sixth batch

sr=bzbarsky
Attachment #260390 - Flags: review?(bzbarsky) → review+
Er, r=bzbarsky is what I meant.
Attached patch batch 7 (obsolete) — Splinter Review
not sure I got all of these in the right location, but this is probably the best way to find out.
Attachment #253663 - Attachment is obsolete: true
Attachment #253881 - Attachment is obsolete: true
Attachment #253995 - Attachment is obsolete: true
Attachment #254070 - Attachment is obsolete: true
Attachment #255811 - Attachment is obsolete: true
Attachment #260390 - Attachment is obsolete: true
Attachment #261615 - Flags: review?(bzbarsky)
Comment on attachment 261615 [details] [diff] [review]
batch 7

>Index: content/html/document/test/Makefile.in

>+		test_bug311681.xml \

That belongs in either content/base or content/xml/document.

I'd prefer the former, I think.

>Index: content/xul/document/test/Makefile.in
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@

Weird spacing.

>+_TEST_FILES = \
>+    test_bug311681.xul \
>+		$(NULL)

Weird indent.  Use tabs?

>Index: layout/generic/Makefile.in

>+ifdef MOZ_MOCHITEST
>+DIRS  = test
>+endif

Looks like I checked this in sometime between when you pulled and now...  Undo this change, since that dir exists now and is built?

>Index: layout/generic/test/Makefile.in

And be careful merging this?

r=bzbarsky with that.
Attachment #261615 - Flags: review?(bzbarsky) → review+
Attached patch batch 7 to check in (obsolete) — Splinter Review
Attachment #261615 - Attachment is obsolete: true
Attached patch batch 7 to check in (obsolete) — Splinter Review
Attachment #261682 - Attachment is obsolete: true
Attachment #261683 - Attachment is obsolete: true
I think this is fixed now.
Status: NEW → RESOLVED
Closed: 11 years ago
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
Resolution: --- → FIXED
Version: Trunk → unspecified
There are plenty of test_bug* in testing/mochitest still.  Maybe they got copied elsewhere, in which case we should delete them from here.  Needs to be checked on a test-by-test basis.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
All these files have been moved.
Attachment #422366 - Flags: review?(sayrer)
Attachment #422366 - Flags: review?(sayrer) → review+
(In reply to comment #37)
> Created an attachment (id=422366) [details]
> Removing moved files

4 files don't remove cleanly.
Whiteboard: [c-n: needs updated patch]
Serge: the patch applies cleanly here. Which files cause problems for you?
(In reply to comment #39)
> Serge: the patch applies cleanly here. Which files cause problems for you?

Indeed: it looks like notepad converted the "few" CRLF to LF when I edited the patch :-< Sorry about that.
Whiteboard: [c-n: needs updated patch] → [c-n: fwiw, note that 4 of the files have CRLF.]
Comment on attachment 422366 [details] [diff] [review]
Removing moved files
[Checkin: Comment 42]


http://hg.mozilla.org/mozilla-central/rev/583d9880da65
Attachment #422366 - Attachment description: Removing moved files → Removing moved files [Checkin: Comment 42]
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: fwiw, note that 4 of the files have CRLF.]
Target Milestone: --- → mozilla1.9.3a2
Hum, there's 2 remaining tests to move:
not sure why they were left in place, but anyway.

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/index.html?force=1&mark=47-48#43
{
47 'test_bug362788.xhtml',
48 'test_bug366645.xhtml'
}

They should be run(!) within the m-plain (I assume) test suite.
If for some reasons that's not possible, then index.html should move along to a manual directory or the like.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like the tests mentioned in comment 43 have been removed from the tree sometime in the last 7 years. Re-closing.
Status: REOPENED → RESOLVED
Closed: 10 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.