Bug 26336 – Some test cases in LayoutTests/editing/execCommand ought to use dumpAsText tests instead

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Some test cases in /LayoutTests/editing/execCommand are using a pixel test when a simple dumpAsText test would suffice the purpose.  This slows down the LayoutTexts considerably.

[Ryosuke Niwa]

Created attachment 31184 [details]
Fixes the bug.  Most of test cases now use a dumpAsText test.

[Ryosuke Niwa]

I converted 21 test cases to use dumpAsText test.

[Eric Seidel (no email)]

Comment on attachment 31184 [details]
Fixes the bug.  Most of test cases now use a dumpAsText test.

I can't review 20 tests at once.  Please post these in smaller patches.  Ideally 20k or less.

[Ryosuke Niwa]

Created attachment 31381 [details]
converts 4916583.html 5119244.html 5120591.html

[Ryosuke Niwa]

Created attachment 31382 [details]
converts 5142012-3.html 5144139-1.html 5164796.html

[Ryosuke Niwa]

Created attachment 31383 [details]
converts 5207369.html 5210032.html 5432254-1.html

[Ryosuke Niwa]

Created attachment 31384 [details]
converts 5432254-2.html 5700414-1.html 5700414-2.html

[Ryosuke Niwa]

Created attachment 31385 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

[Ryosuke Niwa]

Created attachment 31387 [details]
converts insertHTML.html paste-1.html paste-2.html

[Ryosuke Niwa]

Created attachment 31388 [details]
converts remove-list-1.html remove-list-items.html switch-list-type.html

[Ryosuke Niwa]

last three patches are fairly large but these patches are mostly deletion of files.  so the actual context isn't that large.

[Eric Seidel (no email)]

Comment on attachment 31381 [details]
converts 4916583.html 5119244.html 5120591.html

Looks fine.

[Eric Seidel (no email)]

Comment on attachment 31382 [details]
converts 5142012-3.html 5144139-1.html 5164796.html

Looks OK.

[Ryosuke Niwa]

Created attachment 31432 [details]
converts 4916583.html 5119244.html 5120591.html

[Ryosuke Niwa]

Created attachment 31433 [details]
converts 4916583.html 5119244.html 5120591.html

[Ryosuke Niwa]

Created attachment 31434 [details]
converts 5142012-3.html 5144139-1.html 5164796.html

[Ryosuke Niwa]

Created attachment 31435 [details]
converts 5207369.html 5210032.html 5432254-1.html

[Ryosuke Niwa]

Created attachment 31436 [details]
converts 5432254-2.html 5700414-1.html 5700414-2.html

[Ryosuke Niwa]

Created attachment 31437 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

[Ryosuke Niwa]

Created attachment 31438 [details]
converts insertHTML.html switch-list-type.html

[Ryosuke Niwa]

Created attachment 31439 [details]
converts paste-1.html paste-2.html

[Ryosuke Niwa]

Created attachment 31440 [details]
converts remove-list-1.html remove-list-items.html

[Eric Seidel (no email)]

Comment on attachment 31433 [details]
converts 4916583.html 5119244.html 5120591.html

LGTM.

[Eric Seidel (no email)]

Comment on attachment 31434 [details]
converts 5142012-3.html 5144139-1.html 5164796.html

LGTM.

[Eric Seidel (no email)]

Comment on attachment 31435 [details]
converts 5207369.html 5210032.html 5432254-1.html

LGTM.

[Eric Seidel (no email)]

Comment on attachment 31436 [details]
converts 5432254-2.html 5700414-1.html 5700414-2.html

Typo in:
LayoutTests/editing/execCommand/5700414-2.html
 4     <div id="div">There should be be a single empty paragraph above.</div>

Otherwise looks fine.

[Eric Seidel (no email)]

Comment on attachment 31437 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

Tab:
 if (window.layoutTestController) {
 3     layoutTestController.dumpEditingCallbacks();
 4 	layoutTestController.dumpAsText();
 5 }


Is there no button?
5  <p>The three items below should be stitched together into one ordered list when you click the button.</div>

[Eric Seidel (no email)]

Comment on attachment 31438 [details]
converts insertHTML.html switch-list-type.html

