Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9368031:
Expose a static configuration value for the host to use for URLRequestTestHTTP tests. (Closed)
|
Created:
8 years, 10 months ago by erikwright (departed) Modified:
8 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionExpose a static configuration value for the host to use for URLRequestTestHTTP tests.
This defaults to 127.0.0.1 but may be modified, affecting all test cases in the URLRequestTestHTTP suite.
BUG=114369
TEST=net_unittests still pass.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123057
Patch Set 1 #Patch Set 2 : Parameterize ALL the HTTP servers! #
Total comments: 11
Patch Set 3 : Respond to review comments. #
Total comments: 1
Patch Set 4 : Scoped override object rather than setter. #
Total comments: 15
Patch Set 5 : Make overrides stackable, and other review comments. #
Total comments: 2
Patch Set 6 : Respond to nits, fix compiler error. #
Messages
Total messages: 16 (0 generated)
Depends on these changes to testserver.py and .cc: http://codereview.chromium.org/9368030/ http://codereview.chromium.org/9369029/ One more change just needs some tidying up (to set the parameter during chrome_frame_net_tests initialization). Motivation is mysterious failures in chrome_frame_net_tests when they are run over loopback adapter. These failures go away when another adapter is used. The failure has been localized to somewhere between socket.read() in python and ws2_32!send in urlmon (via Internet Explorer). Further diagnosis will probably require kernel debugging. This workaround fixes the issue (dozens of try jobs without encountering the failure, which otherwise occurred nearly every run) and will bring us closer to getting the chrome_frame_net_tests back on the waterfall.
http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... File net/url_request/url_request_test_util.h (right): http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.h:245: void set_url_request_test_http_host(const std::string& host); Global, I don't like that. If anything, use a scoper to guarantee all tests leave consistent state.
On 2012/02/15 17:15:44, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > File net/url_request/url_request_test_util.h (right): > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > net/url_request/url_request_test_util.h:245: void > set_url_request_test_http_host(const std::string& host); > Global, I don't like that. If anything, use a scoper to guarantee all tests > leave consistent state. It is not set during a test run, but in the custom chrome_frame_net_tests TestRunner. It is never reset (in that mode, we wish it to apply to all tests). See the follow-up CL (in-progress): http://codereview.chromium.org/9401013/ I don't see any way around sharing some global state between the test runner and the tests, but I would be happy for an alternative.
LGTM after addressing comments. This seems a bit hockey, and I am not convinced the local interface is the root problem (although I haven't read that thread in detail yet). Either way, I am not going to stand in your way to smoothing over flaky tests! Hopefully we will figure out more conclusively why this change seems to help, and be able to remove this. AFAIK we haven't seen this issue in regular Chrome. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:509: char url_request_test_http_host_[256] = "127.0.0.1"; First off, please prefix globals with "g_". Secondly, since this is test code, I think it is fine to have static initializers. So in other words, you can get away with defining this global as a std::string and not needing to do the fancy copying of C strings. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:516: url_request_test_http_host_[arraysize(url_request_test_http_host_) - 1] = 0; nit: If we have a host literal string longer than 255 something is not right :-) This is fine as-is, but you might alternately consider asserting the length is smaller at the start. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:519: char* url_request_test_http_host() { can you return a const char* so caller can't mutate the array? http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_unittest.cc:70: class LocalHttpServer : public TestServer { nit: I suggest calling LocalHttpTestServer http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_unittest.cc:3610: TestServer test_server_; You don't need to make the same change here (for the FTP server)?
http://codereview.chromium.org/9368031/diff/8001/net/url_request/url_request_... File net/url_request/url_request_test_util.h (right): http://codereview.chromium.org/9368031/diff/8001/net/url_request/url_request_... net/url_request/url_request_test_util.h:245: void set_url_request_test_http_host(const std::string& host); Please make this scoped as requested in my previous comment. I have seen it many times that API like this led to a huge mess afterwards. A scoped object will prevent most of the known issues.
On 2012/02/16 15:37:24, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/9368031/diff/8001/net/url_request/url_request_... > File net/url_request/url_request_test_util.h (right): > > http://codereview.chromium.org/9368031/diff/8001/net/url_request/url_request_... > net/url_request/url_request_test_util.h:245: void > set_url_request_test_http_host(const std::string& host); > Please make this scoped as requested in my previous comment. > > I have seen it many times that API like this led to a huge mess afterwards. A > scoped object will prevent most of the known issues. Sorry, I think I didn't quite understand what you were suggesting in your first comment. But I believe that I do now. PTAL.
http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:509: char url_request_test_http_host_[256] = "127.0.0.1"; On 2012/02/16 02:41:00, eroman wrote: > First off, please prefix globals with "g_". > > Secondly, since this is test code, I think it is fine to have static > initializers. So in other words, you can get away with defining this global as a > std::string and not needing to do the fancy copying of C strings. Done. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:516: url_request_test_http_host_[arraysize(url_request_test_http_host_) - 1] = 0; On 2012/02/16 02:41:00, eroman wrote: > nit: If we have a host literal string longer than 255 something is not right :-) > This is fine as-is, but you might alternately consider asserting the length is > smaller at the start. N/A. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:519: char* url_request_test_http_host() { On 2012/02/16 02:41:00, eroman wrote: > can you return a const char* so caller can't mutate the array? Done. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_unittest.cc:70: class LocalHttpServer : public TestServer { On 2012/02/16 02:41:00, eroman wrote: > nit: I suggest calling LocalHttpTestServer Done. http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... net/url_request/url_request_unittest.cc:3610: TestServer test_server_; On 2012/02/16 02:41:00, eroman wrote: > You don't need to make the same change here (for the FTP server)? No. Chrome Frame never handles requests besides HTTP and HTTPS. And the types of failures we are seeing, for whatever reason, never occur over HTTPS (which is lucky because that would lead to all kinds of certificate issues that we get around by having certs for 127.0.0.1). In fact, we currently don't run the HTTPS tests at all in chrome_frame_net_tests (probably why they aren't failing) because they require manually installing the test CA in IE.
On 2012/02/16 02:41:00, eroman wrote: > LGTM after addressing comments. > > This seems a bit hockey, and I am not convinced the local interface is the root > problem (although I haven't read that thread in detail yet). > > Either way, I am not going to stand in your way to smoothing over flaky tests! > Hopefully we will figure out more conclusively why this change seems to help, > and be able to remove this. AFAIK we haven't seen this issue in regular Chrome. > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > File net/url_request/url_request_test_util.cc (right): > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > net/url_request/url_request_test_util.cc:509: char > url_request_test_http_host_[256] = "127.0.0.1"; > First off, please prefix globals with "g_". > > Secondly, since this is test code, I think it is fine to have static > initializers. So in other words, you can get away with defining this global as a > std::string and not needing to do the fancy copying of C strings. > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > net/url_request/url_request_test_util.cc:516: > url_request_test_http_host_[arraysize(url_request_test_http_host_) - 1] = 0; > nit: If we have a host literal string longer than 255 something is not right :-) > This is fine as-is, but you might alternately consider asserting the length is > smaller at the start. > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > net/url_request/url_request_test_util.cc:519: char* url_request_test_http_host() > { > can you return a const char* so caller can't mutate the array? > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > File net/url_request/url_request_unittest.cc (right): > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > net/url_request/url_request_unittest.cc:70: class LocalHttpServer : public > TestServer { > nit: I suggest calling LocalHttpTestServer > > http://codereview.chromium.org/9368031/diff/4001/net/url_request/url_request_... > net/url_request/url_request_unittest.cc:3610: TestServer test_server_; > You don't need to make the same change here (for the FTP server)? Hi Eric, (sorry, thought I had already sent this). Agree that I don't like needing to add this. Short of some global state there is no way to communicate from the Test Runner to the tests. Alternatively I could: 1) Extract the test definitions into parameterized tests (i.e., they would go in a .h and there would be an instance in url_request_unittest.cc and another instance in chrome_frame_url_request_unittest.cc) 2) Make it a static member of the class UrlRequestTestHTTP (and move the declaration of this class and its subclasses to a header) I don't think either is better than this. As to what the root cause is, I admit that I don't know. If you click through to the bug, I have gathered the data I have so far. I do know that this workaround solves the problem, so thanks for your support in getting the tests back on the waterfall while we investigate further.
http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:508: std::string CustomUrlRequestTestHttpHost::value_("127.0.0.1"); nit: // static above http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:510: CustomUrlRequestTestHttpHost::CustomUrlRequestTestHttpHost( Where is this instantiated? http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:516: value_ = "127.0.0.1"; This doesn't allow stacking. It'd be better to store the original value and restore it afterwards. http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:519: const std::string& CustomUrlRequestTestHttpHost::value() { nit: // static above http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... File net/url_request/url_request_test_util.h (right): http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.h:245: class CustomUrlRequestTestHttpHost { nit: Include "Scoped" somewhere in the name. Also, whole class with _no_ comments? http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.h:251: static std::string value_; nit: DISALLOW_COPY_AND_ASSIGN.
http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:510: CustomUrlRequestTestHttpHost::CustomUrlRequestTestHttpHost( On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > Where is this instantiated? In fake_external_tab.cc. I have not yet uploaded the patch to that CL that adjusts it to use this. http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:516: value_ = "127.0.0.1"; On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > This doesn't allow stacking. It'd be better to store the original value and > restore it afterwards. Presumably it will be OK to support naive (i.e., fully nested) stacking?
Hi Pawel, PTAL. Thanks, Erik http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:508: std::string CustomUrlRequestTestHttpHost::value_("127.0.0.1"); On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > nit: // static above It doesn't compile with a static here: 1>.\url_request\url_request_test_util.cc(508) : error C2720: 'ScopedCustomUrlRequestTestHttpHost::value_' : 'static ' storage-class specifier illegal on members http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:510: CustomUrlRequestTestHttpHost::CustomUrlRequestTestHttpHost( On 2012/02/17 11:15:21, erikwright wrote: > On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > > Where is this instantiated? > > In fake_external_tab.cc. I have not yet uploaded the patch to that CL that > adjusts it to use this. See http://codereview.chromium.org/9401013/diff/8001/chrome_frame/test/net/fake_e... http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:516: value_ = "127.0.0.1"; On 2012/02/17 11:15:21, erikwright wrote: > On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > > This doesn't allow stacking. It'd be better to store the original value and > > restore it afterwards. > > Presumably it will be OK to support naive (i.e., fully nested) stacking? Done. http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:519: const std::string& CustomUrlRequestTestHttpHost::value() { On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > nit: // static above Doesn't compile: 1>.\url_request\url_request_test_util.cc(522) : error C2724: 'ScopedCustomUrlRequestTestHttpHost::value' : 'static' should not be used on member functions defined at file scope http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... File net/url_request/url_request_test_util.h (right): http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.h:245: class CustomUrlRequestTestHttpHost { On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > nit: Include "Scoped" somewhere in the name. > > Also, whole class with _no_ comments? Done. http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.h:251: static std::string value_; On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > nit: DISALLOW_COPY_AND_ASSIGN. Done.
lgtm
LGTM http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/13001/net/url_request/url_request... net/url_request/url_request_test_util.cc:508: std::string CustomUrlRequestTestHttpHost::value_("127.0.0.1"); On 2012/02/17 17:48:12, erikwright wrote: > On 2012/02/17 11:06:36, Paweł Hajdan Jr. wrote: > > nit: // static above > > It doesn't compile with a static here: > > 1>.\url_request\url_request_test_util.cc(508) : error C2720: > 'ScopedCustomUrlRequestTestHttpHost::value_' : 'static ' storage-class specifier > illegal on members We have a convention to add it as a *comment* . http://codereview.chromium.org/9368031/diff/15001/net/url_request/url_request... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/9368031/diff/15001/net/url_request/url_request... net/url_request/url_request_test_util.cc:508: std::string ScopedCustomUrlRequestTestHttpHost::value_("127.0.0.1"); nit: // static above unless it contradicts eroman's comment http://codereview.chromium.org/9368031/diff/15001/net/url_request/url_request... net/url_request/url_request_test_util.cc:522: const std::string& ScopedCustomUrlRequestTestHttpHost::value() { nit: // static above unless it contradicts eroman's comment
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/9368031/19001
Change committed as 123057 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
