Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
448373003:
parameterize http response header tests (Closed)
|
Created:
6 years, 4 months ago by Mostyn Bramley-Moore Modified:
6 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionparameterize http response header tests
Followup to https://codereview.chromium.org/391763002 to allow this
to build on more compilers (which can fail when using structs defined
in funtions in arraysize calls- as mentioned in base/macros.h).
By refactoring to use TEST_P we can skip the manual loop entirely.
BUG=348877
TEST=net_unittests --gtest_filter=Http*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291025
Patch Set 1 #
Total comments: 4
Patch Set 2 : sample parameterized test #Patch Set 3 : parameterize HttpResponseHeadersCacheControlTest.MaxAgeEdgeCases #Patch Set 4 : parameterize common HttpResponseHeadersTest.*'s #Patch Set 5 : parameterize HttpResponseHeadersTest.Persist #Patch Set 6 : parameterize HttpResponseHeadersTest.RequiresValidation #Patch Set 7 : parameterize HttpResponseHeadersTest.Update #Patch Set 8 : parameterize HttpResponseHeadersTest.EnumerateHeaderLines #Patch Set 9 : parameterize HttpResponseHeadersTest.IsRedirect #Patch Set 10 : parameterize HttpResponseHeadersTest.GetContentLength #Patch Set 11 : parameterize HttpResponseHeaders.GetContentRange #Patch Set 12 : parameterize HttpResponseHeadersTest.IsKeepAlive #Patch Set 13 : parameterize HttpResponseHeadersTest.HasStrongValidators #Patch Set 14 : parameterize HttpResponseHeadersTest.AddHeader #Patch Set 15 : parameterize HttpResponseHeadersTest.RemoveHeader #Patch Set 16 : parameterize HttpResponseHeadersTest.RemoveIndividualHeader #Patch Set 17 : parameterize HttpResponseHeadersTest.ReplaceStatus #Patch Set 18 : parameterize HttpResponseHeadersTest.UpdateWithNewRange #Patch Set 19 : misc formatting cleanup #Patch Set 20 : misc cleanup 2 #
Total comments: 4
Patch Set 21 : comment cleanups #
Total comments: 8
Patch Set 22 : final formatting adjustments #Messages
Total messages: 29 (0 generated)
@Adam, tyoshino: does this small improvement look OK?
I was hoping since it passed all the bot tests we no longer needed to use ARRAYSIZE_UNSAFE. Apparently not. https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { It would probably be better to use ARRAYSIZE_UNSAFE in the loop for consistency with the rest of the file.
https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { On 2014/08/11 12:17:54, Adam Rice wrote: > It would probably be better to use ARRAYSIZE_UNSAFE in the loop for consistency > with the rest of the file. I could try converting them all to use arraysize, which would you prefer?
https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { On 2014/08/11 12:24:02, Mostyn Bramley-Moore wrote: > On 2014/08/11 12:17:54, Adam Rice wrote: > > It would probably be better to use ARRAYSIZE_UNSAFE in the loop for > consistency > > with the rest of the file. > > I could try converting them all to use arraysize, which would you prefer? Unless you're willing to refactor all the tests to use TEST_P at the same time, I don't think the disruption would be justified.
https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { On 2014/08/11 12:28:46, Adam Rice wrote: > On 2014/08/11 12:24:02, Mostyn Bramley-Moore wrote: > > On 2014/08/11 12:17:54, Adam Rice wrote: > > > It would probably be better to use ARRAYSIZE_UNSAFE in the loop for > > consistency > > > with the rest of the file. > > > > I could try converting them all to use arraysize, which would you prefer? > > Unless you're willing to refactor all the tests to use TEST_P at the same time, > I don't think the disruption would be justified. I will give that a try, but it might take a day or two until I have time.
On 2014/08/11 12:35:36, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... > File net/http/http_response_headers_unittest.cc (right): > > https://codereview.chromium.org/448373003/diff/1/net/http/http_response_heade... > net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = > { > On 2014/08/11 12:28:46, Adam Rice wrote: > > On 2014/08/11 12:24:02, Mostyn Bramley-Moore wrote: > > > On 2014/08/11 12:17:54, Adam Rice wrote: > > > > It would probably be better to use ARRAYSIZE_UNSAFE in the loop for > > > consistency > > > > with the rest of the file. This sounds fine. > > > > > > I could try converting them all to use arraysize, which would you prefer? > > > > Unless you're willing to refactor all the tests to use TEST_P at the same > time, > > I don't think the disruption would be justified. > > I will give that a try, but it might take a day or two until I have time. Sorry I'm not catching up. What is Adam's suggestion? Not defining named structs and replacing ARRAYSIZE_UNSAFE with arraysize?
On 2014/08/12 18:02:53, tyoshino wrote: > Sorry I'm not catching up. What is Adam's suggestion? Not defining named structs > and replacing ARRAYSIZE_UNSAFE with arraysize? Adam's suggestion was to either change arraysize to ARRAYSIZE_UNSAFE, or do each of the following: 1. define the struct outside the test function and keep the use of arraysize 2. convert all the other uses of ARRAYSIZE_UNSAFE to arraysize 3. refactor all the tests to use TEST_P I'm happy to implement either solution, but it will take me a little longer to refactor to use TEST_P (since I need to read up on it first). Note: I figured out that the real problem is not that the struct is anonymous, but that it's defined inside a function- I will update the CL description.
On 2014/08/12 18:55:36, Mostyn Bramley-Moore wrote: > Adam's suggestion was to either change arraysize to ARRAYSIZE_UNSAFE, or do each > of the following: > 1. define the struct outside the test function and keep the use of arraysize > 2. convert all the other uses of ARRAYSIZE_UNSAFE to arraysize > 3. refactor all the tests to use TEST_P After reading about TEST_P, I think I understand what Adam was actually talking about- by parameterizing the test we don't need to manually loop through the array, so neither arraysize nor ARRAYSIZE_UNSAFE are required. Patchset 2 is an example of what I mean. @Adam: does this look right, before I go off and refactor the other tests?
On 2014/08/13 22:39:26, Mostyn Bramley-Moore wrote: > After reading about TEST_P, I think I understand what Adam was actually talking > about- by parameterizing the test we don't need to manually loop through the > array, so neither arraysize nor ARRAYSIZE_UNSAFE are required. Patchset 2 is an > example of what I mean. > > @Adam: does this look right, before I go off and refactor the other tests? Yes! Sorry I didn't explain myself more clearly. I like this way much better because no logic is required in the test itself.
How does this look?
Very nice cleanup. https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_... net/http/http_response_headers_unittest.cc:117: // normalise whitespace (nit) Please start the comment with a capital letter and end with a full stop. There were some pre-existing comments further down that didn't follow this rule properly. I will leave it up to you whether you want to fix those comments as well. https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_... net/http/http_response_headers_unittest.cc:208: // Normalize bad status header. I think it should be "bad status line", not "bad status header".
https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_... net/http/http_response_headers_unittest.cc:117: // normalise whitespace On 2014/08/15 16:37:06, Adam Rice wrote: > (nit) Please start the comment with a capital letter and end with a full stop. > > There were some pre-existing comments further down that didn't follow this rule > properly. I will leave it up to you whether you want to fix those comments as > well. Fixed this and the preexisting comments. https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_... net/http/http_response_headers_unittest.cc:208: // Normalize bad status header. On 2014/08/15 16:37:06, Adam Rice wrote: > I think it should be "bad status line", not "bad status header". Done.
@rvargas: can you give this OWNERS approval?
lgtm
Sorry I forgot about this. rbstmp lgtm https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:85: class CommonHttpResponseHeadersTest : public HttpResponseHeadersTest, nit: send ": public ..." to the next line and indent the second "public" under the first one https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:329: class PersistenceTest : public HttpResponseHeadersTest, ditto https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:620: class ContentTypeTest : public HttpResponseHeadersTest, ditto https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:782: class RequiresValidationTest : public HttpResponseHeadersTest, etc
https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:85: class CommonHttpResponseHeadersTest : public HttpResponseHeadersTest, On 2014/08/20 19:06:06, rvargas wrote: > nit: send ": public ..." to the next line and indent the second "public" under > the first one Done. https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:329: class PersistenceTest : public HttpResponseHeadersTest, On 2014/08/20 19:06:06, rvargas wrote: > ditto Done. https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:620: class ContentTypeTest : public HttpResponseHeadersTest, On 2014/08/20 19:06:06, rvargas wrote: > ditto Done. https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_... net/http/http_response_headers_unittest.cc:782: class RequiresValidationTest : public HttpResponseHeadersTest, On 2014/08/20 19:06:05, rvargas wrote: > etc Done.
The CQ bit was checked by mostynb@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/448373003/400001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by mostynb@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/448373003/400001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by mostynb@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/448373003/400001
Message was sent while issue was closed.
Committed patchset #22 (400001) as 291025 |
