Add asserts to TestingProfile::CreateHistoryService to ensure files are deleted

Add asserts to TestingProfile::CreateHistoryService to ensure files are deleted

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213148

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213413

[rmcilroy, 2013-07-19 17:49:29]

The context on wanting to assert these files are deleted is that
http://crrev.com/212459 got reverted because some unittests failed (which didn't
fail on the trybots).  I want to make sure this file is being deleted to see if
this is what caused the flaky test failure.

[sky, 2013-07-19 20:04:57]

https://codereview.chromium.org/19616004/diff/1/chrome/test/base/testing_prof...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/19616004/diff/1/chrome/test/base/testing_prof...
chrome/test/base/testing_profile.cc:362: ASSERT_TRUE(base::DeleteFile(path,
false));
In general you shouldn't use ASSERTS in tests. It makes the whole harness crash
and no further tests are run.

[rmcilroy, 2013-07-22 11:18:07]

Changed ASSERTS to EXPECTS.  PTAL

https://codereview.chromium.org/19616004/diff/1/chrome/test/base/testing_prof...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/19616004/diff/1/chrome/test/base/testing_prof...
chrome/test/base/testing_profile.cc:362: ASSERT_TRUE(base::DeleteFile(path,
false));
On 2013/07/19 20:04:57, sky wrote:
> In general you shouldn't use ASSERTS in tests. It makes the whole harness
crash
> and no further tests are run.

Changed these to EXPECTS (I thought ASSERTS were allowed in setup-type code, but
am fine with these being EXPECTS in either case).

[sky, 2013-07-22 14:53:52]

Expects won't stop test flow. You should change the return type.

[rmcilroy_google, 2013-07-22 15:47:52]

On 2013/07/22 14:53:52, sky wrote:
> Expects won't stop test flow. You should change the return type.

Done.  I'm back to using ASSERT_TRUE in the caller, because I did a test locally
and it seems that ASSERT_XX will only abort the current test (not crash the test
harness). Is this different in the bot infrastructure?  If not I think this is
what we actually want - abort the current test if the files aren't deleted
correctly during the setup phase (Note, there is an
ASSERT_TRUE(history_service_) a few lines on in InMemoryURLIndexTest::SetUp()
which seems to be a similar use-case).

[sky, 2013-07-22 18:26:27]

Actually, ASSERT just returns from current caller. The problem with putting an
ASSERT in CreateHistoryService means all call sites continue on as if nothing
bad happened.

Moving the ASSERT out to test bodies/call sites is the right way. Hence, the
checking return value.

https://codereview.chromium.org/19616004/diff/16001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/19616004/diff/16001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:136: bool CreateHistoryService(bool
delete_file, bool no_db);
Can you add WARN_UNUSED_RESULT?

[rmcilroy, 2013-07-22 22:44:17]

https://codereview.chromium.org/19616004/diff/16001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/19616004/diff/16001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:136: bool CreateHistoryService(bool
delete_file, bool no_db);
On 2013/07/22 18:26:27, sky wrote:
> Can you add WARN_UNUSED_RESULT?

Done, and fixed all callers to assert on this being true.

[sky, 2013-07-22 23:02:23]

https://codereview.chromium.org/19616004/diff/22001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

https://codereview.chromium.org/19616004/diff/22001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_model_unittest.cc:919:
ASSERT_TRUE(profile_->CreateHistoryService(true, false));
You need to wrap all calls to RecreateProfile() in ASSERT_NO_FATAL_FAILURE (or
promote the return value to the function itself and ASSERT there (assuming all
callers are SetUp/test-body, otherwise you'll have to continue propagating
return)). You'll want to do the same for all other places that aren't in
SetUp/test-body.

[rmcilroy, 2013-07-22 23:53:31]

https://codereview.chromium.org/19616004/diff/22001/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

https://codereview.chromium.org/19616004/diff/22001/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_model_unittest.cc:919:
ASSERT_TRUE(profile_->CreateHistoryService(true, false));
On 2013/07/22 23:02:23, sky wrote:
> You need to wrap all calls to RecreateProfile() in ASSERT_NO_FATAL_FAILURE (or
> promote the return value to the function itself and ASSERT there (assuming all
> callers are SetUp/test-body, otherwise you'll have to continue propagating
> return)). You'll want to do the same for all other places that aren't in
> SetUp/test-body.

This method was never actually used, so I removed it.  Done elsewhere
(history_url_provider_unittest.cc). Apologies for missing this first time round.

[sky, 2013-07-23 13:59:37]

LGTM

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:338:
RemoveHistoryTester() : query_url_success_(false) {}
nit: member initialize history_service_ to NULL.

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:340: bool
Initialize(TestingProfile* profile) WARN_UNUSED_RESULT {
nit: we generally call this Init.

https://codereview.chromium.org/19616004/diff/31001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/19616004/diff/31001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:362: if (!base::DeleteFile(path, false) ||
nit: seems like you could fit this on one line and nuke the {}

[rmcilroy, 2013-07-23 14:30:50]

Thanks for the quick reviews!  I learned a fair bit about how gtest actually
works :).

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:338:
RemoveHistoryTester() : query_url_success_(false) {}
On 2013/07/23 13:59:37, sky wrote:
> nit: member initialize history_service_ to NULL.

Done.

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:340: bool
Initialize(TestingProfile* profile) WARN_UNUSED_RESULT {
On 2013/07/23 13:59:37, sky wrote:
> nit: we generally call this Init.

I had Init before, but have been hit by the "don't abbreviate" rule before so
changed it :).

https://codereview.chromium.org/19616004/diff/31001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/19616004/diff/31001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:362: if (!base::DeleteFile(path, false) ||
On 2013/07/23 13:59:37, sky wrote:
> nit: seems like you could fit this on one line and nuke the {}

Done.

[commit-bot: I haz the power, 2013-07-23 14:33:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/19616004/37001

[sky, 2013-07-23 14:36:09]

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/19616004/diff/31001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:340: bool
Initialize(TestingProfile* profile) WARN_UNUSED_RESULT {
On 2013/07/23 14:30:50, rmcilroy wrote:
> On 2013/07/23 13:59:37, sky wrote:
> > nit: we generally call this Init.
> 
> I had Init before, but have been hit by the "don't abbreviate" rule before so
> changed it :).

Some times the style guide is at odds with itself:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_...
if you expand that you'll see:
"If your object requires non-trivial initialization, consider using a factory
function or Init() method."

[commit-bot: I haz the power, 2013-07-23 17:19:53]

Change committed as 213148

[commit-bot: I haz the power, 2013-07-24 09:22:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/19616004/57001

[commit-bot: I haz the power, 2013-07-24 12:23:34]

Change committed as 213413