Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
869073002:
Add WebSocket cookie tests. (Closed)
DescriptionAdd WebSocket cookie tests.
As it turned out WebSocket implementation had some problems with Cookie, this
CL adds unit tests.
BUG=None
R=rsleevi@chromium.org
Committed: https://crrev.com/01a5d66a26a60f970d9a8b595d3b463d6c876c00
Cr-Commit-Position: refs/heads/master@{#315930}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 29
Patch Set 6 : #
Total comments: 88
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 29
Patch Set 10 #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 4
Patch Set 14 : #
Messages
Total messages: 49 (12 generated)
ricea@chromium.org changed reviewers: + ricea@chromium.org
I assume you are going to add a test with a Domain=example.com attribute after https://codereview.chromium.org/859663003/ has landed? https://codereview.chromium.org/869073002/diff/40001/net/websockets/websocket... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/40001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:231: struct WaitTask { You don't need this class. You can do the same thing directly with base::RunLoop(). Use run_loop_.Run() instead of RunUntilIdle() and then use run_loop_.Quit() when the callback is called to indicate the cookies are available. https://codereview.chromium.org/869073002/diff/40001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:1333: TEST_F(WebSocketStreamCreateTest, UseCookie) { You should try to only test one thing per test case. As far as I can see, this test case tests six things: 1. That http cookies are sent on ws: connections 2. That non-secure https cookies are sent on ws: connections 3. That secure https cookies are not sent sent on ws: connections 4. That ws cookies are sent on ws: connections 5. That non-secure wss cookies are sent on ws: connections 6. That secure wss cookies are not sent on wss: connections So it really should be six test cases. Since they would all be nearly identical, a data-driven test using TEST_P and INSTANTIATE_TEST_CASE_P might be the cleanest solution.
Patchset #4 (id:60001) has been deleted
Thank you for the review. > I assume you are going to add a test with a Domain=example.com attribute after https://codereview.chromium.org/859663003/ has landed? In fact, I didn't realize that we could write a failing test. Thank you! I added those ones. Now I want to commit this CL after the fixing CL lands. https://codereview.chromium.org/869073002/diff/40001/net/websockets/websocket... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/40001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:231: struct WaitTask { On 2015/01/23 07:31:27, Adam Rice wrote: > You don't need this class. You can do the same thing directly with > base::RunLoop(). Use run_loop_.Run() instead of RunUntilIdle() and then use > run_loop_.Quit() when the callback is called to indicate the cookies are > available. Done. https://codereview.chromium.org/869073002/diff/40001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:1333: TEST_F(WebSocketStreamCreateTest, UseCookie) { On 2015/01/23 07:31:27, Adam Rice wrote: > You should try to only test one thing per test case. As far as I can see, this > test case tests six things: > > 1. That http cookies are sent on ws: connections > 2. That non-secure https cookies are sent on ws: connections > 3. That secure https cookies are not sent sent on ws: connections > 4. That ws cookies are sent on ws: connections > 5. That non-secure wss cookies are sent on ws: connections > 6. That secure wss cookies are not sent on wss: connections > > So it really should be six test cases. Since they would all be nearly identical, > a data-driven test using TEST_P and INSTANTIATE_TEST_CASE_P might be the > cleanest solution. Done.
https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:467: const GURL url; Personally I would make all these types const char * so that I could use static initialisation, ie. const WebSocketStreamUseCookieTestParameter kUseCookieTestParameters[] = { {"ws://www.example.com", "http://www.example.com", "http://www.example.com", "test-cookie", "Cookie: test-cookie\r\n"}, ... }; and then afterwards INSTANTIATE_TEST_CASE_P( WebSocketStreamUseCookieTest, WebSocketStreamUseCookieTest, ::testing::ValuesIn(kUseCookieTestParameters)); But of course that means converting them to GURL and std::string within the test code. It is up to you. https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:505: struct GetCookies { I find having a struct called GetCookies inside a method called GetCookies really confusing. I would be fine with a simple name like "Helper" or maybe "GetCookieHelper" if you think that is too generic. https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:508: ; This semi-colon does nothing! https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:1679: // Non-maching cookies for ws "matching" (and below).
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:467: const GURL url; On 2015/01/23 11:14:02, Adam Rice wrote: > Personally I would make all these types const char * so that I could use static > initialisation, ie. > > const WebSocketStreamUseCookieTestParameter kUseCookieTestParameters[] = > { > {"ws://www.example.com", "http://www.example.com", > "http://www.example.com", "test-cookie", "Cookie: test-cookie\r\n"}, > ... > }; > > and then afterwards > > INSTANTIATE_TEST_CASE_P( > WebSocketStreamUseCookieTest, > WebSocketStreamUseCookieTest, > ::testing::ValuesIn(kUseCookieTestParameters)); > > But of course that means converting them to GURL and std::string within the test > code. It is up to you. I would like to leave it as is, because of type checking. We will be able to write test cases in a similar way with the initializer list when C++11 is fully supported. https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:505: struct GetCookies { On 2015/01/23 11:14:02, Adam Rice wrote: > I find having a struct called GetCookies inside a method called GetCookies > really confusing. I would be fine with a simple name like "Helper" or maybe > "GetCookieHelper" if you think that is too generic. Done. https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:508: ; On 2015/01/23 11:14:02, Adam Rice wrote: > This semi-colon does nothing! Done. https://codereview.chromium.org/869073002/diff/80001/net/websockets/websocket... net/websockets/websocket_stream_test.cc:1679: // Non-maching cookies for ws On 2015/01/23 11:14:03, Adam Rice wrote: > "matching" (and below). Done.
lgtm Thanks for doing this. We were badly lacking test coverage in this area. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1761: // Cookies for ws Nitpick: the cookie comes *from* ws (or is "set by" ws) and then is looked up via some other protocol. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1786: // Cookies for wss Maybe we should test that the "Secure" attribute works when using Set-Cookie from wss? I'm not sure, because obviously since we use the same code path as HTTP it has to work. But it would be more thorough to test it. We might also want to test that the "Path" attribute works correctly... or not? I don't know.
https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:130: const std::string& socket_path, Why is this not simply a single GURL? [edit: I guess this is consistent with the existing tests, which I consider fairly unreadable and unmaintainable. Not that the rest of //net is much better, but I wish we could do better and we need to] https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:163: // errors like "Unable to perform synchronous IO while stopped" will occur. Both of these seem undesirable. Can this be expressed via compile-time capabilities / assertions, rather than the comment that will inevitably be ignored (because this file is huge) https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:455: struct WebSocketStreamUseCookieTestParameter { Document & Pluralize (Parameters) https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:456: WebSocketStreamUseCookieTestParameter(const GURL& url, Document https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:471: const std::string cookies; Making these const prevents operator=, which would be quite useful. I'm not sure it's worth the const-ness to do this. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:482: class RunLoop { This is deeply confusing, because it shadows a name in another namespace *that it's directly interacting with*. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:494: base::Bind(&RunLoop::Quit, base::Unretained(&run_loop))); You should be using https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&l=71 instead of creating a custom object. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:522: base::Bind(&GetCookiesHelper::Quit, base::Unretained(&helper))); static void StashCookiesHelper(const base::Closure& task, const base::WeakPtr<std::string>& weak_cookies, const std::string& incoming_cookies) { if (weak_cookies.get()) *weak_cookies = incoming_cookies; PostTask(task); } std::string cookies; base::WeakPtrFactory<std::string> weak_cookies(&cookies); base::Bind(&StashCookiesHelper, run_loop.QuitClosure(), weak_cookies.GetWeakPtr()) Benefits: If the message loop gets quit because of some other reason, things don't blow up if the task is still pending. And you avoid the class helper and base::Unretained(), which are non-idiomatic Chromium threading. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1494: // In case we use a TLS connection. This comment doesn't make sense. It sounds like you're uncertain whether it's necessary, but it is (judging by your test below) https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1510: ""); s/""/std::string()/ https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1521: WebSocketStreamUseCookieTestParameter(GURL("ws://www.example.com"), If you deal with the constness I mentioned above, you should be able to use the simple { } struct initialization sequence. ::testing::Values<MyType>( { GURL("ws://..."), GURL("http://..."), ... }, { GURL("ws://...") } ) etc https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1525: "Cookie: test-cookie\r\n"), The amount of copy-pasta for thees \r\n makes it seem like you should just be handling it in your class initializer, unless you're explicitly trying to test bad cookies (and you should be) https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1652: ""), s/""/std::string()/
https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:130: const std::string& socket_path, On 2015/01/24 04:13:07, Ryan Sleevi wrote: > Why is this not simply a single GURL? Because then both the production code and the test would have a common point of failure in the GURL code. Plus it is tempting to use GURL::host to get the value for |socket_host|, but that isn't actually correct when port numbers are involved. It's a specific case of the general maxim that "putting logic in tests in bad". > [edit: I guess this is consistent with the existing tests, which I consider > fairly unreadable and unmaintainable. Not that the rest of //net is much better, > but I wish we could do better and we need to] It grew organically to the current state. Certainly it could use some re-factoring. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:163: // errors like "Unable to perform synchronous IO while stopped" will occur. On 2015/01/24 04:13:08, Ryan Sleevi wrote: > Both of these seem undesirable. Can this be expressed via compile-time > capabilities / assertions, rather than the comment that will inevitably be > ignored (because this file is huge) How? We can't make compile-time assertions about the contents of a std::string. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:494: base::Bind(&RunLoop::Quit, base::Unretained(&run_loop))); On 2015/01/24 04:13:07, Ryan Sleevi wrote: > You should be using > https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&l=71 > instead of creating a custom object. We need to swallow the bool parameter that SetCookieWithOptionsAsync is passing to the callback somehow.
https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1521: WebSocketStreamUseCookieTestParameter(GURL("ws://www.example.com"), On 2015/01/24 04:13:07, Ryan Sleevi wrote: > If you deal with the constness I mentioned above, you should be able to use the > simple { } struct initialization sequence. > > ::testing::Values<MyType>( > { GURL("ws://..."), > GURL("http://..."), > ... > }, > { GURL("ws://...") } > ) > > etc IIUC having constant members is not related to initialization. As Adam pointed, we can use {} initialization if we make the parameter POD class (in C++03). On the other hand, in C++11, the uniform initialization syntax is introduced and it's working on my local environment, though it is currently banned (See http://chromium-cpp.appspot.com/). I would rather keep this until the uniform initialization syntax is allowed in the project. I'm not a C++ guru, so please correct me if I'm wrong.
https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:130: const std::string& socket_path, On 2015/01/26 06:38:59, Adam Rice wrote: > On 2015/01/24 04:13:07, Ryan Sleevi wrote: > > Why is this not simply a single GURL? > > Because then both the production code and the test would have a common point of > failure in the GURL code. I'm sorry, I don't understand this? It's not your responsibility to test GURL. GURL should test GURL. You should presume that GURL does the right thing (or conforms to your interface), and then test to make sure that *your* code does the right thing and conforms to *your* interface. > Plus it is tempting to use GURL::host to get the value > for |socket_host|, but that isn't actually correct when port numbers are > involved. It's a specific case of the general maxim that "putting logic in tests > in bad". I'm sorry, but I think it's the opposite. You should strive to make sure your tests conform to reasonable interfaces and meet the external behaviours, regardless of the internal implementation. Your argument about using GURL::host to get the value to |socket_host| doesn't make sense. I could use GURL::querystring to get the value to |socket_host|, and it'd be just as well a bug. Using host() and port() explicitly from GURL seems better, not worse here. > > > [edit: I guess this is consistent with the existing tests, which I consider > > fairly unreadable and unmaintainable. Not that the rest of //net is much > better, > > but I wish we could do better and we need to] > > It grew organically to the current state. Certainly it could use some > re-factoring. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:163: // errors like "Unable to perform synchronous IO while stopped" will occur. On 2015/01/26 06:38:59, Adam Rice wrote: > On 2015/01/24 04:13:08, Ryan Sleevi wrote: > > Both of these seem undesirable. Can this be expressed via compile-time > > capabilities / assertions, rather than the comment that will inevitably be > > ignored (because this file is huge) > > How? We can't make compile-time assertions about the contents of a std::string. It seems entirely pointless to require "" or "something\r\n". If !string.empty(), then this method could append "\r\n", so then the test author needs only write "" or "something". That is, requiring everyone to ensure "something\r\n" seems pointless, especially since you never test that "something" fails (and it would be unrelated to the test anyways) https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:494: base::Bind(&RunLoop::Quit, base::Unretained(&run_loop))); On 2015/01/26 06:38:59, Adam Rice wrote: > On 2015/01/24 04:13:07, Ryan Sleevi wrote: > > You should be using > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&l=71 > > instead of creating a custom object. > > We need to swallow the bool parameter that SetCookieWithOptionsAsync is passing > to the callback somehow. Then just use a bound free function static void IgnoreBoolArg(const base::Closure& closure, bool unused) { return closure.Run(); } Base::Bind(&IgnoreBoolArg, run_loop_->QuitClosure()); https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1521: WebSocketStreamUseCookieTestParameter(GURL("ws://www.example.com"), On 2015/01/26 07:01:40, yhirano wrote: > On 2015/01/24 04:13:07, Ryan Sleevi wrote: > > If you deal with the constness I mentioned above, you should be able to use > the > > simple { } struct initialization sequence. > > > > ::testing::Values<MyType>( > > { GURL("ws://..."), > > GURL("http://..."), > > ... > > }, > > { GURL("ws://...") } > > ) > > > > etc > > IIUC having constant members is not related to initialization. As Adam pointed, > we can use {} initialization if we make the parameter POD class (in C++03). On > the other hand, in C++11, the uniform initialization syntax is introduced and > it's working on my local environment, though it is currently banned (See > http://chromium-cpp.appspot.com/). I would rather keep this until the uniform > initialization syntax is allowed in the project. > I'm not a C++ guru, so please correct me if I'm wrong. Using { } to initialize POD structs is not a C++11 construct. The C++11 construct is using { } to initialize complex types (e.g. a vector). Here, what we're initializing is a constant array, which uses { } syntax, with a structure, which uses { }. The { } applies regardless of POD-ness, provided that each member not elaborated in the list follows a default-initializer rule (e.g. even std::string() can be default-initialized by calling the no-arg constructor) The issue with const-ness is now ::testing::Values() can behave with that array, which is why it was easier to not explicitly specify it, but you may be able to get away with it.
https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:130: const std::string& socket_path, On 2015/01/27 00:47:22, Ryan Sleevi wrote: > It's not your responsibility to test GURL. GURL should test GURL. You should > presume that GURL does the right thing (or conforms to your interface), and then > test to make sure that *your* code does the right thing and conforms to *your* > interface. In principle, yes. However, if the implementation and the tests both consist of the same code, then the tests are no longer testing anything. > I'm sorry, but I think it's the opposite. You should strive to make sure your > tests conform to reasonable interfaces and meet the external behaviours, > regardless of the internal implementation. The tests don't have to conform to anything. They need to be concise and readable, so that a) A maintainer unfamiliar with the code can look at a test failure and quickly see what it means b) Someone with some familiarity with the code or subject matter can browse through the tests and efficiently determine what is and is not covered. > Your argument about using GURL::host to get the value to |socket_host| doesn't > make sense. I could use GURL::querystring to get the value to |socket_host|, and > it'd be just as well a bug. I doubt that anyone in the history of HTTP has ever put the query string in the Host: header. Whereas leaving off the port, or specifying it wrongly, is something that happens in HTTP implementations all the time. > Using host() and port() explicitly from GURL seems better, not worse here. We'd need some code like the following std::string socket_host = socket_url.host(); if (socket_url.has_port()) socket_host += ":" + socket_url.port(); That's logic. It needs tests. Fortunately, we can create websocket_test_util_test.cc and that's probably what we'll do. Like I said, the tests need refactoring, although why this is blocking fixing a crash bug I don't quite understand. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:163: // errors like "Unable to perform synchronous IO while stopped" will occur. On 2015/01/27 00:47:22, Ryan Sleevi wrote: > It seems entirely pointless to require "" or "something\r\n". If > !string.empty(), then this method could append "\r\n", so then the test author > needs only write "" or "something". That's logic. Logic requires tests. At some point you have to stop putting logic in your tests, or you'll have an infinite regression. Different people may draw the line in different places. Personally I have no problem with a few "\r\n"s here and there. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1521: WebSocketStreamUseCookieTestParameter(GURL("ws://www.example.com"), On 2015/01/26 07:01:40, yhirano wrote: > On 2015/01/24 04:13:07, Ryan Sleevi wrote: > > If you deal with the constness I mentioned above, you should be able to use > the > > simple { } struct initialization sequence. > > > > ::testing::Values<MyType>( > > { GURL("ws://..."), > > GURL("http://..."), > > ... > > }, > > { GURL("ws://...") } > > ) > > > > etc > > IIUC having constant members is not related to initialization. As Adam pointed, > we can use {} initialization if we make the parameter POD class (in C++03). On > the other hand, in C++11, the uniform initialization syntax is introduced and > it's working on my local environment, though it is currently banned (See > http://chromium-cpp.appspot.com/). I would rather keep this until the uniform > initialization syntax is allowed in the project. > I'm not a C++ guru, so please correct me if I'm wrong. Indeed. Brace-initialisation of function arguments is not valid before C++11, and so is forbidden. If the initialisation is taken out of the function then it would be a global array, which would have to be of POD type to be permitted by the style guide.
https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:130: const std::string& socket_path, On 2015/01/27 06:32:46, Adam Rice wrote: > I doubt that anyone in the history of HTTP has ever put the query string in the > Host: header. Whereas leaving off the port, or specifying it wrongly, is > something that happens in HTTP implementations all the time. > > > Using host() and port() explicitly from GURL seems better, not worse here. > > We'd need some code like the following > > std::string socket_host = socket_url.host(); > if (socket_url.has_port()) > socket_host += ":" + socket_url.port(); > > That's logic. It needs tests. Fortunately, we can create > websocket_test_util_test.cc and that's probably what we'll do. Like I said, the > tests need refactoring, although why this is blocking fixing a crash bug I don't > quite understand. Because quality matters. Because I've had to deal with a lot of teams acting reactively towards bugs, rather than proactively towards debt. Because I think it's important that when code is in front of you, it's important to call out patterns and designs that don't make sense or add unnecessary complexity. Yes, this is unfamiliar code, and I'm viewing in a limited context of a diff, but the important feedback is and remains that "This is confusing, complex code, and I have trouble reasoning about the correctness of _this fix_ precisely because of it". That's the last place you want ot be when reactively fixing something. Please, ensure test cleanup is a high priority, and please, feel free to get more //net owners involved in reviewing, so that people unfamiliar with the code can share their impressions and confusion, since that overall helps raise the quality and readability. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:163: // errors like "Unable to perform synchronous IO while stopped" will occur. On 2015/01/27 06:32:46, Adam Rice wrote: > On 2015/01/27 00:47:22, Ryan Sleevi wrote: > > It seems entirely pointless to require "" or "something\r\n". If > > !string.empty(), then this method could append "\r\n", so then the test author > > needs only write "" or "something". > > That's logic. Logic requires tests. At some point you have to stop putting logic > in your tests, or you'll have an infinite regression. Different people may draw > the line in different places. Personally I have no problem with a few "\r\n"s > here and there. I'm sorry, I really have to disagree with you on your testing philosophy. I want tests that are easy to read, easy to write, and easy to understand what's going on. Every single test has logic. Logic makes tests. The important thing is to not _duplicate_ logic, and to make sure you're testing the _right_ interface. I think it's absolutely unnecessary to require that the test author know to put \r\n at the end of every string, lest their test mysteriously fail. That's an onerous requirement on test authors, and a burden for test readers, for what is ultimately something so superfluous that it's something a test harness _should_ be helping with. Logic *BELONGS* in tests. That's how tests test. It's about finding a balance. https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke... net/websockets/websocket_stream_test.cc:1521: WebSocketStreamUseCookieTestParameter(GURL("ws://www.example.com"), On 2015/01/27 06:32:46, Adam Rice wrote: > Indeed. Brace-initialisation of function arguments is not valid before C++11, > and so is forbidden. > > If the initialisation is taken out of the function then it would be a global > array, which would have to be of POD type to be permitted by the style guide. This hasn't applied to unit tests historically, but can be trivially dealt with by making the class POD, which introduces *no* functional/logic change (using your criteria) struct WebSocketStreamUseCookieTestParameters { const char* url; const char* cookie_url; const char* origin; const char* cookie_line; const char* cookies; }; Then just: const GURL url(GetParam().url); const GURL cookie_url(GetParam().cookie_url); std::string cookie_line(GetParam().cookie_line); if (!cookie_line.empty()) cookie_line.append("\r\n"); SetCookie(store, cookie_url, cookie_line); CreateAndConnectStandardWithCookies(url.spec(), url.host(), url.path(), NoSubProtocols(), GetParam().origin, GetParam().cookies, std::string(), std::string()); then your test simply becomes static const WebSocketStreamUseCookieTestParameters kTestCookies[] = { { "ws://www.example.com", "http://www.example.com", "http://www.example.com", "test-cookie", "Cookie: test-cookie" }, { ... }, { ... }, }; INSTANTIATE_TEST_CASE_P( ..., ::testing::ValuesIn(kTestCookies));
Adam, websocket_stream_test.cc is already big, and it will be bigger if we add cookie tests. I wonder if we should have another test file for such tests. What do you think? I uploaded a WIP CL[1] for WebSocketStreamCreateTestBase which will be shared between test classes among multiple test files. 1: https://codereview.chromium.org/859133004/
On 2015/01/30 10:43:50, yhirano wrote: > Adam, websocket_stream_test.cc is already big, and it will be bigger if we add > cookie tests. I wonder if we should have another test file for such tests. What > do you think? I uploaded a WIP CL[1] for WebSocketStreamCreateTestBase which > will be shared between test classes among multiple test files. > > 1: https://codereview.chromium.org/859133004/ Sounds good to me. Actually I think probably most of the tests in websocket_stream_test.cc don't belong there, because they are really tests of the handshake in general rather than the code in websocket_stream.cc specifically. But I see no harm in putting WebSocketStreamCreateTestBase in websocket_test_util.{h,cc} rather than creating new files for it. Is there an important reason to keep it separate? I want to do some refactoring after this CL has landed to make the tests easier to read (reducing duplicate code, strange rules, and long argument lists).
On 2015/01/30 11:20:22, Adam Rice wrote:
> But I see no harm in putting WebSocketStreamCreateTestBase in
> websocket_test_util.{h,cc} rather than creating new files for it. Is there an
> important reason to keep it separate?
>
Yeah, I did that before, but defining the class requires to include
net/socket/socket_test_util.h that includes gtest.h. This is problematic because
a non-test file (websocket_stream.cc) includes websocket_test_util.h.
On 2015/01/30 11:35:36, yhirano wrote: > Yeah, I did that before, but defining the class requires to include > net/socket/socket_test_util.h that includes gtest.h. This is problematic because > a non-test file (websocket_stream.cc) includes websocket_test_util.h. Okay.
Patchset #7 (id:180001) has been deleted
I decoupled cookie tests into another file, to make my emacs happy. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:45: // The URL for the WebSocket connection. Comment: done https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:46: const char* const url; I don't see any harm from constness. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:70: class RunLoop { I will fix this helper if necessary after the GetCookies discussion settles. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { Ryan's comment (https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke...): static void StashCookiesHelper(const base::Closure& task, const base::WeakPtr<std::string>& weak_cookies, const std::string& incoming_cookies) { if (weak_cookies.get()) *weak_cookies = incoming_cookies; PostTask(task); } std::string cookies; base::WeakPtrFactory<std::string> weak_cookies(&cookies); base::Bind(&StashCookiesHelper, run_loop.QuitClosure(), weak_cookies.GetWeakPtr()) Benefits: If the message loop gets quit because of some other reason, things don't blow up if the task is still pending. And you avoid the class helper and base::Unretained(), which are non-idiomatic Chromium threading. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:156: // For wss tests. Ryan's comment: This comment doesn't make sense. It sounds like you're uncertain whether it's necessary, but it is (judging by your test below) https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:163: const std::string cookie_header(AddCRLFIfNotEmpty(GetParam().cookie_header)); I don't have a strong opinion whether we enforce "\r\n" or not.
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:43: class ClientUseCookieParameter { I made this (and the other) class POD. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { On 2015/02/02 05:29:56, yhirano wrote: > Ryan's comment > (https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke...): > > static void StashCookiesHelper(const base::Closure& task, const > base::WeakPtr<std::string>& weak_cookies, const std::string& incoming_cookies) { > if (weak_cookies.get()) > *weak_cookies = incoming_cookies; > PostTask(task); > } > > std::string cookies; > base::WeakPtrFactory<std::string> weak_cookies(&cookies); > base::Bind(&StashCookiesHelper, run_loop.QuitClosure(), > weak_cookies.GetWeakPtr()) > > > Benefits: If the message loop gets quit because of some other reason, things > don't blow up if the task is still pending. And you avoid the class helper and > base::Unretained(), which are non-idiomatic Chromium threading. I'm not sure what you presume: If you are worrying that the message loop gets quit without GetCookiesHelper::Quit call, you should check it explicitly to avoid accidental pass. That means you should add another WeakPtr<bool> parameter to your function but it is messy. It's more reasonable to add a bool member to GetCookiesHelper. I can remove base::Unretained by using scoped_ref_ptr if you prefer. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:156: // For wss tests. On 2015/02/02 05:29:56, yhirano wrote: > Ryan's comment: > This comment doesn't make sense. It sounds like you're uncertain whether it's > necessary, but it is (judging by your test below) I'm not certain because it depends parameters. Fixed the comment.
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:156: // For wss tests. > I'm not certain because it depends parameters. Fixed the comment. Sorry, I meant 'it depends on parameters.'
There are a lot of quality issues with this CL. I'm feeling more and more uncomfortable with it. I've tried to tag those that immediately stood out, but please take a detailed personal look over this code before sending the next CL, to see if there are any patterns noticed. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:29: CHECK(cookie_header.empty() || EndsWith(cookie_header, "\r\n", true)); Comment explaining why? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:38: static std::string AddCRLFIfNotEmpty(const std::string& s) { No need to make this static. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:43: class ClientUseCookieParameter { On 2015/02/02 05:39:57, yhirano wrote: > I made this (and the other) class POD. Make this a struct then, since you're following struct rules - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C... https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:87: class ServerSetCookieParameter { ditto struct https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { On 2015/02/02 05:39:57, yhirano wrote: > On 2015/02/02 05:29:56, yhirano wrote: > > Ryan's comment > > > (https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke...): > > > > static void StashCookiesHelper(const base::Closure& task, const > > base::WeakPtr<std::string>& weak_cookies, const std::string& incoming_cookies) > { > > if (weak_cookies.get()) > > *weak_cookies = incoming_cookies; > > PostTask(task); > > } > > > > std::string cookies; > > base::WeakPtrFactory<std::string> weak_cookies(&cookies); > > base::Bind(&StashCookiesHelper, run_loop.QuitClosure(), > > weak_cookies.GetWeakPtr()) > > > > > > Benefits: If the message loop gets quit because of some other reason, things > > don't blow up if the task is still pending. And you avoid the class helper and > > base::Unretained(), which are non-idiomatic Chromium threading. > > I'm not sure what you presume: If you are worrying that the message loop gets > quit without GetCookiesHelper::Quit call, you should check it explicitly to > avoid accidental pass. The concern here is that the message loop is quit by some other task prior to the StashCookiesHelper being run, and then that task being run. In a practical sense, it's not a real issue. However, it highlights that your coding pattern is playing very fast and very loose with memory safety, and just because it's test code isn't a good reason for that. Tests, like production code, should assume hostility / weird things, and be coded safely. base::Unretained() is always a read flag for me. > That means you should add another WeakPtr<bool> parameter > to your function but it is messy. I don't follow your logic. The WeakPtr *is* the bool. If the message loop is quit > It's more reasonable to add a bool member to > GetCookiesHelper. I can remove base::Unretained by using scoped_ref_ptr if you > prefer. scoped_refptr<> is equally an anti-pattern used to hide over people not understanding / documenting their threading model. That's why I tried to make it more explicit. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:489: ValuesIn(kClientUseCookieParameters)); Move this to line 358 - keep your code & instantiations close. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:7: #include <algorithm> Unnecessary? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:10: #include <vector> These three are in your header, unnecessary. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:12: #include "base/compiler_specific.h" unnecessary https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:14: #include "net/base/net_errors.h" unnecessary https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:17: #include "net/socket/client_socket_handle.h" unnecessary https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:18: #include "net/socket/socket_test_util.h" unnecessary? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:19: #include "net/test/cert_test_util.h" unnecessary https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:20: #include "net/url_request/url_request_test_util.h" unnecessary? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:21: #include "net/websockets/websocket_basic_handshake_stream.h" unnecessary? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:25: #include "net/websockets/websocket_test_util.h" unnecessary, in your header https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:15: #include "net/http/http_request_headers.h" Forward declare https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:16: #include "net/http/http_response_headers.h" Forward declare https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:17: #include "net/socket/socket_test_util.h" unnecessary? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:18: #include "net/websockets/websocket_handshake_request_info.h" Forward declare https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:19: #include "net/websockets/websocket_handshake_response_info.h" Forward declare https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:21: #include "url/gurl.h" Forward declare https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:25: NET_EXPORT_PRIVATE class WebSocketStreamCreateTestBase { class NET_EXPORT_PRIVATE WebSocketStreamCreateTestBase https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:26: class TestConnectDelegate; STYLE: Inappropriate forward declaration. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Format private goes last / end of file. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:39: scoped_ptr<base::Timer> timer); IWYU: Include timer header https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:49: static void RunUntilIdle() { base::RunLoop().RunUntilIdle(); } Don't inline https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:51: // A simple function to make the tests more readable. Creates an empty vector. Drop second sentance https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:52: static std::vector<std::string> NoSubProtocols() { Is static really necessary? Ditto line 49. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:53: return std::vector<std::string>(); Don't inline https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:67: SSLInfo ssl_info_; IWYU: Include SSLInfo header https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:69: ScopedVector<SSLSocketDataProvider> ssl_data_; IWYU: Include base/memory/scoped_vector https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:71: }; private: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_test_util.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_test_util.cc:39: return WebSocketStandardRequestWithCookies(path, host, origin, "", s/""/std::string()/ https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_test_util.h:68: extern std::string WebSocketStandardRequestWithCookies( Why are these all flagged extern?
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { On 2015/02/02 20:14:41, Ryan Sleevi wrote: > On 2015/02/02 05:39:57, yhirano wrote: > > On 2015/02/02 05:29:56, yhirano wrote: > > > Ryan's comment > > > > > > (https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke...): > > > > > > static void StashCookiesHelper(const base::Closure& task, const > > > base::WeakPtr<std::string>& weak_cookies, const std::string& > incoming_cookies) > > { > > > if (weak_cookies.get()) > > > *weak_cookies = incoming_cookies; > > > PostTask(task); > > > } > > > > > > std::string cookies; > > > base::WeakPtrFactory<std::string> weak_cookies(&cookies); > > > base::Bind(&StashCookiesHelper, run_loop.QuitClosure(), > > > weak_cookies.GetWeakPtr()) > > > > > > > > > Benefits: If the message loop gets quit because of some other reason, things > > > don't blow up if the task is still pending. And you avoid the class helper > and > > > base::Unretained(), which are non-idiomatic Chromium threading. > > > > I'm not sure what you presume: If you are worrying that the message loop gets > > quit without GetCookiesHelper::Quit call, you should check it explicitly to > > avoid accidental pass. > > The concern here is that the message loop is quit by some other task prior to > the StashCookiesHelper being run, and then that task being run. > > In a practical sense, it's not a real issue. However, it highlights that your > coding pattern is playing very fast and very loose with memory safety, and just > because it's test code isn't a good reason for that. > > Tests, like production code, should assume hostility / weird things, and be > coded safely. base::Unretained() is always a read flag for me. > > > That means you should add another WeakPtr<bool> parameter > > to your function but it is messy. > > I don't follow your logic. The WeakPtr *is* the bool. If the message loop is > quit > > > It's more reasonable to add a bool member to > > GetCookiesHelper. I can remove base::Unretained by using scoped_ref_ptr if you > > prefer. > > scoped_refptr<> is equally an anti-pattern used to hide over people not > understanding / documenting their threading model. That's why I tried to make it > more explicit. When the message loop is quit without calling StashCookiesHelper, you need to fail the test because this function does not work as expected. In order to detect it you need a boolean. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:52: static std::vector<std::string> NoSubProtocols() { On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Is static really necessary? Ditto line 49. I'd like to leave it be because we can see more easily that no instance information is used in the implementation.
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { On 2015/02/03 01:26:32, yhirano wrote: > When the message loop is quit without calling StashCookiesHelper, you need to > fail the test because this function does not work as expected. In order to > detect it you need a boolean. I still find this code not up to the degree that we expect of //net, and in particular, too "novel" in the way it deals with threading. base::Unretained() remains an anti-pattern that I will flag in reviews, because it causes way too much reader difficulty in reasoning about the correctness of the code. The added nesting of the run_loop_ is similarly difficult to comprehend. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:52: static std::vector<std::string> NoSubProtocols() { On 2015/02/03 01:26:32, yhirano wrote: > I'd like to leave it be because we can see more easily that no instance > information is used in the implementation. Why is this desirable? It's a unit test. A new instance of this class is created for every single test. Rather than providing clarity that this can be called without accessing instance data, it instead suggests that this can be publicly called by any test. Further in that regard, why aren't these test methods all protected as well? This is the base class for unit tests, so everything will derive from it.
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { On 2015/02/03 02:16:27, Ryan Sleevi wrote: > On 2015/02/03 01:26:32, yhirano wrote: > > When the message loop is quit without calling StashCookiesHelper, you need to > > fail the test because this function does not work as expected. In order to > > detect it you need a boolean. > > I still find this code not up to the degree that we expect of //net, and in > particular, too "novel" in the way it deals with threading. > > base::Unretained() remains an anti-pattern that I will flag in reviews, because > it causes way too much reader difficulty in reasoning about the correctness of > the code. The added nesting of the run_loop_ is similarly difficult to > comprehend. Sorry, I still don't understand. I understand your point that it might be possible that message loop is quit without calling the callback. I said that your solution was insufficient. If you care about the case, you need to notify that to the caller. I don't understand your point about threading: are you talking about the case when the callback runs on another thread?
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:52: static std::vector<std::string> NoSubProtocols() { On 2015/02/03 02:16:27, Ryan Sleevi wrote: > On 2015/02/03 01:26:32, yhirano wrote: > > I'd like to leave it be because we can see more easily that no instance > > information is used in the implementation. > > Why is this desirable? It's a unit test. A new instance of this class is created > for every single test. Rather than providing clarity that this can be called > without accessing instance data, it instead suggests that this can be publicly > called by any test. > > Further in that regard, why aren't these test methods all protected as well? > This is the base class for unit tests, so everything will derive from it. I agree with yhirano that this method should be static. Methods which read/write instance data must be clearly distinguished from those which don't. I don't think it matters whether it is public or protected. It would be pointless to use it in a test which doesn't inherit from this class, but it would not break anything.
On 2015/02/03 05:34:36, Adam Rice wrote: > https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... > File net/websockets/websocket_stream_test_util.h (right): > > https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... > net/websockets/websocket_stream_test_util.h:52: static std::vector<std::string> > NoSubProtocols() { > On 2015/02/03 02:16:27, Ryan Sleevi wrote: > > On 2015/02/03 01:26:32, yhirano wrote: > > > I'd like to leave it be because we can see more easily that no instance > > > information is used in the implementation. > > > > Why is this desirable? It's a unit test. A new instance of this class is > created > > for every single test. Rather than providing clarity that this can be called > > without accessing instance data, it instead suggests that this can be publicly > > called by any test. > > > > Further in that regard, why aren't these test methods all protected as well? > > This is the base class for unit tests, so everything will derive from it. > > I agree with yhirano that this method should be static. Methods which read/write > instance data must be clearly distinguished from those which don't. Sorry, I feel I must still disagree. I think it's a matter of perspective here - the argument against marking these static is that it implies they read/write global data (or may do so), and I think that is also counter-intuitive. > I don't think it matters whether it is public or protected. Great, then make it protected please. You should always prefer the most restrictive visibility possible.
https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:6: #define NET_WEBSOCKETS_WEBSOCKET_STREAM_TEST_UTIL_H_ This file needs to be renamed to websocket_stream_test_base.h
Patchset #9 (id:240001) has been deleted
rsleevi@, I'm sorry for bothering you, can you take another look? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:29: CHECK(cookie_header.empty() || EndsWith(cookie_header, "\r\n", true)); On 2015/02/02 20:14:41, Ryan Sleevi wrote: > Comment explaining why? Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:38: static std::string AddCRLFIfNotEmpty(const std::string& s) { On 2015/02/02 20:14:41, Ryan Sleevi wrote: > No need to make this static. Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:43: class ClientUseCookieParameter { On 2015/02/02 20:14:41, Ryan Sleevi wrote: > On 2015/02/02 05:39:57, yhirano wrote: > > I made this (and the other) class POD. > > Make this a struct then, since you're following struct rules - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C... Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:70: class RunLoop { On 2015/02/02 05:29:56, yhirano wrote: > I will fix this helper if necessary after the GetCookies discussion settles. Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:87: class ServerSetCookieParameter { On 2015/02/02 20:14:41, Ryan Sleevi wrote: > ditto struct Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:111: struct GetCookiesHelper { Although I am not 100% sure what you suggested, but I did my best to presume. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:489: ValuesIn(kClientUseCookieParameters)); On 2015/02/02 20:14:41, Ryan Sleevi wrote: > Move this to line 358 - keep your code & instantiations close. Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:7: #include <algorithm> On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Unnecessary? Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:10: #include <vector> On 2015/02/02 20:14:41, Ryan Sleevi wrote: > These three are in your header, unnecessary. Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:12: #include "base/compiler_specific.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > unnecessary Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:14: #include "net/base/net_errors.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > unnecessary Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:17: #include "net/socket/client_socket_handle.h" On 2015/02/02 20:14:41, Ryan Sleevi wrote: > unnecessary Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:18: #include "net/socket/socket_test_util.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > unnecessary? Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:19: #include "net/test/cert_test_util.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > unnecessary Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:20: #include "net/url_request/url_request_test_util.h" On 2015/02/02 20:14:41, Ryan Sleevi wrote: > unnecessary? Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:21: #include "net/websockets/websocket_basic_handshake_stream.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > unnecessary? For WebSocketBasicHandshakeStream https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.cc:25: #include "net/websockets/websocket_test_util.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > unnecessary, in your header Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:6: #define NET_WEBSOCKETS_WEBSOCKET_STREAM_TEST_UTIL_H_ On 2015/02/07 02:32:46, Ryan Sleevi wrote: > This file needs to be renamed to websocket_stream_test_base.h websocket_stream_create_test_base.h https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:15: #include "net/http/http_request_headers.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Forward declare Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:16: #include "net/http/http_response_headers.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Forward declare Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:17: #include "net/socket/socket_test_util.h" On 2015/02/02 20:14:43, Ryan Sleevi wrote: > unnecessary? For ScopedWebSocketEndpointZeroUnlockDelay https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:18: #include "net/websockets/websocket_handshake_request_info.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Forward declare Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:19: #include "net/websockets/websocket_handshake_response_info.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Forward declare Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:21: #include "url/gurl.h" On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Forward declare removed https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:25: NET_EXPORT_PRIVATE class WebSocketStreamCreateTestBase { On 2015/02/02 20:14:42, Ryan Sleevi wrote: > class NET_EXPORT_PRIVATE WebSocketStreamCreateTestBase Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:26: class TestConnectDelegate; On 2015/02/02 20:14:42, Ryan Sleevi wrote: > STYLE: Inappropriate forward declaration. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Format > > private goes last / end of file. Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:39: scoped_ptr<base::Timer> timer); On 2015/02/02 20:14:43, Ryan Sleevi wrote: > IWYU: Include timer header Done. You suggested forward-declaring WebSocketHandshakeRequest used in scoped_ptr<WebSocketHandshakeRequestInfo> request_info_; and suggested including timer.h for scoped_ptr<base::Timer>. Can you tell me the difference between them? https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:49: static void RunUntilIdle() { base::RunLoop().RunUntilIdle(); } On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Don't inline Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:51: // A simple function to make the tests more readable. Creates an empty vector. On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Drop second sentance Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:52: static std::vector<std::string> NoSubProtocols() { On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Is static really necessary? Ditto line 49. Done (static). Regarding public / protected, some existing tests are assuming that some methods are public. I don't want to fix them in this CL. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:53: return std::vector<std::string>(); On 2015/02/02 20:14:42, Ryan Sleevi wrote: > Don't inline Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:67: SSLInfo ssl_info_; On 2015/02/02 20:14:43, Ryan Sleevi wrote: > IWYU: Include SSLInfo header Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:69: ScopedVector<SSLSocketDataProvider> ssl_data_; On 2015/02/02 20:14:42, Ryan Sleevi wrote: > IWYU: Include base/memory/scoped_vector Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:71: }; On 2015/02/02 20:14:42, Ryan Sleevi wrote: > private: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_test_util.cc (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_test_util.cc:39: return WebSocketStandardRequestWithCookies(path, host, origin, "", On 2015/02/02 20:14:43, Ryan Sleevi wrote: > s/""/std::string()/ Done. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_test_util.h:68: extern std::string WebSocketStandardRequestWithCookies( On 2015/02/02 20:14:43, Ryan Sleevi wrote: > Why are these all flagged extern? Done.
Looking much better, thanks for paying close attention to these. https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... File net/websockets/websocket_stream_test_util.h (right): https://codereview.chromium.org/869073002/diff/160001/net/websockets/websocke... net/websockets/websocket_stream_test_util.h:39: scoped_ptr<base::Timer> timer); On 2015/02/09 10:50:15, yhirano wrote: > On 2015/02/02 20:14:43, Ryan Sleevi wrote: > > IWYU: Include timer header > > Done. > You suggested forward-declaring WebSocketHandshakeRequest used in > scoped_ptr<WebSocketHandshakeRequestInfo> request_info_; and suggested including > timer.h for scoped_ptr<base::Timer>. Can you tell me the difference between > them? Forward declaring should also work here. I was noting that there was no definition of base::Timer, either in forward declare or explicit headers. Sorry for the ambiguity. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:26: const char* const kNoCookieHeader = ""; const char kNoCookieHeader[] = ""; https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:82: base::MessageLoop::current()->PostTask(FROM_HERE, task); base::ThreadTaskRunnerHandle::Get()->PostTask(...); https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:114: DCHECK(weak_result.get()); Note: You can actually just do *weak_is_called = true; *weak_result = cookies; In as much as they'll DCHECK() if anything else cancelled the loop (such that your WeakPtrs went out of scope). This highlights what would be expected - that there's a developer error in play. This is a stronger guarantee than using naked pointers (which would just clobber memory that may or may not get picked up by ASAN/MSAN), and a weaker guarantee than gracefully handling (because it'll be a NULL pointer deref in Release mode), but it's a more explicit statement codewise that "These weakptr's should never ever go away" https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_create_test_base.cc (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:36: }; private: DISALLOW_COPY_AND_ASSIGN(...); https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:59: void OnFinishOpeningHandshake( nit newline after } https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:65: void OnSSLCertificateError( nit: newline after } https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:77: }; DISALLOW_COPY_AND_ASSIGN(...) https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:84: } git cl format do this? I thought it preferred {} still. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:96: ssl_data_.clear(); Doesn't it make more sense to remove line 93 and use weak_clear() instead here? (Clears, but doesn't delete any elements) https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:103: stream_request_ = ::net::CreateAndConnectStreamForTesting( You're in net::, no need for ::net:: here https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_create_test_base.h (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:30: class NET_EXPORT_PRIVATE WebSocketStreamCreateTestBase { Does this actually need to be NET_EXPORT_PRIVATE? Nothing from this CL makes it immediately evident that this is the case. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:47: const HttpResponseHeaders& headers); Per http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over... , this is quite ambiguious and discouraged. RequestHeadersToVector ResponseHeadersToVector (RequestHeaders and ResponseHeaders are not related in the same way that, say, std::string and const char* are) https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:52: void RunUntilIdle(); I'd love to see this "confusing because it duplicates existing Chrome functionality that it probably shouldn't be" method documented. (After reading through the test) Actually, I'd like to see this method removed. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:71: ScopedWebSocketEndpointZeroUnlockDelay zero_unlock_delay_; I'd love to see these documented.
Patchset #12 (id:320001) has been deleted
Patchset #11 (id:300001) has been deleted
Patchset #10 (id:280001) has been deleted
https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:26: const char* const kNoCookieHeader = ""; On 2015/02/10 01:26:48, Ryan Sleevi wrote: > const char kNoCookieHeader[] = ""; Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:82: base::MessageLoop::current()->PostTask(FROM_HERE, task); On 2015/02/10 01:26:48, Ryan Sleevi wrote: > base::ThreadTaskRunnerHandle::Get()->PostTask(...); Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:114: DCHECK(weak_result.get()); On 2015/02/10 01:26:48, Ryan Sleevi wrote: > Note: You can actually just do > > *weak_is_called = true; > *weak_result = cookies; > > In as much as they'll DCHECK() if anything else cancelled the loop (such that > your WeakPtrs went out of scope). This highlights what would be expected - that > there's a developer error in play. > > This is a stronger guarantee than using naked pointers (which would just clobber > memory that may or may not get picked up by ASAN/MSAN), and a weaker guarantee > than gracefully handling (because it'll be a NULL pointer deref in Release > mode), but it's a more explicit statement codewise that "These weakptr's should > never ever go away" Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_create_test_base.cc (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:36: }; On 2015/02/10 01:26:49, Ryan Sleevi wrote: > private: > DISALLOW_COPY_AND_ASSIGN(...); Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:59: void OnFinishOpeningHandshake( On 2015/02/10 01:26:49, Ryan Sleevi wrote: > nit newline after } Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:65: void OnSSLCertificateError( On 2015/02/10 01:26:49, Ryan Sleevi wrote: > nit: newline after } Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:77: }; On 2015/02/10 01:26:49, Ryan Sleevi wrote: > DISALLOW_COPY_AND_ASSIGN(...) Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:84: } On 2015/02/10 01:26:49, Ryan Sleevi wrote: > git cl format do this? I thought it preferred {} still. Yes, it likes this style. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:96: ssl_data_.clear(); On 2015/02/10 01:26:48, Ryan Sleevi wrote: > Doesn't it make more sense to remove line 93 and use weak_clear() instead here? > > (Clears, but doesn't delete any elements) Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.cc:103: stream_request_ = ::net::CreateAndConnectStreamForTesting( On 2015/02/10 01:26:49, Ryan Sleevi wrote: > You're in net::, no need for ::net:: here Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_create_test_base.h (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:30: class NET_EXPORT_PRIVATE WebSocketStreamCreateTestBase { On 2015/02/10 01:26:49, Ryan Sleevi wrote: > Does this actually need to be NET_EXPORT_PRIVATE? Nothing from this CL makes it > immediately evident that this is the case. Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:47: const HttpResponseHeaders& headers); On 2015/02/10 01:26:49, Ryan Sleevi wrote: > Per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over... > , this is quite ambiguious and discouraged. > > RequestHeadersToVector > ResponseHeadersToVector > > (RequestHeaders and ResponseHeaders are not related in the same way that, say, > std::string and const char* are) Done. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:52: void RunUntilIdle(); On 2015/02/10 01:26:49, Ryan Sleevi wrote: > I'd love to see this "confusing because it duplicates existing Chrome > functionality that it probably shouldn't be" method documented. > > (After reading through the test) > Actually, I'd like to see this method removed. Replaced with base::RunLoop().RunUntilIdle(), though I'm not sure if it is a good idea. https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:71: ScopedWebSocketEndpointZeroUnlockDelay zero_unlock_delay_; On 2015/02/10 01:26:49, Ryan Sleevi wrote: > I'd love to see these documented. Done.
Patchset #11 (id:360001) has been deleted
https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... File net/websockets/websocket_stream_create_test_base.h (right): https://codereview.chromium.org/869073002/diff/260001/net/websockets/websocke... net/websockets/websocket_stream_create_test_base.h:52: void RunUntilIdle(); On 2015/02/10 03:14:03, yhirano wrote: > On 2015/02/10 01:26:49, Ryan Sleevi wrote: > > I'd love to see this "confusing because it duplicates existing Chrome > > functionality that it probably shouldn't be" method documented. > > > > (After reading through the test) > > Actually, I'd like to see this method removed. > > Replaced with base::RunLoop().RunUntilIdle(), though I'm not sure if it is a > good idea. Reverted. Adam, as we discussed, I created "WaitUntilDone" in PS11 and started to used it in websocket_stream_cookie_test.cc. What do you think about it? If you are happy with it, I would like to replace RunUntilIdle with it.
On 2015/02/10 05:51:53, yhirano wrote: > Adam, as we discussed, I created "WaitUntilDone" in PS11 and started to used it > in websocket_stream_cookie_test.cc. What do you think about it? If you are happy > with it, I would like to replace RunUntilIdle with it. I like it. The code is clean and the semantics are very precise. Maybe we should rename it "WaitUntilConnectDone" just to make it clearer? I had forgotten that we still need to use RunUntilIdle() in the destuctors to allow the endpoints to be unlocked. Still, if those are the only places we need to use it it will still be pretty clean.
On 2015/02/10 05:59:40, Adam Rice wrote: > On 2015/02/10 05:51:53, yhirano wrote: > > Adam, as we discussed, I created "WaitUntilDone" in PS11 and started to used > it > > in websocket_stream_cookie_test.cc. What do you think about it? If you are > happy > > with it, I would like to replace RunUntilIdle with it. > > I like it. The code is clean and the semantics are very precise. Maybe we should > rename it "WaitUntilConnectDone" just to make it clearer? > > I had forgotten that we still need to use RunUntilIdle() in the destuctors to > allow the endpoints to be unlocked. Still, if those are the only places we need > to use it it will still be pretty clean. Thanks for pointing it out, I think we can use simple base::RunLoop().RunUntilIdle() in that case. So I removed RunUntilIdle function from WebSocketStreamCreateTestBase. I found in some tests we can't use WaitUntilConnectDone - cancellation tests and self signed cert tests. I use base::RunLoop().RunUntilIdle() in those tests, too. > Maybe we should > rename it "WaitUntilConnectDone" just to make it clearer? Done.
LGTM % two nits. Thanks for putting up with my persistent pedantry. https://codereview.chromium.org/869073002/diff/420001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/420001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:79: if (weak_is_called.get()) { Note: My previous comment was to remove this |if| conditional completely (delete line 79 and 83) It's a programmer error if |weak_is_called| is ever false, and calling *weak_is_called line 80 is sufficient to DCHECK this in debug, and be a null ptr de-ref in release (and without causing memory corruption, as would happen with a naked pointer) https://codereview.chromium.org/869073002/diff/420001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:113: if (weak_is_called.get()) { ditto
New patchsets have been uploaded after l-g-t-m from rsleevi@chromium.org
Thank you for taking your time for the review. I should have been more careful, and I will be. https://codereview.chromium.org/869073002/diff/420001/net/websockets/websocke... File net/websockets/websocket_stream_cookie_test.cc (right): https://codereview.chromium.org/869073002/diff/420001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:79: if (weak_is_called.get()) { On 2015/02/10 21:37:34, Ryan Sleevi wrote: > Note: My previous comment was to remove this |if| conditional completely (delete > line 79 and 83) > > It's a programmer error if |weak_is_called| is ever false, and calling > *weak_is_called line 80 is sufficient to DCHECK this in debug, and be a null ptr > de-ref in release (and without causing memory corruption, as would happen with a > naked pointer) Done. https://codereview.chromium.org/869073002/diff/420001/net/websockets/websocke... net/websockets/websocket_stream_cookie_test.cc:113: if (weak_is_called.get()) { On 2015/02/10 21:37:34, Ryan Sleevi wrote: > ditto Done.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869073002/440001
Message was sent while issue was closed.
Committed patchset #14 (id:440001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/01a5d66a26a60f970d9a8b595d3b463d6c876c00 Cr-Commit-Position: refs/heads/master@{#315930} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
