31580 – Rewrite the test fast/parser/comment-in-textarea to use dumpAsText

RESOLVED FIXED 31580

Rewrite the test fast/parser/comment-in-textarea to use dumpAsText

https://bugs.webkit.org/show_bug.cgi?id=31580

Summary Rewrite the test fast/parser/comment-in-textarea to use dumpAsText

Kinuko Yasuda

Reported 2009-11-17 03:16:32 PST

The layout test fast/parser/comment-in-textarea.html can be written using dumpAsText so that the results can be independent of minor platform differences.

Attachments
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (9.98 KB, patch) 2009-11-17 03:18 PST, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) (9.90 KB, patch) 2009-11-18 00:37 PST, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) (10.00 KB, patch) 2009-11-18 21:00 PST, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js) (10.68 KB, patch) 2009-11-18 21:58 PST, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js and updated ChangeLog) (10.74 KB, patch) 2009-11-18 22:00 PST, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js) (10.88 KB, patch) 2009-11-25 02:24 PST, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Kinuko Yasuda

Comment 1 2009-11-17 03:18:43 PST

Created attachment 43352 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText

Dmitry Titov

Comment 2 2009-11-18 00:21:42 PST

Comment on attachment 43352 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText Yeah! More 'dumpAsText' tests! Few nits: > <html> > -<body> > -<textarea> > +<head> <head> doesn't really add to the test, could be removed. > + <script> > + function log(msg) the <script> tag with content is usually not indented. > + } > + function test() Could be nice to have an empty line between these. r- because if you fix those, we'll just run it through commit-bot.

Kinuko Yasuda

Comment 3 2009-11-18 00:37:14 PST

Created attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)

Darin Adler

Comment 4 2009-11-18 10:00:20 PST

Comment on attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) > +<div id="console"></div> > +<script> > +function log(msg) > +{ > + document.getElementById('console').appendChild(document.createTextNode(msg + '\n')); > +} I'm not sure why you're including a newline in a text node you're putting inside a <div>. The newline will just be treated the same way as any other whitespace. It won't cause the next item to move to a new line.

Eric Seidel (no email)

Comment 5 2009-11-18 13:49:16 PST

Comment on attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) I agree with Darin, your log() function doens't really do what it intends to do. You might as well just use the log/shouldBe stuff provided by fast/js/resources/js-test-pre.js See all the other TEMPLATE.html files and http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree You don't actually have to make a TEMPLATE.html and use real script-tests to include the script test headers. You can just modify this test to include the necessary <div id='console'> and the two script tags (like every template does) and you're set. Or you could convert the whole thing to a script-test (with or w/o a custom template). If you don't use a custom template, you'd need to use innerHTML in order to test the parser.

Eric Seidel (no email)

Comment 6 2009-11-18 13:50:57 PST

Comment on attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) r- because it's kinda silly to add a log() function which doesn't work as intended. You don't need to do any of the conversion I suggested above, just fix log() and I'm happy to r+. You should definitely familiarize yourself with js-test-pre.js and script-tests as you'll undoubtably use them in the future.

Kinuko Yasuda

Comment 7 2009-11-18 21:00:09 PST

Created attachment 43481 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)

Kinuko Yasuda

Comment 8 2009-11-18 21:10:38 PST

Thanks for reviewing. The code was a bit careless and wasn't using standard js test functions (even though I've used them before), sorry for that. Updated the patch with TEMPLATES/js-test-pre.js.

Dmitry Titov

Comment 9 2009-11-18 21:41:15 PST

Comment on attachment 43481 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) This patch looks good but does not include script-tests/comment-in-textarea.js. Very close!

Kinuko Yasuda

Comment 10 2009-11-18 21:58:34 PST

Created attachment 43485 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js)

Kinuko Yasuda

Comment 11 2009-11-18 22:00:44 PST

Created attachment 43486 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js and updated ChangeLog)

Darin Adler

Comment 12 2009-11-19 09:50:48 PST

Comment on attachment 43486 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js and updated ChangeLog) This no longer tests the original bug. That bug had to do with the HTML parser when in textarea mode, but I don't think setting the innerHTML of a textarea is sufficient. Instead I think you need to set innerHTML of some other element and include a <textarea> element in the markup. That's the danger of rewriting a test. One of the things about the original test is that it was run before the bug was fixed, and so we know it fails if the bug exists. But to do that now you'd have to find a way to roll out the original fix and reintroduce the bug to double check that it still tests the bug. I think you'd be OK if you created some other kind of element and used innerHTML with markup that includes the "<textarea>" and "</textarea>", not just the comment.

Kinuko Yasuda

Comment 13 2009-11-25 02:19:56 PST

(In reply to comment #12) > (From update of attachment 43486 [details]) > This no longer tests the original bug. That bug had to do with the HTML parser > when in textarea mode, but I don't think setting the innerHTML of a textarea is > sufficient. Instead I think you need to set innerHTML of some other element and > include a <textarea> element in the markup. > > That's the danger of rewriting a test. One of the things about the original > test is that it was run before the bug was fixed, and so we know it fails if > the bug exists. But to do that now you'd have to find a way to roll out the > original fix and reintroduce the bug to double check that it still tests the > bug. > > I think you'd be OK if you created some other kind of element and used > innerHTML with markup that includes the "<textarea>" and "</textarea>", not > just the comment. Thanks for your careful review. For later changes I'll try reading the original bug report (probably bug 19597 in this case?) and try understand what needs to be tested. I'm updating the patch based on your comment-

Kinuko Yasuda

Comment 14 2009-11-25 02:24:43 PST

Created attachment 43834 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js)

Eric Seidel (no email)

Comment 15 2009-11-26 21:35:12 PST

Comment on attachment 43834 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js) Looks right to me.

WebKit Commit Bot

Comment 16 2009-11-26 21:53:10 PST

Comment on attachment 43834 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js) Clearing flags on attachment: 43834 Committed r51427: <http://trac.webkit.org/changeset/51427>

WebKit Commit Bot

Comment 17 2009-11-26 21:53:18 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Minor

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS OS X 10.5

Product WebKit

Component Tools / Tests

Assignee

Kinuko Yasuda

Reported

2009-11-17 03:16 PST

Modified

2009-11-26 21:53 PST History

CC List

2 users Show

URL

Keywords

Depends on

Blocks