[sql] Verify control of close-time WAL checkpoint.

sql/sqlite_features_unittest.cc

Index: sql/sqlite_features_unittest.cc
diff --git a/sql/sqlite_features_unittest.cc b/sql/sqlite_features_unittest.cc
index 0f124af3c50a6d5ad8a7bff9f9bdd89150ee87a1..88f7802fc02d5a56fb98a5b3dab7f3268e7220c6 100644
--- a/sql/sqlite_features_unittest.cc
+++ b/sql/sqlite_features_unittest.cc
@@ -32,8 +32,12 @@
 
 // Test that certain features are/are-not enabled in our SQLite.
 
+namespace sql {
 namespace {
 
+using sql::test::ExecuteWithResult;
+using sql::test::ExecuteWithResults;
+
 void CaptureErrorCallback(int* error_pointer, std::string* sql_text,
                           int error, sql::Statement* stmt) {
   *error_pointer = error;
@@ -41,6 +45,8 @@ void CaptureErrorCallback(int* error_pointer, std::string* sql_text,
   *sql_text = text ? text : "no statement available";
 }
 
+}  // namespace
+
 class SQLiteFeaturesTest : public sql::SQLTestBase {
  public:
   SQLiteFeaturesTest() : error_(SQLITE_OK) {}
@@ -102,10 +108,8 @@ TEST_F(SQLiteFeaturesTest, FTS3_Prefix) {
 
   ASSERT_TRUE(db().Execute("INSERT INTO foo (x) VALUES ('test')"));
 
-  sql::Statement s(db().GetUniqueStatement(
-      "SELECT x FROM foo WHERE x MATCH 'te*'"));
-  ASSERT_TRUE(s.Step());
-  EXPECT_EQ("test", s.ColumnString(0));
+  EXPECT_EQ("test",
+            ExecuteWithResult(&db(), "SELECT x FROM foo WHERE x MATCH 'te*'"));
 }
 #endif
 
@@ -118,10 +122,9 @@ TEST_F(SQLiteFeaturesTest, UsesUsleep) {
   sqlite3_sleep(1);
   base::TimeDelta delta = base::TimeTicks::Now() - before;
 
-  // It is not impossible for this to be over 1000 if things are compiled the
-  // right way.  But it is very unlikely, most platforms seem to be around
-  // <TBD>.
-  LOG(ERROR) << "Milliseconds: " << delta.InMilliseconds();
+  // It is not impossible for this to be over 1000 if things are compiled
+  // correctly, but that is very unlikely.  Most platforms seem to be exactly
+  // 1ms, with the rest at 2ms, and the worst observed cases was ASAN at 7ms.
   EXPECT_LT(delta.InMilliseconds(), 1000);
 }
 #endif
@@ -135,28 +138,29 @@ TEST_F(SQLiteFeaturesTest, ForeignKeySupport) {
       "CREATE TABLE children ("
       "    id INTEGER PRIMARY KEY,"
       "    pid INTEGER NOT NULL REFERENCES parents(id) ON DELETE CASCADE)"));
+  const char kSelectParents[] = "SELECT * FROM parents ORDER BY id";
+  const char kSelectChildren[] = "SELECT * FROM children ORDER BY id";
 
   // Inserting without a matching parent should fail with constraint violation.
   // Mask off any extended error codes for USE_SYSTEM_SQLITE.
-  int insertErr = db().ExecuteAndReturnErrorCode(
-      "INSERT INTO children VALUES (10, 1)");
-  EXPECT_EQ(SQLITE_CONSTRAINT, (insertErr&0xff));
-
-  size_t rows;
-  EXPECT_TRUE(sql::test::CountTableRows(&db(), "children", &rows));
-  EXPECT_EQ(0u, rows);
+  EXPECT_EQ("", ExecuteWithResult(&db(), kSelectParents));
+  const int insert_error =
+      db().ExecuteAndReturnErrorCode("INSERT INTO children VALUES (10, 1)");
+  EXPECT_EQ(SQLITE_CONSTRAINT, (insert_error & 0xff));
+  EXPECT_EQ("", ExecuteWithResult(&db(), kSelectChildren));
 
   // Inserting with a matching parent should work.
   ASSERT_TRUE(db().Execute("INSERT INTO parents VALUES (1)"));
+  EXPECT_EQ("1", ExecuteWithResults(&db(), kSelectParents, "|", "\n"));
   EXPECT_TRUE(db().Execute("INSERT INTO children VALUES (11, 1)"));
   EXPECT_TRUE(db().Execute("INSERT INTO children VALUES (12, 1)"));
-  EXPECT_TRUE(sql::test::CountTableRows(&db(), "children", &rows));
-  EXPECT_EQ(2u, rows);
+  EXPECT_EQ("11|1\n12|1",
+            ExecuteWithResults(&db(), kSelectChildren, "|", "\n"));
 
-  // Deleting the parent should cascade, i.e., delete the children as well.
+  // Deleting the parent should cascade, deleting the children as well.
   ASSERT_TRUE(db().Execute("DELETE FROM parents"));
-  EXPECT_TRUE(sql::test::CountTableRows(&db(), "children", &rows));
-  EXPECT_EQ(0u, rows);
+  EXPECT_EQ("", ExecuteWithResult(&db(), kSelectParents));
+  EXPECT_EQ("", ExecuteWithResult(&db(), kSelectChildren));
 }
 
 #if defined(MOJO_APPTEST_IMPL) || defined(OS_IOS)
@@ -453,4 +457,36 @@ TEST_F(SQLiteFeaturesTest, SmartAutoVacuum) {
 }
 #endif  // !defined(USE_SYSTEM_SQLITE)
 
-}  // namespace
+#if !defined(USE_SYSTEM_SQLITE)

