Closed Bug 746915 Opened 8 years ago Closed 8 years ago

Move editor-related tests to editor/


(Core :: DOM: Editor, enhancement)

Not set





(Reporter: ayg, Assigned: ayg)



(2 files)

We have some editing-related mochitests in content/html/document/test/, and the reftests are mostly under layout/ for some reason (see bug 279330 comment 26).  At the very least, we should move all the editing-related mochitests to editor/.  I don't see why we shouldn't move layout/reftests/editor/ to editor/reftests/ too -- that way to run all editor-related tests is just

  TEST_PATH=editor make -C $(OBJDIR) mochitest-plain
  TEST_PATH=editor make -C $(OBJDIR) crashtest
  TEST_PATH=editor make -C $(OBJDIR) reftest

But I'll write that as a second, separate patch in case you'd prefer to keep them under layout/ for some reason.
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 616495, 616496
	Branch: mozilla-central => try
Try run started, revision 8b75c3514ea3. To cancel or monitor the job, see:
Try run for 8b75c3514ea3 is complete.
Detailed breakdown of the results available here:
Results (out of 220 total builds):
    success: 182
    warnings: 38
Builds (or logs if builds failed) available at:
Whiteboard: [autoland-in-queue]
Comment on attachment 616495 [details] [diff] [review]
Patch part 1, v1 -- Move editing-related tests to editor/ directories

This is awesome!
Attachment #616495 - Flags: review?(ehsan) → review+
Comment on attachment 616496 [details] [diff] [review]
Patch part 2, v1 -- Move layout/reftests/editor/ to editor/reftests/

I'm not sure how much we care about putting most reftests under layout.  There are components which do this, but I always thought that the idea is to keep reftests close to each other.  If dbaron is fine with this, I'm fine as well.
Attachment #616496 - Flags: review?(ehsan) → review?(dbaron)
I can find


Seems pretty popular.  I think it's quite surprising to have tests unrelated to layout under layout/.
Comment on attachment 616496 [details] [diff] [review]
Patch part 2, v1 -- Move layout/reftests/editor/ to editor/reftests/

>diff --git a/layout/reftests/reftest.list b/layout/reftests/reftest.list
>--- a/layout/reftests/reftest.list
>+++ b/layout/reftests/reftest.list
>@@ -127,17 +127,17 @@ include counters/reftest.list
> # datalist
> include datalist/reftest.list
> # dom/
> include dom/reftest.list
> # editor/
>-include editor/reftest.list
>+include ../../editor/reftests/reftest.list
> # generated-content/
> include generated-content/reftest.list
> # first-letter/
> skip-if(Android) include first-letter/reftest.list
> # first-line/

Please move these 3 lines lower, someplace appropriate near the bottom of the list so it stays sorted reasonably.

r=dbaron with that
Attachment #616496 - Flags: review?(dbaron) → review+
Sorry -- which three lines?
Comment on attachment 616496 [details] [diff] [review]
Patch part 2, v1 -- Move layout/reftests/editor/ to editor/reftests/

Sorry, I didn't understand your review comment -- which three lines?
Attachment #616496 - Flags: review+ → review?(dbaron)
> # editor/
>-include editor/reftest.list
>+include ../../editor/reftests/reftest.list
Attachment #616496 - Flags: review?(dbaron) → review+
Okay, so I still don't understand -- how is it not sorted reasonably?  editor/ comes after dom/ and before generated-content/, which is roughly where it should go.  If anything, generated-content/ should be moved down between forms and gfx.  Where do you want me to move the lines?  Sorry for being slow!
Oh, the stuff that begins with ".." is down at the bottom, separate from the subdirectories of layout/reftests/ which are at the top (and where it's currently listed).
I don't see them at the bottom.  I see them sorted alphabetically.  Like:

# columns/
include columns/reftest.list

# content/
include ../../content/test/reftest/reftest.list

# counters/
include counters/reftest.list


# forms
skip-if(Android) include forms/reftest.list

# gfx
include ../../gfx/tests/reftest/reftest.list

# block-inside-inline splits
include ib-split/reftest.list


# native-theme/
skip-if(Android) include native-theme/reftest.list

# netwerk/
include ../../netwerk/test/reftest/reftest.list

# object/
include object/reftest.list

# ogg-video/
include ogg-video/reftest.list

# webm-video/
include webm-video/reftest.list

# parser/
include ../../parser/htmlparser/tests/reftest/reftest.list

# percent-overflow-sizing/
include percent-overflow-sizing/reftest.list

# pixel-rounding/
include pixel-rounding/reftest.list

# plugin/
include ../../dom/plugins/test/reftest/reftest.list

# printing
include printing/reftest.list
include pagination/reftest.list


# text-shadow/
skip-if(Android) include text-shadow/reftest.list

# theme (pinstripe)
include ../../toolkit/themes/pinstripe/reftests/reftest.list

# text-transform/
include text-transform/reftest.list


# unicode/ (verify that we don't do expend effort doing unicode-aware case checks)
include unicode/reftest.list

# widget/
include ../../widget/reftests/reftest.list

# xml-stylesheet/
include ../../content/test/reftest/xml-stylesheet/reftest.list

# xul-document-load/
include xul-document-load/reftest.list

# xul/
include xul/reftest.list

# xul
include ../xul/base/reftest/reftest.list

# xul grid
include ../xul/base/src/grid/reftests/reftest.list

# z-index/
include z-index/reftest.list

The only paths starting ../ that aren't in some type of alphabetical order are ../../image/test/reftest/reftest.list at the very top, and ../../content/html/{document,content}/reftests/reftests.list at the very bottom.  All the others are alphabetical, as in this patch.
ok, either way, but I'd prefer it at the bottom
You need to log in before you can comment on or make changes to this bug.