Add SQLite feature test to verify working foreign key support.

Add SQLite feature test to verify working foreign key support.

This low-level test specifically ensures that foreign key support will not break upon importing a new SQLite version or changing compile-time flags.

BUG=437865
Committed: https://crrev.com/be80d53e9b6b4508eeaf0c43d86964b8d6dee1a7
Cr-Commit-Position: refs/heads/master@{#312596}

[engedy, 2015-01-21 10:53:27]

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

[engedy, 2015-01-21 10:53:28]

@Scott, I have added the test you have requested in
https://codereview.chromium.org/797983003/.

Could you please take a look?

[Scott Hess - ex-Googler, 2015-01-21 18:17:02]

lgtm with a minor suggestion.

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest.cc
File sql/sqlite_features_unittest.cc (right):

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest...
sql/sqlite_features_unittest.cc:143: EXPECT_TRUE(s.Succeeded());
This may be prone to false positives, since there are many ways to generate an
empty result.  Maybe before the delete, check that this SELECT statement
actually returned elements.

[engedy, 2015-01-21 19:50:22]

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest.cc
File sql/sqlite_features_unittest.cc (right):

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest...
sql/sqlite_features_unittest.cc:143: EXPECT_TRUE(s.Succeeded());
On 2015/01/21 18:17:02, Scott Hess wrote:
> This may be prone to false positives, since there are many ways to generate an
> empty result.  Maybe before the delete, check that this SELECT statement
> actually returned elements.

Do you think doing it with SELECT COUNT(*) would be better?

[Scott Hess - ex-Googler, 2015-01-21 20:23:31]

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest.cc
File sql/sqlite_features_unittest.cc (right):

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest...
sql/sqlite_features_unittest.cc:143: EXPECT_TRUE(s.Succeeded());
On 2015/01/21 19:50:22, engedy wrote:
> On 2015/01/21 18:17:02, Scott Hess wrote:
> > This may be prone to false positives, since there are many ways to generate
an
> > empty result.  Maybe before the delete, check that this SELECT statement
> > actually returned elements.
> 
> Do you think doing it with SELECT COUNT(*) would be better?

Not in-place, because that still wouldn't demonstrate the transition between
having elements and not having elements.  What I mean is that while above you
tested that a particular insert which shouldn't work doesn't, you don't check
that an insert which should work does.  Admittedly, we don't generally test for
such things in here, but we have lots of databases without foreign_keys enabled,
so I'm pretty sure some of the test coverage would throw something up.  Mostly
I'm worried about foreign_keys being subtle, could just be nerves.

You could do something like:

EXPECT_TRUE(sql::test::CountTableRows(db(), "children", &rows));
EXPECT_GT(rows, 0);
// execute delete
EXPECT_TRUE(sql::test::CountTableRows(db(), "children", &rows));
EXPECT_EQ(rows, 0);

I thought I had a test helper for "query everything in a table and format it a
certain way", but I don't see it at this time.  Maybe it's in a particular unit
test instead of in the sql/test/test_helpers.h file.  Maybe I should promote
it...

[engedy, 2015-01-22 09:11:07]

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest.cc
File sql/sqlite_features_unittest.cc (right):

https://codereview.chromium.org/856343002/diff/1/sql/sqlite_features_unittest...
sql/sqlite_features_unittest.cc:143: EXPECT_TRUE(s.Succeeded());
On 2015/01/21 20:23:30, Scott Hess wrote:
> On 2015/01/21 19:50:22, engedy wrote:
> > On 2015/01/21 18:17:02, Scott Hess wrote:
> > > This may be prone to false positives, since there are many ways to
generate
> an
> > > empty result.  Maybe before the delete, check that this SELECT statement
> > > actually returned elements.
> > 
> > Do you think doing it with SELECT COUNT(*) would be better?
> 
> Not in-place, because that still wouldn't demonstrate the transition between
> having elements and not having elements.  What I mean is that while above you
> tested that a particular insert which shouldn't work doesn't, you don't check
> that an insert which should work does.  Admittedly, we don't generally test
for
> such things in here, but we have lots of databases without foreign_keys
enabled,
> so I'm pretty sure some of the test coverage would throw something up.  Mostly
> I'm worried about foreign_keys being subtle, could just be nerves.
> 
> You could do something like:
> 
> EXPECT_TRUE(sql::test::CountTableRows(db(), "children", &rows));
> EXPECT_GT(rows, 0);
> // execute delete
> EXPECT_TRUE(sql::test::CountTableRows(db(), "children", &rows));
> EXPECT_EQ(rows, 0);
> 
> I thought I had a test helper for "query everything in a table and format it a
> certain way", but I don't see it at this time.  Maybe it's in a particular
unit
> test instead of in the sql/test/test_helpers.h file.  Maybe I should promote
> it...

Done.

[engedy, 2015-01-22 09:11:10]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-01-22 09:11:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856343002/20001

[commit-bot: I haz the power, 2015-01-22 09:55:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-22 09:58:13]

Patchset 2 (id:??) landed as
https://crrev.com/be80d53e9b6b4508eeaf0c43d86964b8d6dee1a7
Cr-Commit-Position: refs/heads/master@{#312596}