Refactor this test to reduce code duplication.

Refactor this test to reduce code duplication.

This was done as a precursor to implementing a TDD test for the bug 111376. I'm noting the bug number here in case the follow-up revision is merged to M17 or M18 (in which case this refactoring might be required to make the tests compile).


BUG=111376
TEST=unit_tests --gtest_filter=SQLitePersistentCookieStoreTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124650

[erikwright (departed), 2012-03-01 21:05:55]

PTAL.

See also http://codereview.chromium.org/9567022/ .

[Randy Smith (Not in Mondays), 2012-03-01 22:12:10]

LGTM.  

(Just FYI: I'll be OOO/Away from Email Friday & Monday if there are followups.)

http://codereview.chromium.org/9569028/diff/1/chrome/browser/net/sqlite_persi...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

http://codereview.chromium.org/9569028/diff/1/chrome/browser/net/sqlite_persi...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:55: // Make sure
we wait until the destructor has run.
nit, suggestion: It's not obvious from the name what using ThreadTestHelper
without inheritance does, so you might want to add "by waiting for all pending
tasks on the DB thread to run."

http://codereview.chromium.org/9569028/diff/1/chrome/browser/net/sqlite_persi...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:117:
AddCookie("A", "B", "http://foo.bar", "/", base::Time::Now());
Hmmm.  This is only a suggestion (up to you) but the purpose of this line isn't
obvious.  I presume it's to replace the "make sure the store gets written at
least once" AddCookie that used to be in SetUp(), but the code would be clearer
if that was in a helper function, maybe one named something like TouchStore() or
DirtyStore()?

[erikwright (departed), 2012-03-02 03:20:31]

http://codereview.chromium.org/9569028/diff/1/chrome/browser/net/sqlite_persi...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

http://codereview.chromium.org/9569028/diff/1/chrome/browser/net/sqlite_persi...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:55: // Make sure
we wait until the destructor has run.
On 2012/03/01 22:12:10, rdsmith wrote:
> nit, suggestion: It's not obvious from the name what using ThreadTestHelper
> without inheritance does, so you might want to add "by waiting for all pending
> tasks on the DB thread to run."

Done.

http://codereview.chromium.org/9569028/diff/1/chrome/browser/net/sqlite_persi...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:117:
AddCookie("A", "B", "http://foo.bar", "/", base::Time::Now());
On 2012/03/01 22:12:10, rdsmith wrote:
> Hmmm.  This is only a suggestion (up to you) but the purpose of this line
isn't
> obvious.  I presume it's to replace the "make sure the store gets written at
> least once" AddCookie that used to be in SetUp(), but the code would be
clearer
> if that was in a helper function, maybe one named something like TouchStore()
or
> DirtyStore()?

Done.

[commit-bot: I haz the power, 2012-03-02 03:22:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/9569028/5

[commit-bot: I haz the power, 2012-03-02 04:42:14]

Try job failure for 9569028-5 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-02 12:00:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/9569028/5

[commit-bot: I haz the power, 2012-03-02 16:00:01]

Change committed as 124650