WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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.
Top of Page
Format For Printing
XML
Clone This Bug