Bug 31465 – In testing handshake-error case, web_socket_do_extra_handshake calls non-existent method

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yuzo Fujishima]

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'

[Yuzo Fujishima]

Created attachment 43148 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

[Yuzo Fujishima]

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

[Yuzo Fujishima]

Created attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

See http://webkit.org/coding/contributing.html for documentation of the review and commit-queue flags.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

False rejection caused by bug 31461.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 43149 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

False rejection due to bug 31461.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Yuzo Fujishima]

Created attachment 43482 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

[Yuzo Fujishima]

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

[Eric Seidel (no email)]

Comment on attachment 43482 [details]
Fix: web_socket_do_extra_handshake calls non-existent method

OK.

[WebKit Commit Bot]

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

[Alexey Proskuryakov]

Manually committed as <http://trac.webkit.org/changeset/51508>. I re-wrote the ChangeLog to make it a bit more informative.

[Eric Seidel (no email)]

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! ;)

[Eric Seidel (no email)]

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. :)

[Alexey Proskuryakov]

> 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.