Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715393002:
Add tests for HttpRequestCompletionErrorCodes (Closed)
DescriptionAdd tests for HttpRequestCompletionErrorCodes
This CL increases coverage for HttpRequestCompletionErrorCodes metrics.
TEST = ChromeNetworkDelegateTest
Review-Url: https://codereview.chromium.org/2715393002
Cr-Commit-Position: refs/heads/master@{#455793}
Committed: https://chromium.googlesource.com/chromium/src/+/56da1fdfc0061b0eb2f800ecf7018998098f4e4d
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add tests for HttpRequestCompletionErrorCodes #
Total comments: 3
Patch Set 3 : Add tests for HttpRequestCompletionErrorCodes #Messages
Total messages: 21 (11 generated)
jahagird@amazon.com changed reviewers: + mef@chromium.org
Hi, Could you please take a look at this change. - Amey
Description was changed from ========== Add tests for HttpRequestCompletionErrorCodes This CL increases coverage for HttpRequestCompletionErrorCodes metrics. TEST = ChromeNetworkDelegateTest ========== to ========== Add tests for HttpRequestCompletionErrorCodes This CL increases coverage for HttpRequestCompletionErrorCodes metrics. TEST = ChromeNetworkDelegateTest ==========
jahagird@amazon.com changed reviewers: + asanka@chromium.org - mef@chromium.org
Hello asanka@, Could you please take a look at this change. - Amey
https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:54: "Net.HttpRequestCompletionErrorCodes"; Use something like: const char kHttpRequestCompletionErrorCodes[] = "..."; ... which accomplishes what you are doing with the extra 'const's without introducing a pointer. Also let's move this to the function that uses these constants. Keeping them here would only be useful if multiple tests were using these constants. https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:249: bool main_frame; is_main_frame https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:261: for (size_t i = 0; i < arraysize(tests); ++i) { use a range based iterator. I.e. something like: for (const auto& test : tests) { ... // and use 'test' to refer to the test case instead of 'tests[i]'.
PTAL https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:54: "Net.HttpRequestCompletionErrorCodes"; On 2017/03/07 19:15:47, asanka wrote: > Use something like: > > const char kHttpRequestCompletionErrorCodes[] = "..."; > > ... which accomplishes what you are doing with the extra 'const's without > introducing a pointer. > > > Also let's move this to the function that uses these constants. Keeping them > here would only be useful if multiple tests were using these constants. Acknowledged. https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:249: bool main_frame; On 2017/03/07 19:15:47, asanka wrote: > is_main_frame Acknowledged. https://codereview.chromium.org/2715393002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:261: for (size_t i = 0; i < arraysize(tests); ++i) { On 2017/03/07 19:15:47, asanka wrote: > use a range based iterator. I.e. something like: > > for (const auto& test : tests) { > ... > // and use 'test' to refer to the test case instead of 'tests[i]'. Sweet! Somehow, I always thought that range based loops are just for std::iterators.
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. Thanks! https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_unittest.cc:248: } tests[] = { kTests https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_unittest.cc:256: const char http_request_completion_error_code[] = kHttpRequestCompletionErrorCode for constants. https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_unittest.cc:258: const char http_request_completion_error_code_main_frame[] = kHttpRequestCompletionErrorCodeMainFrame
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/08 21:23:30, asanka wrote: > LGTM. Thanks! > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > File chrome/browser/net/chrome_network_delegate_unittest.cc (right): > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > chrome/browser/net/chrome_network_delegate_unittest.cc:248: } tests[] = { > kTests > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > chrome/browser/net/chrome_network_delegate_unittest.cc:256: const char > http_request_completion_error_code[] = > kHttpRequestCompletionErrorCode for constants. > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > chrome/browser/net/chrome_network_delegate_unittest.cc:258: const char > http_request_completion_error_code_main_frame[] = > kHttpRequestCompletionErrorCodeMainFrame Thanks for the feedback. I guess I am confused about style here. https://google.github.io/styleguide/cppguide.html#Constant_Names lead me to believe that kNaming convention rule applies only for **static storage duration** and for function level object where we just use normal naming convention (e.g. const auto& blah or https://cs.chromium.org/chromium/src/net/dns/dns_query_unittest.cc?type=cs&q=...)
On 2017/03/08 23:10:23, Amey J wrote: > On 2017/03/08 21:23:30, asanka wrote: > > LGTM. Thanks! > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > File chrome/browser/net/chrome_network_delegate_unittest.cc (right): > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > chrome/browser/net/chrome_network_delegate_unittest.cc:248: } tests[] = { > > kTests > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > chrome/browser/net/chrome_network_delegate_unittest.cc:256: const char > > http_request_completion_error_code[] = > > kHttpRequestCompletionErrorCode for constants. > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > chrome/browser/net/chrome_network_delegate_unittest.cc:258: const char > > http_request_completion_error_code_main_frame[] = > > kHttpRequestCompletionErrorCodeMainFrame > > Thanks for the feedback. I guess I am confused about style here. > https://google.github.io/styleguide/cppguide.html#Constant_Names lead me to > believe that kNaming convention rule applies only for **static storage > duration** and for function level object where we just use normal naming > convention (e.g. const auto& blah or > https://cs.chromium.org/chromium/src/net/dns/dns_query_unittest.cc?type=cs&q=...) The kNaming convention is optional as the style guide says. It's a matter of consistency with surrounding code and preference. There's not a lot of weight to the consistency argument since there aren't many examples in this code. So either way is fine. You'll see that since the convention is optional, it's not consistently followed. The conventions differ sometimes in adjacent code (e.g.: https://cs.chromium.org/chromium/src/net/http/http_network_transaction_unitte...). The CL L-G-T-M either way.
On 2017/03/09 02:00:35, asanka wrote: > On 2017/03/08 23:10:23, Amey J wrote: > > On 2017/03/08 21:23:30, asanka wrote: > > > LGTM. Thanks! > > > > > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > > File chrome/browser/net/chrome_network_delegate_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > > chrome/browser/net/chrome_network_delegate_unittest.cc:248: } tests[] = { > > > kTests > > > > > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > > chrome/browser/net/chrome_network_delegate_unittest.cc:256: const char > > > http_request_completion_error_code[] = > > > kHttpRequestCompletionErrorCode for constants. > > > > > > > > > https://codereview.chromium.org/2715393002/diff/20001/chrome/browser/net/chro... > > > chrome/browser/net/chrome_network_delegate_unittest.cc:258: const char > > > http_request_completion_error_code_main_frame[] = > > > kHttpRequestCompletionErrorCodeMainFrame > > > > Thanks for the feedback. I guess I am confused about style here. > > https://google.github.io/styleguide/cppguide.html#Constant_Names lead me to > > believe that kNaming convention rule applies only for **static storage > > duration** and for function level object where we just use normal naming > > convention (e.g. const auto& blah or > > > https://cs.chromium.org/chromium/src/net/dns/dns_query_unittest.cc?type=cs&q=...) > > The kNaming convention is optional as the style guide says. It's a matter of > consistency with surrounding code and preference. There's not a lot of weight to > the consistency argument since there aren't many examples in this code. So > either way is fine. > > You'll see that since the convention is optional, it's not consistently > followed. The conventions differ sometimes in adjacent code (e.g.: > https://cs.chromium.org/chromium/src/net/http/http_network_transaction_unitte...). > > The CL L-G-T-M either way. Thanks for weighing in. Since some of the adjacent code follows it, I have changed it to use same kName style.
The CQ bit was checked by jahagird@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2715393002/#ps40001 (title: "Add tests for HttpRequestCompletionErrorCodes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489080211358530,
"parent_rev": "d80c4b7df762231c28fd4349a35cd4e4408d635f", "commit_rev":
"56da1fdfc0061b0eb2f800ecf7018998098f4e4d"}
Message was sent while issue was closed.
Description was changed from ========== Add tests for HttpRequestCompletionErrorCodes This CL increases coverage for HttpRequestCompletionErrorCodes metrics. TEST = ChromeNetworkDelegateTest ========== to ========== Add tests for HttpRequestCompletionErrorCodes This CL increases coverage for HttpRequestCompletionErrorCodes metrics. TEST = ChromeNetworkDelegateTest Review-Url: https://codereview.chromium.org/2715393002 Cr-Commit-Position: refs/heads/master@{#455793} Committed: https://chromium.googlesource.com/chromium/src/+/56da1fdfc0061b0eb2f800ecf701... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/56da1fdfc0061b0eb2f800ecf701... |
