Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
722343003:
Add end to end tests for WebSockets in net/websockets/ (Closed)
DescriptionAdd end to end tests for WebSockets in net/websockets/
Some things (notably proxies) are impossible to test using layout tests,
and expensive and difficult to test using browser_tests.
Add a way to do simple end-to-end tests in net_unittests.
BUG=441709
TEST=net_unittests
Committed: https://crrev.com/433bdabf0f4d673b8ce49250849e8571bd93cf1b
Cr-Commit-Position: refs/heads/master@{#313045}
Patch Set 1 #Patch Set 2 : Fix tests and add some more. #Patch Set 3 : Minor comment cleanups. #Patch Set 4 : Rebase. #Patch Set 5 : Fix memory leak. #Patch Set 6 : Disable the tests on Android. #
Total comments: 15
Patch Set 7 : Use NetworkDelegate::OnResolveProxy to check proxy usage. #
Total comments: 2
Patch Set 8 : Clarify comment concerning fail() #
Total comments: 20
Patch Set 9 : Changes suggested by rvargas. #Patch Set 10 : Remove mentions of things outside net/ from the comments. #
Messages
Total messages: 27 (7 generated)
ricea@chromium.org changed reviewers: + tyoshino@chromium.org
I have decided to land this without fixing crbug.com/433695 first, because this is blocking higher priority work like crbug.com/423667 PTAL
https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:24: string https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:51: quit_closure_ = run_loop_.QuitClosure(); quit_closure_ is initialized here to make DCHECK on L122 meaningful? https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:54: explain that this variable is used for storing several different types of results. Here or in the class comment. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:116: ssl_error_callbacks->CancelSSLRequest(ERR_SSL_PROTOCOL_ERROR, &ssl_info); I'm a little worried about CancelSSLRequest() being called synchronously. Is this safe? https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:165: url::Origin origin_; how about making sub_protocols_ and origin_ local to ConnectAndWait() until when we need to use some different value on these arguments? https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:279: network_delegate_->observed_before_proxy_headers_sent_callbacks()); what does this expectations add? reading the variable name, it sounds like that we're checking that we communicated with the proxy, but after reading the code carefully, it looks not.
https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:30: #include "net/base/test_data_directory.h" net/proxy/proxy_service.h
https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:24: On 2015/01/05 08:45:48, tyoshino wrote: > string Done. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:30: #include "net/base/test_data_directory.h" On 2015/01/05 08:47:29, tyoshino wrote: > net/proxy/proxy_service.h Done. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:51: quit_closure_ = run_loop_.QuitClosure(); On 2015/01/05 08:45:48, tyoshino wrote: > quit_closure_ is initialized here to make DCHECK on L122 meaningful? No, it was a mistake. I misunderstood the API of RunLoop. Actually QuitClosure() wasn't needed at all. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:54: On 2015/01/05 08:45:48, tyoshino wrote: > explain that this variable is used for storing several different types of > results. Here or in the class comment. I decided that setting fail_ in OnDropChannel() was a mistake. So now fail() unambiguously means that the handshake failed. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:116: ssl_error_callbacks->CancelSSLRequest(ERR_SSL_PROTOCOL_ERROR, &ssl_info); On 2015/01/05 08:45:48, tyoshino wrote: > I'm a little worried about CancelSSLRequest() being called synchronously. Is > this safe? I'm not 100% sure. I decided to call it via the PostTask to be on the safe side. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:165: url::Origin origin_; On 2015/01/05 08:45:48, tyoshino wrote: > how about making sub_protocols_ and origin_ local to ConnectAndWait() until when > we need to use some different value on these arguments? Done. https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:279: network_delegate_->observed_before_proxy_headers_sent_callbacks()); On 2015/01/05 08:45:48, tyoshino wrote: > what does this expectations add? reading the variable name, it sounds like that > we're checking that we communicated with the proxy, but after reading the code > carefully, it looks not. It works because of a bug. I'm still looking for a way to check that we used the proxy that will work once the bug is fixed.
https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/100001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:279: network_delegate_->observed_before_proxy_headers_sent_callbacks()); On 2015/01/06 12:27:15, Adam Rice wrote: > On 2015/01/05 08:45:48, tyoshino wrote: > > what does this expectations add? reading the variable name, it sounds like > that > > we're checking that we communicated with the proxy, but after reading the code > > carefully, it looks not. > > It works because of a bug. I'm still looking for a way to check that we used the > proxy that will work once the bug is fixed. I found a way to do it. PTAL.
lgtm https://codereview.chromium.org/722343003/diff/120001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/120001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:58: // fail() is true if the handshake failed. also if FailChannel was called
https://codereview.chromium.org/722343003/diff/120001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/120001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:58: // fail() is true if the handshake failed. On 2015/01/19 03:59:48, tyoshino wrote: > also if FailChannel was called Actually, it always means FailChannel was called. I have filed crbug.com/449891 to make this less confusing.
ricea@chromium.org changed reviewers: + rvargas@chromium.org
+rvargas for net/ OWNERS.
Just nits. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... File net/websockets/websocket_end_to_end_test.cc (right): https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:7: // 1) Blink layout tests. These are easy to write, and efficient because the I don't think this file (net) should be talking about tests that live on upper layers of the code. The opposite is not a problem, so you can add comments on the other files pointing readers to write a net_unittest instead without any layering violation. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:18: // 4) These tests. They are somewhat inefficient because the server is restarted This comment makes sense by itself: when it is ok to write a test here. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:22: // Tests should generally not be duplicated in the different places, except for isn't this always true? https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:52: class ConnectTestingEventInterface : public WebSocketEventInterface { Nit: I think this class is slightly too long to have declaration mixed with definition. It makes the code harder to read. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:58: // fail() is true if the handshake failed (ie. OnFailChannel was called). Shouldn't it be called failed_ then? And the comment belongs with the member variable. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:62: // Failure messages appear on the console and are checked by the layout tests, layout tests? https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:143: struct OnResolveProxyInfo { nit: Don't declare a type named OnFoo. That pattern is strongly tied to a callback method. ResolvedProxyInfo sounds much better.
https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... File net/websockets/websocket_end_to_end_test.cc (right): https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:7: // 1) Blink layout tests. These are easy to write, and efficient because the On 2015/01/21 00:47:04, rvargas wrote: > I don't think this file (net) should be talking about tests that live on upper > layers of the code. The opposite is not a problem, so you can add comments on > the other files pointing readers to write a net_unittest instead without any > layering violation. I think the end-to-end tests need to be viewed holistically. If net/ existed in a vacuum, then we'd need to duplicate all the Blink WebSocket layout tests here, which would be ridiculous. From a pragmatic point of view, there's nowhere else to put this comment where people have a reasonable chance of seeing it. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:22: // Tests should generally not be duplicated in the different places, except for On 2015/01/21 00:47:04, rvargas wrote: > isn't this always true? Yes. But you don't have to look far to find pointless duplicate tests. I'm afraid that without this comment, in a few years there'd be 10,000 lines of tests here. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:52: class ConnectTestingEventInterface : public WebSocketEventInterface { On 2015/01/21 00:47:04, rvargas wrote: > Nit: I think this class is slightly too long to have declaration mixed with > definition. It makes the code harder to read. Done. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:58: // fail() is true if the handshake failed (ie. OnFailChannel was called). On 2015/01/21 00:47:04, rvargas wrote: > Shouldn't it be called failed_ then? And the comment belongs with the member > variable. Okay. I made just this definition online, so that it is easy to refer to the comment. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:62: // Failure messages appear on the console and are checked by the layout tests, On 2015/01/21 00:47:04, rvargas wrote: > layout tests? Blink layout tests. I clarified the comment slightly. https://chromiumcodereview.appspot.com/722343003/diff/140001/net/websockets/w... net/websockets/websocket_end_to_end_test.cc:143: struct OnResolveProxyInfo { On 2015/01/21 00:47:04, rvargas wrote: > nit: Don't declare a type named OnFoo. That pattern is strongly tied to a > callback method. ResolvedProxyInfo sounds much better. Thanks!
https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:7: // 1) Blink layout tests. These are easy to write, and efficient because the On 2015/01/21 09:45:56, Adam Rice wrote: > On 2015/01/21 00:47:04, rvargas wrote: > > I don't think this file (net) should be talking about tests that live on upper > > layers of the code. The opposite is not a problem, so you can add comments on > > the other files pointing readers to write a net_unittest instead without any > > layering violation. > > I think the end-to-end tests need to be viewed holistically. If net/ existed in > a vacuum, then we'd need to duplicate all the Blink WebSocket layout tests here, > which would be ridiculous. I don't agree with the conclusion. net_unittests should make sure that the functionality implemented by net works as advertised. If that means test both ends of a connection, and a proxy in the middle, so be it. Blink requires net (in that in order to build the whole thing and run a layout test, net is a required), so it can know what is implemented (and tested) at the net layer and avoid duplication (only test the functionality that is added by blink, or some aspect of the integration). ... and net somewhat exists in a vacuum these days, as we explicitly want to support users that are not tied to content or above. So talking about layout tests or browser tests here is a layering violation. (sure, very small given that this is a comment on a unit test file) I must admit that I'm completely unfamiliar with how we've been moving WS code to net, but I don't think any module (much less something like net) should be delegating testing on some other module that may or may not be present (layout tests) > > From a pragmatic point of view, there's nowhere else to put this comment where > people have a reasonable chance of seeing it. I would agree with the whole thing if I believed this statement. But I think it is perfectly fine to add relevant comments on browser/ppapi/layout/whatever-test* stating what _kind_ of tests it is fine to add at that layer, and that net_unittest may be a better place to add tests that don't fit those requirements. If I'm adding code say at the ppapi layer, I'm very likely to be aware that there is WS code living in net, so there should be some unit tests there... the opposite is not true. This is philosophically the same as base code talking about chrome_threads just because they are implemented with the threading primitives exposed by base. https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:22: // Tests should generally not be duplicated in the different places, except for On 2015/01/21 09:45:56, Adam Rice wrote: > On 2015/01/21 00:47:04, rvargas wrote: > > isn't this always true? > > Yes. But you don't have to look far to find pointless duplicate tests. I'm > afraid that without this comment, in a few years there'd be 10,000 lines of > tests here. To be honest, I don't think a line saying don't duplicate code is going to have any effect... it will have to be a popup warning on Rietveld :P https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:62: // Failure messages appear on the console and are checked by the layout tests, On 2015/01/21 09:45:56, Adam Rice wrote: > On 2015/01/21 00:47:04, rvargas wrote: > > layout tests? > > Blink layout tests. I clarified the comment slightly. My point is that this file should not know what a layout test is, or what is it going to do with error messages generated by net code... and presumably the caller of this method will not be a layout test (right?) so it shouldn't care what other code is going to do with a message.
https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... File net/websockets/websocket_end_to_end_test.cc (right): https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:7: // 1) Blink layout tests. These are easy to write, and efficient because the On 2015/01/21 19:46:53, rvargas wrote: > If I'm adding code say at the ppapi layer, I'm very likely to be aware that > there is WS code living in net, so there should be some unit tests there... the > opposite is not true. I didn't find out about the PPAPI tests until I'd already been working on WebSockets for a year. If someone is looking in there, they're already having a bad day. Anyway, I have removed the offending comments. https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:18: // 4) These tests. They are somewhat inefficient because the server is restarted On 2015/01/21 00:47:05, rvargas wrote: > This comment makes sense by itself: when it is ok to write a test here. Okay. This comment is now by itself. https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:22: // Tests should generally not be duplicated in the different places, except for On 2015/01/21 19:46:53, rvargas wrote: > On 2015/01/21 09:45:56, Adam Rice wrote: > > On 2015/01/21 00:47:04, rvargas wrote: > > > isn't this always true? > > > > Yes. But you don't have to look far to find pointless duplicate tests. I'm > > afraid that without this comment, in a few years there'd be 10,000 lines of > > tests here. > > To be honest, I don't think a line saying don't duplicate code is going to have > any effect... it will have to be a popup warning on Rietveld :P Removed. https://codereview.chromium.org/722343003/diff/140001/net/websockets/websocke... net/websockets/websocket_end_to_end_test.cc:62: // Failure messages appear on the console and are checked by the layout tests, On 2015/01/21 19:46:53, rvargas wrote: > On 2015/01/21 09:45:56, Adam Rice wrote: > > On 2015/01/21 00:47:04, rvargas wrote: > > > layout tests? > > > > Blink layout tests. I clarified the comment slightly. > > My point is that this file should not know what a layout test is, or what is it > going to do with error messages generated by net code... and presumably the > caller of this method will not be a layout test (right?) so it shouldn't care > what other code is going to do with a message. Done.
lgtm, thanks.
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/722343003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/722343003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/722343003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/433bdabf0f4d673b8ce49250849e8571bd93cf1b Cr-Commit-Position: refs/heads/master@{#313045} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
