| Summary: | Convert LayoutTests/fast/events/mouseout-dead-node.html from pixel test to text-based test | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Naoki Takano <honten> | ||||||||||||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, dbates, eric, rniwa, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Naoki Takano
2011-04-25 16:01:35 PDT
Created attachment 90975 [details]
Converted to use dumpAsText()
Please review.
Oops, forgot to write ChangeLog... Created attachment 91004 [details]
Add ChangeLog and remove unneeded files.
Please review.
Created attachment 91005 [details]
Remove Japanese char.
Removed Japanese chars.
Comment on attachment 91005 [details] Remove Japanese char. View in context: https://bugs.webkit.org/attachment.cgi?id=91005&action=review We can further improve this patch. I suggest we use the LayoutTests/fast/js/resources/js-test-pre.js and its testPassed(), testFailed() functions instead of out(). See <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html> for an example of using js-test-pre.js. Also, the paths in the this patch aren't from the the top-level project directory. It looks like the patch was created from within the LayoutTests directory. As you can extrapolate from the EWS bot output (e.g. <https://webkit-commit-queue.appspot.com/results/8505750>), the commit queue is not smart enough to apply such a patch. So, it would need to be landed by hand. Instead, I suggest you generate your patch from the top-level WebKit directory. > fast/events/mouseout-dead-node-expected.txt:3 > +you should see PASS below > +you should see PASS below Can we remove this text when this test is run using Dump Render Tree so as to simplify the result output? See <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html> for an example of this. In particular, see <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html#L181> where we remove the test container so that we only leave the test description and test results in the output. > fast/events/mouseout-dead-node.html:1 > -<body onload='test()'> > +<body onload='autoTest()'> No need to rename this. This name of the function is sufficiently descriptive as-is. > fast/events/mouseout-dead-node.html:-26 > - if (window.eventSender) { We shouldn't change this to layoutTestController since the code within this block uses aspects of the EventSender object. Although, in practice the existence of the EventSender object can be seen as equivalent to the presence of the LayoutTestController object ;=> we are running the test under Dump Render Tree, I suggest we instead only use LayoutTestController and EventSender functionality when we know such objects exist, respectively. For example, we should check for the presence of the EventSender object before using eventSender.mouseMoveTo(). > fast/events/mouseout-dead-node.html:26 > + if (window.layoutTestController) { > + layoutTestController.dumpAsText(); I would hoist this if-statement outside of this function so that it's at the top of the <script> body. Thank you for your review, Daniel. I'll do that. (In reply to comment #5) > (From update of attachment 91005 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91005&action=review > > We can further improve this patch. > > I suggest we use the LayoutTests/fast/js/resources/js-test-pre.js and its testPassed(), testFailed() functions instead of out(). See <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html> for an example of using js-test-pre.js. > > Also, the paths in the this patch aren't from the the top-level project directory. It looks like the patch was created from within the LayoutTests directory. As you can extrapolate from the EWS bot output (e.g. <https://webkit-commit-queue.appspot.com/results/8505750>), the commit queue is not smart enough to apply such a patch. So, it would need to be landed by hand. Instead, I suggest you generate your patch from the top-level WebKit directory. > > > fast/events/mouseout-dead-node-expected.txt:3 > > +you should see PASS below > > +you should see PASS below > > Can we remove this text when this test is run using Dump Render Tree so as to simplify the result output? > > See <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html> for an example of this. In particular, see <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/drag-and-drop.html#L181> where we remove the test container so that we only leave the test description and test results in the output. > > > fast/events/mouseout-dead-node.html:1 > > -<body onload='test()'> > > +<body onload='autoTest()'> > > No need to rename this. This name of the function is sufficiently descriptive as-is. > > > fast/events/mouseout-dead-node.html:-26 > > - if (window.eventSender) { > > We shouldn't change this to layoutTestController since the code within this block uses aspects of the EventSender object. Although, in practice the existence of the EventSender object can be seen as equivalent to the presence of the LayoutTestController object ;=> we are running the test under Dump Render Tree, I suggest we instead only use LayoutTestController and EventSender functionality when we know such objects exist, respectively. For example, we should check for the presence of the EventSender object before using eventSender.mouseMoveTo(). > > > fast/events/mouseout-dead-node.html:26 > > + if (window.layoutTestController) { > > + layoutTestController.dumpAsText(); > > I would hoist this if-statement outside of this function so that it's at the top of the <script> body. Created attachment 91175 [details]
According to the review, change the test and update the expected result.
Please review again.
Comment on attachment 91175 [details]
According to the review, change the test and update the expected result.
Looks good. Thanks Naoki for the fix!
Comment on attachment 91175 [details]
According to the review, change the test and update the expected result.
Could you commit?
I'm not yet committer;-)
Comment on attachment 91175 [details] According to the review, change the test and update the expected result. Rejecting attachment 91175 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2 Last 500 characters of output: hing file LayoutTests/platform/chromium-win/fast/events/mouseout-dead-node-expected.txt rm 'LayoutTests/platform/chromium-win/fast/events/mouseout-dead-node-expected.txt' patching file LayoutTests/fast/events/mouseout-dead-node-expected.txt patching file LayoutTests/fast/events/mouseout-dead-node.html patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Daniel Bates', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8514095 Created attachment 91235 [details]
Patch
Comment on attachment 91235 [details]
Patch
Hmm...
It looks applying patch failed.
I made the previous patch with svn manually.
The reason of the failure was to try to remove checksum files.
So I make new patch again with webkit-patch without those files.
Please review and commit again.
Please commit again.
Comment on attachment 91235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91235&action=review > LayoutTests/fast/events/mouseout-dead-node.html:1 > +<link rel="stylesheet" href="../js/resources/js-test-style.css"> No DOCTYPE / head? > LayoutTests/fast/events/mouseout-dead-node.html:4 > <body onload='test()'> I don't think there's any need to wait until page load. > LayoutTests/fast/events/mouseout-dead-node.html:15 > + <div id=d0 style='border:2px solid red'> > + <div onmouseout='testFailed("mouseout")' onmouseover='document.getElementById("d0").innerHTML ="you should see PASS below"'> > + <div onmouseout='testFailed("mouseout")'> > + <span id=target1 onmouseout='testPassed("mouseout")' > > + mouse over me > + </span> > + </div> I don't think we need to indent each element like this. > LayoutTests/fast/events/mouseout-dead-node.html:32 > function test() { You don't need this function. You can just run the script as it's parsed. > LayoutTests/fast/events/mouseout-dead-node.html:54 > +description("Test that if node dies under mouse it receives mouseout event but that the event does not propagate."); Please move this above the test code so that people reading the test code can understand the intent of this test before reading the code. Thank you for your review every time!! (In reply to comment #13) > (From update of attachment 91235 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91235&action=review > > > LayoutTests/fast/events/mouseout-dead-node.html:1 > > +<link rel="stylesheet" href="../js/resources/js-test-style.css"> > > No DOCTYPE / head? I'll add it. > > LayoutTests/fast/events/mouseout-dead-node.html:4 > > <body onload='test()'> > > I don't think there's any need to wait until page load. > > > LayoutTests/fast/events/mouseout-dead-node.html:15 > > + <div id=d0 style='border:2px solid red'> > > + <div onmouseout='testFailed("mouseout")' onmouseover='document.getElementById("d0").innerHTML ="you should see PASS below"'> > > + <div onmouseout='testFailed("mouseout")'> > > + <span id=target1 onmouseout='testPassed("mouseout")' > > > + mouse over me > > + </span> > > + </div> > > I don't think we need to indent each element like this. Ok, so do I have to erase whole indent for html element? > > LayoutTests/fast/events/mouseout-dead-node.html:32 > > function test() { > > You don't need this function. You can just run the script as it's parsed. I will erase onload. > > LayoutTests/fast/events/mouseout-dead-node.html:54 > > +description("Test that if node dies under mouse it receives mouseout event but that the event does not propagate."); > > Please move this above the test code so that people reading the test code can understand the intent of this test before reading the code. Sure. Created attachment 91407 [details]
Patch
Comment on attachment 91407 [details]
Patch
Could you review again?
Thanks,
Comment on attachment 91407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91407&action=review > LayoutTests/fast/events/mouseout-dead-node.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use HTML5 style <!DOCTYPE html> > LayoutTests/fast/events/mouseout-dead-node.html:11 > +<div onmouseout='testFailed("mouseout")' onmouseover='document.getElementById("d0").innerHTML ="you should see PASS below"'> I would have done: this.parent.innerHTML="~~". > LayoutTests/fast/events/mouseout-dead-node.html:33 > +if (!window.eventSender) > + return; This won't work. I guess you'll have to put everything in an if clause. Created attachment 91408 [details]
Patch
Comment on attachment 91408 [details]
Patch
Actually this.parent.innerText="" doesn't work.
Except that, I follow your suggestion.
Please review again.
Thanks,
Attachment 91408 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). Updating OpenSource perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 146. If any of these errors are false positives, please file a bug against check-webkit-style. Created attachment 91412 [details]
Patch
Comment on attachment 91412 [details]
Patch
Strange error happened.
So upload my patch again.
Comment on attachment 91412 [details] Patch Clearing flags on attachment: 91412 Committed r85163: <http://trac.webkit.org/changeset/85163> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/85163 might have broken Qt Linux Release The following tests are not passing: fast/events/mouseout-dead-node.html |