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

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org

Committed: https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4
Cr-Commit-Position: refs/heads/master@{#400910}

[Scott Hess - ex-Googler, 2016-05-17 23:29:44]

Bit of cleanup in scoped_error_expecter

[Scott Hess - ex-Googler, 2016-05-17 23:31:09]

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

[Scott Hess - ex-Googler, 2016-05-17 23:31:10]

WDYT of a change like this to clarify how the class is used?  It's more than you
probably asked for, but the ambiguity has been bothering me.

[Mark P, 2016-05-18 17:41:22]

On 2016/05/17 23:31:10, Scott Hess wrote:
> WDYT of a change like this to clarify how the class is used?  It's more than
you
> probably asked for, but the ambiguity has been bothering me.

I'll take a look.  Is it possible to trick this code review tool and git into
realizing that you renamed _ignorer to _expecter and then edited so I can get a
much better diff and we can preserve history over the move?

--mark

[Scott Hess - ex-Googler, 2016-05-21 03:53:44]

On 2016/05/18 17:41:22, Mark P wrote:
> On 2016/05/17 23:31:10, Scott Hess wrote:
> > WDYT of a change like this to clarify how the class is used?  It's more than
> you
> > probably asked for, but the ambiguity has been bothering me.
> 
> I'll take a look.  Is it possible to trick this code review tool and git into
> realizing that you renamed _ignorer to _expecter and then edited so I can get
a
> much better diff and we can preserve history over the move?

Hmm, usually I'd expect it to have noticed how similar they are and give you a
diff.  There must be too many changes.  If I get a minute, I'll rephrase it as
two patches, "Changes from original" and "Rename".

[Scott Hess - ex-Googler, 2016-05-23 14:59:20]

Back out file rename

[Scott Hess - ex-Googler, 2016-05-23 15:01:26]

Restore file rename

[Scott Hess - ex-Googler, 2016-05-23 15:03:48]

On 2016/05/21 03:53:44, Scott Hess wrote:
> On 2016/05/18 17:41:22, Mark P wrote:
> > On 2016/05/17 23:31:10, Scott Hess wrote:
> > > WDYT of a change like this to clarify how the class is used?  It's more
than
> > you
> > > probably asked for, but the ambiguity has been bothering me.
> > 
> > I'll take a look.  Is it possible to trick this code review tool and git
into
> > realizing that you renamed _ignorer to _expecter and then edited so I can
get
> a
> > much better diff and we can preserve history over the move?
> 
> Hmm, usually I'd expect it to have noticed how similar they are and give you a
> diff.  There must be too many changes.  If I get a minute, I'll rephrase it as
> two patches, "Changes from original" and "Rename".

OK, PS#3 reverts the file rename, PS#4 puts it back.  You can't use the "Delta
from patch set" column to see the scoped_error_ignorer diffs, you have to open
Patch Set 3 and click "View" in the "Side-by-side diffs" column.

[Mark P, 2016-05-23 19:53:28]

Thanks for patchset #3.  That made this change much easier to review.

I like how you've renamed this class.  It makes the test functions that use it
read a lot more intuitively.

My comments below are mostly criticisms of the comments in new header file, and
also one comment about code flow.

--mark

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h#newcod...
sql/connection.h:480: // using ScopedErrorExpecter.  |true| if the error is
expected.
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();

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h#newcod...
sql/connection.h:483: // Return |true| in cases where Extend
IsExpectedSqliteError() returns |true|,
Extend?
Is this an errant word?  If so, delete it.  If not, correct / explain it better.

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h#newcod...
sql/connection.h:485: // database, rather than the statement being compiled.
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.

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.cc (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.cc:59: return false;
You changed this from false to true.  I assume it was intentional and there is
no other code that needs to be updated... ? (as no code should actually reach
here instead of failing above)

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.h (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.h:52: bool ErrorSeen(int err);
Please explain what the return value means.

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

I'm going to break off the code to remove IsExpectedSqliteCompileError(),
because that was added in response to a waterfall breakage.  So while I've found
the cases that broke at that time, my guess is that the odds are about even as
to whether there are more cases which trybots won't catch but the waterfall will
(due to differences in debug/release flags used on each).

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h#newcod...
sql/connection.h:480: // using ScopedErrorExpecter.  |true| if the error is
expected.
On 2016/05/23 19:53:27, Mark P wrote:
> 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();

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.

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h#newcod...
sql/connection.h:483: // Return |true| in cases where Extend
IsExpectedSqliteError() returns |true|,
On 2016/05/23 19:53:27, Mark P wrote:
> Extend?
> Is this an errant word?  If so, delete it.  If not, correct / explain it
better.

Done.  Errant word.

https://codereview.chromium.org/1991503002/diff/60001/sql/connection.h#newcod...
sql/connection.h:485: // database, rather than the statement being compiled.
On 2016/05/23 19:53:27, Mark P wrote:
> 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.

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.

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.cc (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.cc:59: return false;
On 2016/05/23 19:53:27, Mark P wrote:
> You changed this from false to true.  I assume it was intentional and there is
> no other code that needs to be updated... ? (as no code should actually reach
> here instead of failing above)

Previously all cases returned true, which meant that unexpected errors were
collected by ADD_FAILURE() and reported at the final "the right errors were
seen" expectation.  But that just indicated that new errors were happening, it
didn't tell where they were.  Returning false means the calling code will call
the DLOG(FATAL) case, which will provide a useful backtrace.  I think that's
what I originally intended, based on how Connection::ShouldIgnoreError() was
written.

ADD_FAILURE() would be preferred, DLOG(FATAL) is generally not preferred for
tests, but AFAICT ADD_FAILURE() can't provide backtrace info.  I'm trying to
think of a way to square the circle, but can't.  Since this case can only happen
if someone is not running tests on their local machine, I'm inclined to just go
with the useful crash, so that what to fix it obvious.

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.h (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.h:52: bool ErrorSeen(int err);
On 2016/05/23 19:53:27, Mark P wrote:
> Please explain what the return value means.

Done.

[Mark P, 2016-06-08 22:13:37]

I know you split off some of this into the other changelist that I just
reviewed.  But it looks to me here like you also intended to upload a new
patchset.  e.g.,
"On 2016/05/23 19:53:27, Mark P wrote:
> Please explain what the return value means.

Done.
"
yet there is no new patchset after those comments.

Let me know when you want me to look at this again.

[Scott Hess - ex-Googler, 2016-06-14 22:24:22]

rebase

[Scott Hess - ex-Googler, 2016-06-14 22:27:06]

On 2016/06/14 22:24:22, Scott Hess wrote:
> rebase

[Scott Hess - ex-Googler, 2016-06-14 22:27:41]

On 2016/06/14 22:24:22, Scott Hess wrote:
> rebase

AFAICT, the rebase only pulled in expected changes from trunk.

[Scott Hess - ex-Googler, 2016-06-14 22:29:24]

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

[commit-bot: I haz the power, 2016-06-14 22:29:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991503002/80001

[commit-bot: I haz the power, 2016-06-15 00:31:00]

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

[commit-bot: I haz the power, 2016-06-15 00:31:01]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Mark P, 2016-06-15 04:39:44]

lgtm with two quibbles below

--mark

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.cc (right):

https://codereview.chromium.org/1991503002/diff/60001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.cc:59: return false;
On 2016/06/03 00:02:12, Scott Hess wrote:
> On 2016/05/23 19:53:27, Mark P wrote:
> > You changed this from false to true.  I assume it was intentional and there
is
> > no other code that needs to be updated... ? (as no code should actually
reach
> > here instead of failing above)
> 
> Previously all cases returned true, which meant that unexpected errors were
> collected by ADD_FAILURE() and reported at the final "the right errors were
> seen" expectation.  But that just indicated that new errors were happening, it
> didn't tell where they were.  Returning false means the calling code will call
> the DLOG(FATAL) case, which will provide a useful backtrace.  I think that's
> what I originally intended, based on how Connection::ShouldIgnoreError() was
> written.
> 
> ADD_FAILURE() would be preferred, DLOG(FATAL) is generally not preferred for
> tests, but AFAICT ADD_FAILURE() can't provide backtrace info.  I'm trying to
> think of a way to square the circle, but can't.  Since this case can only
happen
> if someone is not running tests on their local machine, I'm inclined to just
go
> with the useful crash, so that what to fix it obvious.

Acknowledged.

https://codereview.chromium.org/1991503002/diff/80001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/1991503002/diff/80001/sql/connection.h#newcod...
sql/connection.h:480: // using ScopedErrorExpecter.  |true| if the error is
expected.
On 2016/06/03 00:02:11, Scott Hess wrote:
> On 2016/05/23 19:53:27, Mark P wrote:
> > 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();
> 
> 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.

I still don't like this comment.  I'd prefer something like:

// Returns true if |error| was expected according to the value passed into
SetErrorExpecter().  Clients which provide an error_callback should use
IsExpectedSqliteError() to check for unexpected errors; if one is detected,
DLOG(FATAL) is generally appropriate.

https://codereview.chromium.org/1991503002/diff/80001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.h (right):

https://codereview.chromium.org/1991503002/diff/80001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.h:52: bool ErrorSeen(int err);
On 2016/06/03 00:02:12, Scott Hess wrote:
> On 2016/05/23 19:53:27, Mark P wrote:
> > Please explain what the return value means.
> 
> Done.

Not done.

[Scott Hess - ex-Googler, 2016-06-20 23:41:30]

Comment wordsmithing.

[Scott Hess - ex-Googler, 2016-06-21 00:17:58]

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

[commit-bot: I haz the power, 2016-06-21 00:18:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991503002/100001

[Scott Hess - ex-Googler, 2016-06-21 00:18:26]

PS5 has the comment changes, appears w/o rebase convolutions.

https://codereview.chromium.org/1991503002/diff/80001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/1991503002/diff/80001/sql/connection.h#newcod...
sql/connection.h:480: // using ScopedErrorExpecter.  |true| if the error is
expected.
On 2016/06/15 04:39:44, Mark P wrote:
> On 2016/06/03 00:02:11, Scott Hess wrote:
> > On 2016/05/23 19:53:27, Mark P wrote:
> > > 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();
> > 
> > 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.
> 
> I still don't like this comment.  I'd prefer something like:
> 
> // Returns true if |error| was expected according to the value passed into
> SetErrorExpecter().  Clients which provide an error_callback should use
> IsExpectedSqliteError() to check for unexpected errors; if one is detected,
> DLOG(FATAL) is generally appropriate.

Done.

https://codereview.chromium.org/1991503002/diff/80001/sql/test/scoped_error_e...
File sql/test/scoped_error_expecter.h (right):

https://codereview.chromium.org/1991503002/diff/80001/sql/test/scoped_error_e...
sql/test/scoped_error_expecter.h:52: bool ErrorSeen(int err);
On 2016/06/15 04:39:44, Mark P wrote:
> On 2016/06/03 00:02:12, Scott Hess wrote:
> > On 2016/05/23 19:53:27, Mark P wrote:
> > > Please explain what the return value means.
> > 
> > Done.
> 
> Not done.

Done for real.  Looks like I started submitting from one machine, continued from
another, and now have a stranded branch with the only difference being a change
to this comment :-).

https://codereview.chromium.org/1991503002/diff/80001/sync/syncable/directory...
File sync/syncable/directory_backing_store_unittest.cc (left):

https://codereview.chromium.org/1991503002/diff/80001/sync/syncable/directory...
sync/syncable/directory_backing_store_unittest.cc:24: #include
"sql/test/scoped_error_ignorer.h"
I'm posting a TBR to remove this IWYU fail in my previous CL, but for some
reason git-cl isn't stitching it in as a dependency.

[commit-bot: I haz the power, 2016-06-21 01:46:12]

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

[commit-bot: I haz the power, 2016-06-21 01:46:14]

Dry run: This issue passed the CQ dry run.

[Mark P, 2016-06-21 04:34:11]

still lgtm a minor quibble (typo?)

https://codereview.chromium.org/1991503002/diff/80001/sync/syncable/directory...
File sync/syncable/directory_backing_store_unittest.cc (left):

https://codereview.chromium.org/1991503002/diff/80001/sync/syncable/directory...
sync/syncable/directory_backing_store_unittest.cc:24: #include
"sql/test/scoped_error_ignorer.h"
On 2016/06/21 00:18:26, Scott Hess wrote:
> I'm posting a TBR to remove this IWYU fail in my previous CL, but for some
> reason git-cl isn't stitching it in as a dependency.

Acknowledged.

I'm sure you'll figure it out.

https://codereview.chromium.org/1991503002/diff/100001/sql/test/scoped_error_...
File sql/test/scoped_error_expecter.h (right):

https://codereview.chromium.org/1991503002/diff/100001/sql/test/scoped_error_...
sql/test/scoped_error_expecter.h:52: // |err| matches an error passed to
ExpectError(), returns |err| and returns
nit: returns -> records ?

[Scott Hess - ex-Googler, 2016-06-21 05:48:32]

typo

[Scott Hess - ex-Googler, 2016-06-21 05:52:58]

Description was changed from

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
==========

to

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org
==========

[Scott Hess - ex-Googler, 2016-06-21 05:52:58]

shess@chromium.org changed reviewers:
+ jam@chromium.org, sdefresne@chromium.org

[Scott Hess - ex-Googler, 2016-06-21 05:55:13]

The changes outside sql/ are mechanical find/replace to follow the renaming in
sql/, so going TBR for:

sdefresne - components/
jam - chrome/

https://codereview.chromium.org/1991503002/diff/80001/sync/syncable/directory...
File sync/syncable/directory_backing_store_unittest.cc (left):

https://codereview.chromium.org/1991503002/diff/80001/sync/syncable/directory...
sync/syncable/directory_backing_store_unittest.cc:24: #include
"sql/test/scoped_error_ignorer.h"
On 2016/06/21 04:34:11, Mark P wrote:
> On 2016/06/21 00:18:26, Scott Hess wrote:
> > I'm posting a TBR to remove this IWYU fail in my previous CL, but for some
> > reason git-cl isn't stitching it in as a dependency.
> 
> Acknowledged.
> 
> I'm sure you'll figure it out.

The rebase picked it up.  Likely just an edge case no worth dealing with.

https://codereview.chromium.org/1991503002/diff/100001/sql/test/scoped_error_...
File sql/test/scoped_error_expecter.h (right):

https://codereview.chromium.org/1991503002/diff/100001/sql/test/scoped_error_...
sql/test/scoped_error_expecter.h:52: // |err| matches an error passed to
ExpectError(), returns |err| and returns
On 2016/06/21 04:34:11, Mark P wrote:
> nit: returns -> records ?

Doh!

[Scott Hess - ex-Googler, 2016-06-21 05:55:20]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2016-06-21 05:55:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1991503002/#ps120001
(title: "typo")

[commit-bot: I haz the power, 2016-06-21 05:55:35]

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

[commit-bot: I haz the power, 2016-06-21 06:56:47]

Description was changed from

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org
==========

to

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org
==========

[commit-bot: I haz the power, 2016-06-21 06:56:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-21 06:58:15]

Description was changed from

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org
==========

to

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

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org

Committed: https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4
Cr-Commit-Position: refs/heads/master@{#400910}
==========

[commit-bot: I haz the power, 2016-06-21 06:58:16]

Patchset 7 (id:??) landed as
https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4
Cr-Commit-Position: refs/heads/master@{#400910}

[sdefresne, 2016-06-21 07:37:41]

rs lgtm for components/