[sql] sql::ScopedErrorIgnorer rename to sql::test::ScopedErrorExpecter

sql/connection.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SQL_CONNECTION_H_
 #define SQL_CONNECTION_H_
 
 #include <stddef.h>
 #include <stdint.h>
 #include <map>
@@ skipping 21 matching lines @@
 
 namespace sql {
 
 class ConnectionMemoryDumpProvider;
 class Recovery;
 class Statement;
 
 // To allow some test classes to be friended.
 namespace test {
 class ScopedCommitHook;
+class ScopedErrorExpecter;
 class ScopedScalarFunction;
 class ScopedMockTimeSource;
 }
 
 // Uniquely identifies a statement. There are two modes of operation:
 //
 // - In the most common mode, you will use the source file and line number to
 //   identify your statement. This is a convienient way to get uniqueness for
 //   a statement that is only used in one place. Use the SQL_FROM_HERE macro
 //   to generate a StatementID.
@@ skipping 415 matching lines @@
 
   // Returns a pointer to a statically allocated string associated with the
   // last sqlite operation.
   const char* GetErrorMessage() const;
 
   // Return a reproducible representation of the schema equivalent to
   // running the following statement at a sqlite3 command-line:
   //   SELECT type, name, tbl_name, sql FROM sqlite_master ORDER BY 1, 2, 3, 4;
   std::string GetSchema() const;
 
-  // Clients which provide an error_callback don't see the
-  // error-handling at the end of OnSqliteError().  Expose to allow
-  // those clients to work appropriately with ScopedErrorIgnorer in
-  // tests.
-  static bool ShouldIgnoreSqliteError(int error);
+  // Clients which provide an error_callback don't see the error-handling at the
+  // end of OnSqliteError().  Expose to allow those clients to work for tests
+  // using ScopedErrorExpecter.  |true| if the error is expected.

[Mark P, 2016/05/23 19:53:27]

I'm having trouble understanding this comment.  I think it's because the current
flow is confusing.  In particular, if you have an error callback, why can't you
simply remember the error and record it in OnSqliteError after dispatching the
callback?  In other words, why do you need to return immediately?

I mainly ask because this current design requires people who supply callbacks to
have this code at the end of their callback.  This is unnecessary and easy to
forget:
 if (!sql::Connection::IsExpectedSqliteError(extended_error))
    DLOG(FATAL) << db->GetErrorMessage();

[Scott Hess - ex-Googler, 2016/06/03 00:02:11]

This is the same concern you had elsewhere about doing the DLOG(FATAL) after the
code attempted to recover the database.  The code can't do the DLOG(FATAL)
before calling the error callback, because then the callback won't get the
chance to fix it.  It can't do the DLOG(FATAL) after calling the error callback,
because the callback may have fixed things.

+  static bool IsExpectedSqliteError(int error);
 
-  // Additionally ignores errors which are unlikely to be caused by problems
-  // with the syntax of a SQL statement, or problems with the database schema.
-  static bool ShouldIgnoreSqliteCompileError(int error);
+  // Return |true| in cases where Extend IsExpectedSqliteError() returns |true|,

[Mark P, 2016/05/23 19:53:27]

Extend?
Is this an errant word?  If so, delete it.  If not, correct / explain it better.

[Scott Hess - ex-Googler, 2016/06/03 00:02:11]

Done.  Errant word.

+  // and also in cases which are likely to be caused by the state of the
+  // database, rather than the statement being compiled.

[Mark P, 2016/05/23 19:53:27]

This comment is confusing.  It's imply that this returns true if
IsExpectedSqliteError() returns true or the dababase is corrupted.  It's almost
saying, unless IsExpectedSqliteError() returns true, if there's a compile error
here, I don't care.  Yet the function name implies that this is only about
compile errors.

[Scott Hess - ex-Googler, 2016/06/03 00:02:11]

I dug back into the history of this a bit.  It looks like there were two issues
which seemed related but which weren't.  One was that a browser test started
hitting a SQLITE_BUSY error (which means someone else is using the database). 
Normally that should never happen, but this is in a somewhat confusing piece of
diagnostics code which I have not fully detangled.  There were additional
SQLITE_CORRUPT/SQLITE_NOTADB (depending) errors in related unit tests, which I
thought stemmed from the same issue, so they were solved in the same place.

The goal of the call was to have a firm error output in case of SQL syntax
errors, even if the module has an error callback (which generally can suppress
errors).  Currently the code tries to say "Oh, these errors can be ignored",
I'll change it to just look for SQLITE_ERROR, which AFAICT is the only error
which matters for this use case.  Some other errors could plausibly flag static
issues to address, perhaps SQLITE_MISUSE or SQLITE_AUTH, but it's hard to craft
a story for them.  I can just let them lie until someone expresses confusion
over a problem they're finding.

+  static bool IsExpectedSqliteCompileError(int error);
 
   // Collect various diagnostic information and post a crash dump to aid
   // debugging.  Dump rate per database is limited to prevent overwhelming the
   // crash server.
   void ReportDiagnosticInfo(int extended_error, Statement* stmt);
 
  private:
   // For recovery module.
   friend class Recovery;
 
-  // Allow test-support code to set/reset error ignorer.
-  friend class ScopedErrorIgnorer;
+  // Allow test-support code to set/reset error expecter.
+  friend class test::ScopedErrorExpecter;
 
   // Statement accesses StatementRef which we don't want to expose to everybody
   // (they should go through Statement).
   friend class Statement;
 
   friend class test::ScopedCommitHook;
   friend class test::ScopedScalarFunction;
   friend class test::ScopedMockTimeSource;
 
   FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, CollectDiagnosticInfo);
@@ skipping 21 matching lines @@
   // if database wasn't open in memory. Function is inlined to be a no-op in
   // official build.
   void AssertIOAllowed() const {
     if (!in_memory_)
       base::ThreadRestrictions::AssertIOAllowed();
   }
 
   // Internal helper for DoesTableExist and DoesIndexExist.
   bool DoesTableOrIndexExist(const char* name, const char* type) const;
 
-  // Accessors for global error-ignorer, for injecting behavior during tests.
-  // See test/scoped_error_ignorer.h.
-  typedef base::Callback<bool(int)> ErrorIgnorerCallback;
-  static ErrorIgnorerCallback* current_ignorer_cb_;
-  static void SetErrorIgnorer(ErrorIgnorerCallback* ignorer);
-  static void ResetErrorIgnorer();
+  // Accessors for global error-expecter, for injecting behavior during tests.
+  // See test/scoped_error_expecter.h.
+  typedef base::Callback<bool(int)> ErrorExpecterCallback;
+  static ErrorExpecterCallback* current_expecter_cb_;
+  static void SetErrorExpecter(ErrorExpecterCallback* expecter);
+  static void ResetErrorExpecter();
 
   // A StatementRef is a refcounted wrapper around a sqlite statement pointer.
   // Refcounting allows us to give these statements out to sql::Statement
   // objects while also optionally maintaining a cache of compiled statements
   // by just keeping a refptr to these objects.
   //
   // A statement ref can be valid, in which case it can be used, or invalid to
   // indicate that the statement hasn't been created yet, has an error, or has
   // been destroyed.
   //
@@ skipping 238 matching lines @@
 
   // Stores the dump provider object when db is open.
   std::unique_ptr<ConnectionMemoryDumpProvider> memory_dump_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(Connection);
 };
 
 }  // namespace sql
 
 #endif  // SQL_CONNECTION_H_