Bug 31580

Summary: Rewrite the test fast/parser/comment-in-textarea to use dumpAsText
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: Tools / TestsAssignee: Kinuko Yasuda <kinuko>
Status: RESOLVED FIXED    
Severity: Minor CC: commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText
none
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)
none
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)
none
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js)
none
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js and updated ChangeLog)
none
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js) none

Description Kinuko Yasuda 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.
Comment 1 Kinuko Yasuda 2009-11-17 03:18:43 PST
Created attachment 43352 [details]
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText
Comment 2 Dmitry Titov 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.
Comment 3 Kinuko Yasuda 2009-11-18 00:37:14 PST
Created attachment 43410 [details]
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)
Comment 4 Darin Adler 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.
Comment 5 Eric Seidel (no email) 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.
Comment 6 Eric Seidel (no email) 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.
Comment 7 Kinuko Yasuda 2009-11-18 21:00:09 PST
Created attachment 43481 [details]
Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)
Comment 8 Kinuko Yasuda 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.
Comment 9 Dmitry Titov 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!
Comment 10 Kinuko Yasuda 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)
Comment 11 Kinuko Yasuda 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)
Comment 12 Darin Adler 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.
Comment 13 Kinuko Yasuda 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-
Comment 14 Kinuko Yasuda 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)
Comment 15 Eric Seidel (no email) 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.
Comment 16 WebKit Commit Bot 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>
Comment 17 WebKit Commit Bot 2009-11-26 21:53:18 PST
All reviewed patches have been landed.  Closing bug.