[pwnall, 2017/04/19 20:02:23]

Would tests fail anywhere if we removed this #if?

I'd imagine we're OK with tests failing on people's custom builds if their
system libraries don't implement everything we need.

[Scott Hess - ex-Googler, 2017/04/19 20:15:33]

iOS uses the system SQLite.  Also maybe Linux distros bundling Chromium, but I'm
not sure that works any longer.

Perhaps a different question is whether this feature is relevant for iOS, given
that we generally don't actually shutdown in a way that makes this relevant. 
But here I'm trying to make assertions about features for Chromium proper to
depend on, so it's not in the regular shutdown context, so that may also not
really be relevant.

[pwnall, 2017/04/19 20:56:54]

TL;DR: If the test passes on bots, I'd leave it on.

Our tests don't have to pass on Linux distros that replace our libraries with
arbitrary versions. That was the case I was thinking of. In that case, I claim
that it's better to have the test fail, so the distributor knows all bets are
off.

Is the system SQLite on our iOS bots old enough / built with a config that
doesn't support the feature? If newer iOS has WAL, I'd leave the test in. If
not, I'd be tempted to have the test skipped for iOS, with a comment explaining
the situation.

[Scott Hess - ex-Googler, 2017/04/19 22:03:16]

The SQLite change landed last fall, so it's pretty new.  iOS has pretty good
uptake when new OS versions come out, but I expect that older versions of iOS
which are still supported by Chrome will not bump their SQLite version.

I haven't done the research to figure out the exact SQLite version the change
landed in, but based on
https://github.com/yapstudios/YapDatabase/wiki/SQLite-version-(bundled-with-OS)
I suspect no iOS versions of SQLite with the change being tested.

+// SQLite WAL mode defaults to checkpointing the WAL on close.  This would push
+// additional work into Chromium shutdown.  Verify that SQLite supports a config
+// option to not checkpoint on close.
+TEST_F(SQLiteFeaturesTest, WALNoClose) {

[Scott Hess - ex-Googler, 2017/04/18 23:53:39]

Um, this is the main test for whether the desired feature is present.  The other
changes above are mostly "Now that I've imported the ExecuteWithResult() helper,
what else would I change?"

+  base::FilePath wal_path(db_path().value() + FILE_PATH_LITERAL("-wal"));
+
+  // Turn on WAL mode, then verify that the mode changed (WAL is supported).
+  ASSERT_TRUE(db().Execute("PRAGMA journal_mode = WAL"));
+  ASSERT_EQ("wal", ExecuteWithResult(&db(), "PRAGMA journal_mode"));
+
+  // The WAL file is created lazily on first change.
+  ASSERT_TRUE(db().Execute("CREATE TABLE foo (a, b)"));
+
+  // By default, the WAL is checkpointed then deleted on close.
+  ASSERT_TRUE(GetPathExists(wal_path));
+  db().Close();
+  ASSERT_FALSE(GetPathExists(wal_path));
+
+  // Reopen and configure the database to not checkpoint WAL on close.
+  ASSERT_TRUE(Reopen());
+  ASSERT_TRUE(db().Execute("PRAGMA journal_mode = WAL"));
+  ASSERT_TRUE(db().Execute("ALTER TABLE foo ADD COLUMN c"));
+  ASSERT_EQ(
+      SQLITE_OK,
+      sqlite3_db_config(db().db_, SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE, 1, NULL));
+  ASSERT_TRUE(GetPathExists(wal_path));
+  db().Close();
+  ASSERT_TRUE(GetPathExists(wal_path));
+}
+#endif
+
+}  // namespace sql