Bug 31465 - In testing handshake-error case, web_socket_do_extra_handshake calls non-existent method
Summary: In testing handshake-error case, web_socket_do_extra_handshake calls non-exis...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-13 01:18 PST by Yuzo Fujishima
Modified: 2009-11-30 13:54 PST (History)
2 users (show)

See Also:


Attachments
Fix: web_socket_do_extra_handshake calls non-existent method (1.06 KB, patch)
2009-11-13 01:19 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Fix: web_socket_do_extra_handshake calls non-existent method (1.06 KB, patch)
2009-11-13 01:27 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Fix: web_socket_do_extra_handshake calls non-existent method (1.56 KB, patch)
2009-11-18 21:24 PST, Yuzo Fujishima
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuzo Fujishima 2009-11-13 01:18:37 PST
In testing handshake-error case, web_socket_do_extra_handshake calls non-existent method and the following error is raised:

WARNING:root:mod_pywebsocket: web_socket_do_extra_handshake raised exception: Traceback (most recent call last):
  File "/Users/yuzo/src/chromium/src/third_party/pywebsocket/mod_pywebsocket/dispatch.py", line 163, in do_extra_handshake
    do_extra_handshake_(request)
  File "<string>", line 34, in web_socket_do_extra_handshake
AttributeError: '_StandaloneRequest' object has no attribute 'write'
Comment 1 Yuzo Fujishima 2009-11-13 01:19:50 PST
Created attachment 43148 [details]
Fix: web_socket_do_extra_handshake calls non-existent method
Comment 2 Yuzo Fujishima 2009-11-13 01:22:23 PST
Hi, reviewers,

Can you review this and submit it if it looks good?

Sorry for not catching the bug when proposing the original patch.

Yuzo
Comment 3 Yuzo Fujishima 2009-11-13 01:27:06 PST
Created attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method
Comment 4 Alexey Proskuryakov 2009-11-13 12:27:08 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

Why doesn't this mistake affect test output?  Should it?

r=me
Comment 5 Eric Seidel (no email) 2009-11-13 12:31:45 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

In response to your above comment requesting submission: You can mark your patches commit-queue=? if you want them automatically submitted.
Comment 6 Eric Seidel (no email) 2009-11-13 12:32:07 PST
See http://webkit.org/coding/contributing.html for documentation of the review and commit-queue flags.
Comment 7 WebKit Commit Bot 2009-11-13 17:11:42 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

Rejecting patch 43149 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11623 test cases.
http/tests/xmlhttprequest/access-control-basic-allow-access-control-origin-header.html -> crashed

Exiting early after 1 failures. 9152 tests run.
344.54s total testing time

9151 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 8 Eric Seidel (no email) 2009-11-13 17:18:37 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

False rejection caused by bug 31461.
Comment 9 WebKit Commit Bot 2009-11-13 17:34:17 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

Rejecting patch 43149 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11623 test cases.
inspector/console-dir.html -> crashed

Exiting early after 1 failures. 9314 tests run.
368.05s total testing time

9313 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 10 Eric Seidel (no email) 2009-11-13 18:04:00 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

False rejection due to bug 31461.
Comment 11 WebKit Commit Bot 2009-11-13 18:20:57 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

Rejecting patch 43149 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11623 test cases.
http/tests/xmlhttprequest/abort-crash.html -> crashed

Exiting early after 1 failures. 9147 tests run.
341.11s total testing time

9146 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 12 Eric Seidel (no email) 2009-11-13 18:28:41 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

I'm disabling the commit-queue until bug 31461 can be solved.  Sorry.  I expect we'll find a solution to bug 31461 on monday at which point I"ll restart the queue and this will be committed.
Comment 13 WebKit Commit Bot 2009-11-18 15:19:08 PST
Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

Rejecting patch 43149 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11631 test cases.
websocket/tests/handshake-error.html -> failed

Exiting early after 1 failures. 11630 tests run.
387.22s total testing time

11629 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output
Comment 14 Eric Seidel (no email) 2009-11-18 17:51:25 PST
Sorry this error wasn't discovered sooner.  The commit-queue stops running tests after the first failure, and bug 31461 was masking this failure before.
Comment 15 Yuzo Fujishima 2009-11-18 21:24:15 PST
Created attachment 43482 [details]
Fix: web_socket_do_extra_handshake calls non-existent method
Comment 16 Yuzo Fujishima 2009-11-18 21:27:31 PST
Sorry for the test breakage. I thought this test was in Skipped.

I believe this patch itself is correct and should be checked in:
it just revealed a hidden bug. I've filed
https://bugs.webkit.org/show_bug.cgi?id=31659

I added (back) this test to Skipped.

Yuzo
Comment 17 Eric Seidel (no email) 2009-11-26 21:29:33 PST
Comment on attachment 43482 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

OK.
Comment 18 WebKit Commit Bot 2009-11-26 21:34:26 PST
Comment on attachment 43482 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

Rejecting patch 43482 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/mac/Skipped
Hunk #1 FAILED at 101.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/Skipped.rej
patching file LayoutTests/websocket/tests/handshake-error_wsh.py
Comment 19 Alexey Proskuryakov 2009-11-30 13:37:02 PST
Manually committed as <http://trac.webkit.org/changeset/51508>. I re-wrote the ChangeLog to make it a bit more informative.
Comment 20 Eric Seidel (no email) 2009-11-30 13:45:59 PST
http://trac.webkit.org/changeset/51509

I don't think the tools did anything wrong here! :)  I reviewed a later version of the patch.  I'm not seeking credit, just making sure you're not unfairly blaming our innocent tools! ;)
Comment 21 Eric Seidel (no email) 2009-11-30 13:49:13 PST
Btw, if you're using bugzilla-tool for any of this, I believe both land-* and apply-* accept a --reviewer argument which allows you to override reviewers in cases like this where the bug might have a different reviewer than you want the final patch to have. :)
Comment 22 Alexey Proskuryakov 2009-11-30 13:54:06 PST
> I don't think the tools did anything wrong here! :)  I reviewed a later version
> of the patch.  I'm not seeking credit, just making sure you're not unfairly
> blaming our innocent tools! ;)

OK, I didn't know you actually reviewed the change, not just set the flag per earlier review. The latter has certainly happened in the past (many of use did it, including myself).

The reason I see tools share part of the blame is that almost good patches need to be reworked more often now, so reviews get forwarded by whoever gets to it first. This is mostly a good thing, but it makes reviewer info wrong in Bugzilla and in ChangeLogs.