Unit tests for sql::MetaTable.

Unit tests for sql::MetaTable.

BUG=273203
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221922

[Scott Hess - ex-Googler, 2013-08-27 22:34:56]

I was going to post the move of sql::MetaTable methods into sql::Connection, but
there were no tests which bothered me.  So, first some tests.

[Greg Billock, 2013-08-29 18:05:39]

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc
File sql/meta_table_unittest.cc (right):

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:17: SQLMetaTableTest() {}
needed?

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:30: base::FilePath db_path() {
private? or just build in SetUp?

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:51: const int kVersionFirst = 2, kCompatVersionFirst
= 1;
one per line (and below)

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:66: EXPECT_TRUE(meta_table.Init(&db(),
kVersionSecond, kCompatVersionSecond));
This sure looks like it is changing the version info. The comment says it'll
write the version nums if the table is created, but I think it should say it
ONLY writes them if the table is created. (And why is that?)

[Scott Hess - ex-Googler, 2013-08-29 23:32:26]

thanks.

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc
File sql/meta_table_unittest.cc (right):

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:17: SQLMetaTableTest() {}
On 2013/08/29 18:05:40, Greg Billock wrote:
> needed?

You're right.  Also removing in the place I did the copy/paste inheritance from,
and another in sql/.

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:30: base::FilePath db_path() {
On 2013/08/29 18:05:40, Greg Billock wrote:
> private? or just build in SetUp?

Sure.  And connection_unittest.cc while I'm there.

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:36: sql::Connection db_;
Hmm.  Also, seems iffy to have temp_dir_ destructed before db_ (which is in
temp_dir_).  Moving it to the end.

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:51: const int kVersionFirst = 2, kCompatVersionFirst
= 1;
On 2013/08/29 18:05:40, Greg Billock wrote:
> one per line (and below)

Done.

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:66: EXPECT_TRUE(meta_table.Init(&db(),
kVersionSecond, kCompatVersionSecond));
On 2013/08/29 18:05:40, Greg Billock wrote:
> This sure looks like it is changing the version info. The comment says it'll
> write the version nums if the table is created, but I think it should say it
> ONLY writes them if the table is created. (And why is that?)

I believe that the idea is that on first run, the database is presumed empty, so
after the schema stuff runs it will be the current versions in force.  On a
later run, an existing meta table means that the current version needs to be
migrated, so it isn't overwritten.

I think in the future this needs to separate the functions of initializing the
database instance to use from checking for version info from setting up the
initial meta table.  Per the bug, mixing initial setup with migration is a poor
idea.

[Scott Hess - ex-Googler, 2013-09-05 19:57:11]

any additional comments?

[Greg Billock, 2013-09-06 16:02:09]

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc
File sql/meta_table_unittest.cc (right):

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:30: base::FilePath db_path() {
On 2013/08/29 23:32:26, shess wrote:
> On 2013/08/29 18:05:40, Greg Billock wrote:
> > private? or just build in SetUp?
> 
> Sure.  And connection_unittest.cc while I'm there.

I meant just build db_path_ in SetUp and don't have this function. I don't think
its used elsewhere, right?

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:36: sql::Connection db_;
On 2013/08/29 23:32:26, shess wrote:
> Hmm.  Also, seems iffy to have temp_dir_ destructed before db_ (which is in
> temp_dir_).  Moving it to the end.

This was right, no? Destruction is in reverse order of declaration.

https://codereview.chromium.org/23649002/diff/5001/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/23649002/diff/5001/sql/connection_unittest.cc...
sql/connection_unittest.cc:102: const base::FilePath& db_path() { return
db_path_; }
Here it gets used in tests. So having both in the suite seems fine.

[Scott Hess - ex-Googler, 2013-09-06 16:28:28]

Please take another look?  Thanks.

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc
File sql/meta_table_unittest.cc (right):

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:30: base::FilePath db_path() {
On 2013/09/06 16:02:09, Greg Billock wrote:
> On 2013/08/29 23:32:26, shess wrote:
> > On 2013/08/29 18:05:40, Greg Billock wrote:
> > > private? or just build in SetUp?
> > 
> > Sure.  And connection_unittest.cc while I'm there.
> 
> I meant just build db_path_ in SetUp and don't have this function. I don't
think
> its used elsewhere, right?

Done.  Didn't even notice that :-).

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:36: sql::Connection db_;
On 2013/09/06 16:02:09, Greg Billock wrote:
> On 2013/08/29 23:32:26, shess wrote:
> > Hmm.  Also, seems iffy to have temp_dir_ destructed before db_ (which is in
> > temp_dir_).  Moving it to the end.
> 
> This was right, no? Destruction is in reverse order of declaration.

Darn it.  You are correct, and I know this, but it was clearly wrong when I made
the change.  I hate C++.

[Greg Billock, 2013-09-06 23:19:34]

lgtm

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc
File sql/meta_table_unittest.cc (right):

https://codereview.chromium.org/23649002/diff/1/sql/meta_table_unittest.cc#ne...
sql/meta_table_unittest.cc:36: sql::Connection db_;
On 2013/09/06 16:28:28, shess wrote:
> On 2013/09/06 16:02:09, Greg Billock wrote:
> > On 2013/08/29 23:32:26, shess wrote:
> > > Hmm.  Also, seems iffy to have temp_dir_ destructed before db_ (which is
in
> > > temp_dir_).  Moving it to the end.
> > 
> > This was right, no? Destruction is in reverse order of declaration.
> 
> Darn it.  You are correct, and I know this, but it was clearly wrong when I
made
> the change.  I hate C++.

:-) I feel your pain. :-)

[commit-bot: I haz the power, 2013-09-07 00:13:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/23649002/23001

[commit-bot: I haz the power, 2013-09-07 15:48:37]

Change committed as 221922