Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(18)

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
Project:
chromium
Visibility:
Public.

Description

parameterize 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1654 lines, -1485 lines) Patch
M net/http/http_response_headers_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 chunks +1654 lines, -1485 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
Mostyn Bramley-Moore
@Adam, tyoshino: does this small improvement look OK?
6 years, 4 months ago (2014-08-11 12:13:05 UTC) #1
Adam Rice
I was hoping since it passed all the bot tests we no longer needed to ...
6 years, 4 months ago (2014-08-11 12:17:54 UTC) #2
Mostyn Bramley-Moore
https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc#newcode2034 net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { On 2014/08/11 12:17:54, Adam ...
6 years, 4 months ago (2014-08-11 12:24:02 UTC) #3
Adam Rice
https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc#newcode2034 net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { On 2014/08/11 12:24:02, Mostyn ...
6 years, 4 months ago (2014-08-11 12:28:46 UTC) #4
Mostyn Bramley-Moore
https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc#newcode2034 net/http/http_response_headers_unittest.cc:2034: const MaxAgeTestData tests[] = { On 2014/08/11 12:28:46, Adam ...
6 years, 4 months ago (2014-08-11 12:35:36 UTC) #5
tyoshino (SeeGerritForStatus)
On 2014/08/11 12:35:36, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc > File net/http/http_response_headers_unittest.cc (right): > > https://codereview.chromium.org/448373003/diff/1/net/http/http_response_headers_unittest.cc#newcode2034 ...
6 years, 4 months ago (2014-08-12 18:02:53 UTC) #6
Mostyn Bramley-Moore
On 2014/08/12 18:02:53, tyoshino wrote: > Sorry I'm not catching up. What is Adam's suggestion? ...
6 years, 4 months ago (2014-08-12 18:55:36 UTC) #7
Mostyn Bramley-Moore
On 2014/08/12 18:55:36, Mostyn Bramley-Moore wrote: > Adam's suggestion was to either change arraysize to ...
6 years, 4 months ago (2014-08-13 22:39:26 UTC) #8
Adam Rice
On 2014/08/13 22:39:26, Mostyn Bramley-Moore wrote: > After reading about TEST_P, I think I understand ...
6 years, 4 months ago (2014-08-14 01:42:26 UTC) #9
Mostyn Bramley-Moore
How does this look?
6 years, 4 months ago (2014-08-14 14:39:17 UTC) #10
Adam Rice
Very nice cleanup. https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_headers_unittest.cc#newcode117 net/http/http_response_headers_unittest.cc:117: // normalise whitespace (nit) Please start ...
6 years, 4 months ago (2014-08-15 16:37:07 UTC) #11
Mostyn Bramley-Moore
https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/360001/net/http/http_response_headers_unittest.cc#newcode117 net/http/http_response_headers_unittest.cc:117: // normalise whitespace On 2014/08/15 16:37:06, Adam Rice wrote: ...
6 years, 4 months ago (2014-08-15 18:14:53 UTC) #12
Mostyn Bramley-Moore
@rvargas: can you give this OWNERS approval?
6 years, 4 months ago (2014-08-15 18:33:47 UTC) #13
Adam Rice
lgtm
6 years, 4 months ago (2014-08-20 05:17:28 UTC) #14
rvargas (doing something else)
Sorry I forgot about this. rbstmp lgtm https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_headers_unittest.cc#newcode85 net/http/http_response_headers_unittest.cc:85: class CommonHttpResponseHeadersTest ...
6 years, 4 months ago (2014-08-20 19:06:06 UTC) #15
Mostyn Bramley-Moore
https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/448373003/diff/380001/net/http/http_response_headers_unittest.cc#newcode85 net/http/http_response_headers_unittest.cc:85: class CommonHttpResponseHeadersTest : public HttpResponseHeadersTest, On 2014/08/20 19:06:06, rvargas ...
6 years, 4 months ago (2014-08-20 19:36:23 UTC) #16
Mostyn Bramley-Moore
The CQ bit was checked by mostynb@opera.com
6 years, 4 months ago (2014-08-20 19:36:29 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/448373003/400001
6 years, 4 months ago (2014-08-20 19:37:30 UTC) #18
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu ...
6 years, 4 months ago (2014-08-20 21:14:16 UTC) #19
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-20 21:18:23 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/55097) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg/builds/7906)
6 years, 4 months ago (2014-08-20 21:18:24 UTC) #21
Mostyn Bramley-Moore
The CQ bit was checked by mostynb@opera.com
6 years, 4 months ago (2014-08-20 21:23:42 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/448373003/400001
6 years, 4 months ago (2014-08-20 21:25:47 UTC) #23
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu ...
6 years, 4 months ago (2014-08-20 21:31:56 UTC) #24
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-20 21:36:48 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/55129)
6 years, 4 months ago (2014-08-20 21:36:50 UTC) #26
Mostyn Bramley-Moore
The CQ bit was checked by mostynb@opera.com
6 years, 4 months ago (2014-08-21 07:30:07 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/448373003/400001
6 years, 4 months ago (2014-08-21 07:31:06 UTC) #28
commit-bot: I haz the power
6 years, 4 months ago (2014-08-21 08:52:05 UTC) #29
Message was sent while issue was closed.
Committed patchset #22 (400001) as 291025

Powered by Google App Engine
This is Rietveld 408576698