[sql] Verify that recover works for multiple page sizes.

[sql] Verify that recover works for multiple page sizes.

Verify that sql::Recovery maintains the page size if set_page_size() was
called on the database.  Also verify that sql::Recovery uses the SQLite
default page size if no page size was explicitly set.

BUG=none

Committed: https://crrev.com/0b8d5932c4589d097737a3d3f1a660b61621bbc6
Cr-Commit-Position: refs/heads/master@{#428117}

[Scott Hess - ex-Googler, 2016-10-21 22:00:17]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 22:01:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2016-10-21 22:02:20]

shess@chromium.org changed reviewers:
+ michaeln@chromium.org

[Scott Hess - ex-Googler, 2016-10-21 22:02:21]

Another found git branch, this one initiated by a sqlite-users thread about a
change to the default SQLite page_size.  This was the first problem I thought
of.

[commit-bot: I haz the power, 2016-10-21 23:49:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 23:49:21]

Dry run: This issue passed the CQ dry run.

[michaeln, 2016-10-25 00:12:27]

lgtm

https://codereview.chromium.org/2428383009/diff/1/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/2428383009/diff/1/sql/recovery_unittest.cc#ne...
sql/recovery_unittest.cc:877: // Many clients use 4k pages.  This is the SQLite
default after 3.12.0.
there's some repetition, maybe have a helper 

void TestRecoverWithPageSize(int initial_setting,
                             const string& initial_assertion,
                             int second_setting,
                             const string& second_expectation)

[Scott Hess - ex-Googler, 2016-10-26 23:34:58]

Split out a test helper.

[Scott Hess - ex-Googler, 2016-10-26 23:35:33]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 23:36:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2016-10-26 23:37:38]

https://codereview.chromium.org/2428383009/diff/1/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/2428383009/diff/1/sql/recovery_unittest.cc#ne...
sql/recovery_unittest.cc:877: // Many clients use 4k pages.  This is the SQLite
default after 3.12.0.
On 2016/10/25 00:12:26, michaeln wrote:
> there's some repetition, maybe have a helper

Since I like having it be obvious which test case is breaking, I first went off
and used gtest's TEST_P thingie, and ... I don't know, after awhile it felt like
it just made everything more confusing.  But SCOPED_TRACE() seems to provide a
bit of context to make it reasonable to decode failures, so

PTAL?

Thanks.

[commit-bot: I haz the power, 2016-10-26 23:52:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 23:52:24]

Dry run: This issue passed the CQ dry run.

[michaeln, 2016-10-27 00:02:29]

still lgtm

https://codereview.chromium.org/2428383009/diff/20001/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/2428383009/diff/20001/sql/recovery_unittest.c...
sql/recovery_unittest.cc:898: // The database should have the default page size
after recovery.
SCOPED_TRACE looks like a good tool for this

alternatively you could put the scopers up here
{ SCOPED_TRACE("default");
  TestPageSize(...); }

[Scott Hess - ex-Googler, 2016-10-27 19:12:31]

Switch to EXPECT_NO_FATAL_FAILURE() for context.

[Scott Hess - ex-Googler, 2016-10-27 19:14:58]

Thanks!  I'm modifying the Raze() version of this along the same lines.

https://codereview.chromium.org/2428383009/diff/20001/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/2428383009/diff/20001/sql/recovery_unittest.c...
sql/recovery_unittest.cc:898: // The database should have the default page size
after recovery.
On 2016/10/27 00:02:29, michaeln wrote:
> SCOPED_TRACE looks like a good tool for this
> 
> alternatively you could put the scopers up here
> { SCOPED_TRACE("default");
>   TestPageSize(...); }

Indeed, and that removes the need for convoluted string-creation.

Then I re-crawled the googletest advanced guide and found
EXPECT_NO_FATAL_FAILURE(), which seems to target pretty much the case I want. 
So I'll switch to that.  Since this seems like a minor change, I'll let the bots
decide if it's reasonable from here...

[Scott Hess - ex-Googler, 2016-10-27 19:15:03]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2016-10-27 19:15:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2428383009/#ps40001
(title: "Switch to EXPECT_NO_FATAL_FAILURE() for context.")

[commit-bot: I haz the power, 2016-10-27 19:15:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaeln, 2016-10-27 19:34:26]

On 2016/10/27 19:14:58, Scott Hess wrote:
> Thanks!  I'm modifying the Raze() version of this along the same lines.
> 
> https://codereview.chromium.org/2428383009/diff/20001/sql/recovery_unittest.cc
> File sql/recovery_unittest.cc (right):
> 
>
https://codereview.chromium.org/2428383009/diff/20001/sql/recovery_unittest.c...
> sql/recovery_unittest.cc:898: // The database should have the default page
size
> after recovery.
> On 2016/10/27 00:02:29, michaeln wrote:
> > SCOPED_TRACE looks like a good tool for this
> > 
> > alternatively you could put the scopers up here
> > { SCOPED_TRACE("default");
> >   TestPageSize(...); }
> 
> Indeed, and that removes the need for convoluted string-creation.
> 
> Then I re-crawled the googletest advanced guide and found
> EXPECT_NO_FATAL_FAILURE(), which seems to target pretty much the case I want. 
> So I'll switch to that.  Since this seems like a minor change, I'll let the
bots
> decide if it's reasonable from here...

EXPECT_NO_FATAL_FAILURE, heck yeah!

[commit-bot: I haz the power, 2016-10-27 19:54:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 20:00:20]

Description was changed from

==========
[sql] Verify that recover works for multiple page sizes.

Verify that sql::Recovery maintains the page size if set_page_size() was
called on the database.  Also verify that sql::Recovery uses the SQLite
default page size if no page size was explicitly set.

BUG=none
==========

to

==========
[sql] Verify that recover works for multiple page sizes.

Verify that sql::Recovery maintains the page size if set_page_size() was
called on the database.  Also verify that sql::Recovery uses the SQLite
default page size if no page size was explicitly set.

BUG=none

Committed: https://crrev.com/0b8d5932c4589d097737a3d3f1a660b61621bbc6
Cr-Commit-Position: refs/heads/master@{#428117}
==========

[commit-bot: I haz the power, 2016-10-27 20:00:20]

Patchset 3 (id:??) landed as
https://crrev.com/0b8d5932c4589d097737a3d3f1a660b61621bbc6
Cr-Commit-Position: refs/heads/master@{#428117}