Looks OK.  Sad we can't pretty-print this:
 36 <ol style="border: 1px solid red;"> <li>This should be in an ordered list.</li> </ol><span id="item1"><ul><li><span id="item1">This should be in an unordered list.</span>&nbsp;</li></ul></span><ol style="border: 1px solid red;"><br> This should be in an ordered list. <li>This should be in an ordered list.</li> </ol><ul><li>This should be in an unordered list.</li></ul><ol style="border: 1px solid red;"><li>This should be in an ordered list.</li> </ol><ul><li>This should be in an unordered list.</li></ul><ol style="border: 1px solid red;"> <li>This should be in an ordered list.</li> </ol> This should not be in a list.<br><ol><li>This should be in an ordered list.</li></ol>

[Eric Seidel (no email)]

Comment on attachment 31439 [details]
converts paste-1.html paste-2.html

Tabs:
0     if (window.layoutTestController)
20          window.layoutTestController.notifyDone();
 31 		window.layoutTestController.notifyDone();

Missing newline:
2539 </script>
2640 \ No newline at end of file

Otherwise looks fine.  The tab will block checkin (the pre-commit hook will fail).

[Darin Adler]

(In reply to comment #28)
> Sad we can't pretty-print this:
>  36 <ol style="border: 1px solid red;"> <li>This should be in an ordered
> list.</li> </ol><span id="item1"><ul><li><span id="item1">This should be in an
> unordered list.</span>&nbsp;</li></ul></span><ol style="border: 1px solid
> red;"><br> This should be in an ordered list. <li>This should be in an ordered
> list.</li> </ol><ul><li>This should be in an unordered list.</li></ul><ol
> style="border: 1px solid red;"><li>This should be in an ordered list.</li>
> </ol><ul><li>This should be in an unordered list.</li></ul><ol style="border:
> 1px solid red;"> <li>This should be in an ordered list.</li> </ol> This should
> not be in a list.<br><ol><li>This should be in an ordered list.</li></ol>

But we can! There's no reason we just have to log innerHTML. We could use a couple regular expressions to make it easier to read. I would if I was writing a test with this as the result.

[Eric Seidel (no email)]

Comment on attachment 31440 [details]
converts remove-list-1.html remove-list-items.html

Tab:
3     layoutTestController.dumpEditingCallbacks();
 4 	layoutTestController.dumpAsText();


Why?
  <div id="div" contenteditable="true"><ol><li></li><li>There should be a empty paragraph above this one.</li></ol></div>
 8 <div id="div" contenteditable="true"><ol><li></li><li>There should be a single BR above this line and no OL or LI.</li></ol></div>

[Ryosuke Niwa]

(In reply to comment #31)
> Why?
>   <div id="div" contenteditable="true"><ol><li></li><li>There should be a empty
> paragraph above this one.</li></ol></div>
>  8 <div id="div" contenteditable="true"><ol><li></li><li>There should be a
> single BR above this line and no OL or LI.</li></ol></div>
> 

Because if we simply deleted LI tags, then there will be no content above the sentence; i.e. if acts as if we set display:none; for the first LI.  But since we're just untoggling ordered list, there should still be a space allocated for it.  As a place holder, we insert a BR.

[Ryosuke Niwa]

(In reply to comment #27)
> Is there no button?
> 5  <p>The three items below should be stitched together into one ordered list
> when you click the button.</div>

As far as I checked, there was no button.

[Ryosuke Niwa]

Created attachment 31458 [details]
converts 5432254-2.html 5700414-1.html 5700414-2.html

[Ryosuke Niwa]

Created attachment 31459 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

[Ryosuke Niwa]

Created attachment 31460 [details]
converts paste-1.html paste-2.html

[Ryosuke Niwa]

Created attachment 31461 [details]
converts remove-list-1.html remove-list-items.html

[Ryosuke Niwa]

Created attachment 31462 [details]
converts paste-1.html paste-2.html

[Ryosuke Niwa]

Created attachment 31464 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

[Julie Parent]

Committed first 4 patches as:
http://trac.webkit.org/changeset/44824
http://trac.webkit.org/changeset/44823
http://trac.webkit.org/changeset/44822
http://trac.webkit.org/changeset/44830

[Eric Seidel (no email)]

CCing oliver.  He expressed interest in this patch set this morning.

[Eric Seidel (no email)]

Comment on attachment 31458 [details]
converts 5432254-2.html 5700414-1.html 5700414-2.html

Looks fine.

[Eric Seidel (no email)]

Comment on attachment 31461 [details]
converts remove-list-1.html remove-list-items.html

Looks fine.

[Eric Seidel (no email)]

Comment on attachment 31462 [details]
converts paste-1.html paste-2.html

Looks fine.

[Eric Seidel (no email)]

Comment on attachment 31464 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

Looks fine.

[Eric Seidel (no email)]

Which ones of these have already been landed?  It would be best if they were already obsoleted so that the rest of these could be more easily landed.

[Ryosuke Niwa]

(In reply to comment #46)
> Which ones of these have already been landed?  It would be best if they were
> already obsoleted so that the rest of these could be more easily landed.
> 

obsolated 4 patches that have already been landed.

[Eric Seidel (no email)]

Comment on attachment 31458 [details]
converts 5432254-2.html 5700414-1.html 5700414-2.html

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.checksum
	D	LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.png
	D	LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.txt
	D	LayoutTests/platform/mac/editing/execCommand/5700414-1-expected.checksum
	D	LayoutTests/platform/mac/editing/execCommand/5700414-1-expected.png
	D	LayoutTests/platform/mac/editing/execCommand/5700414-1-expected.txt
	D	LayoutTests/platform/mac/editing/execCommand/5700414-2-expected.checksum
	D	LayoutTests/platform/mac/editing/execCommand/5700414-2-expected.png
	D	LayoutTests/platform/mac/editing/execCommand/5700414-2-expected.txt
	D	LayoutTests/platform/qt/editing/execCommand/5432254-2-expected.txt
	M	LayoutTests/ChangeLog
	A	LayoutTests/editing/execCommand/5432254-2-expected.txt
	M	LayoutTests/editing/execCommand/5432254-2.html
	A	LayoutTests/editing/execCommand/5700414-1-expected.txt
	M	LayoutTests/editing/execCommand/5700414-1.html
	A	LayoutTests/editing/execCommand/5700414-2-expected.txt
	M	LayoutTests/editing/execCommand/5700414-2.html
Committed r45224

[Eric Seidel (no email)]

Comment on attachment 31461 [details]
converts remove-list-1.html remove-list-items.html

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	R	LayoutTests/platform/mac/editing/execCommand/remove-list-1-expected.txt => LayoutTests/editing/execCommand/remove-list-1-expected.txt
	D	LayoutTests/platform/mac/editing/execCommand/remove-list-1-expected.checksum
	D	LayoutTests/platform/mac/editing/execCommand/remove-list-1-expected.png
	D	LayoutTests/platform/mac/editing/execCommand/remove-list-items-expected.checksum
	D	LayoutTests/platform/mac/editing/execCommand/remove-list-items-expected.png
	D	LayoutTests/platform/mac/editing/execCommand/remove-list-items-expected.txt
	D	LayoutTests/platform/qt/editing/execCommand/remove-list-1-expected.txt
	D	LayoutTests/platform/qt/editing/execCommand/remove-list-items-expected.txt
	M	LayoutTests/ChangeLog
	M	LayoutTests/editing/execCommand/remove-list-1.html
	A	LayoutTests/editing/execCommand/remove-list-items-expected.txt
	M	LayoutTests/editing/execCommand/remove-list-items.html
Committed r45239
http://trac.webkit.org/changeset/45239

[Eric Seidel (no email)]

Comment on attachment 31462 [details]
converts paste-1.html paste-2.html

Tests fail after conversion:

editing/execCommand/paste-1.html -> failed
.
editing/execCommand/paste-2.html -> failed

[Eric Seidel (no email)]

Comment on attachment 31464 [details]
converts find-after-replace.html findString-2.html insert-list-and-stitch.html

After applying the patch:
editing/execCommand/find-after-replace.html -> failed

[Eric Seidel (no email)]

Given that we seem to want to take a different approach for these.  I think we can just close this bug now.  The last two patches which failed, there seems little point in updating them.  Feel-free to re-open if you disagree.

[Ryosuke Niwa]

(In reply to comment #52)
> Given that we seem to want to take a different approach for these.  I think we
> can just close this bug now.  The last two patches which failed, there seems
> little point in updating them.  Feel-free to re-open if you disagree.
> 

I think those are failing because my patch for 14062.  I see weird nbsp's in the expected results for paste-1/2 & find-after-replace.  It's interesting that these bug didn't show up in layout tests because a regular space and non-breaking space are rendered same way in many cases.