Use sqlite3 as the default driver for unit tests by codingllama · Pull Request #886 · google/trillian

codingllama · 2017-11-02T18:36:15Z

The testdb package adds utilities to create test databases for different
drivers. Currently, it supports sqlite3 (default) and mysql. The default driver
can be specified by the TRILLIAN_SQL_DRIVER environment variable, allowing tests
to transparently execute against different databases/drivers.

Adjustments done to existing code:

storage.sql declares unique constraints separately from the table declaration
(functionally equivalent, but more portable)
A few constants in query strings are now quoted (eg, 'false' instead of
false, otherwise SQLite interprets the constant as a column name)
Duplicate handling (isDuplicateErr) handles both MySQL and SQLite error codes
map_storage.go no longer distinguishes between empty and nil mapper metadata
(SQLite writes []byte{} as NULL). (This is a positive change in many ways, as
an empty Any proto is not valid due to having no type_url.)

A few test loops that defer-close resources had their scope reduced (using
anonymous functions), as doing so greatly reduces test duration on SQLite.

Finally, integration.GetTestDB has been completely replaced by
testdb.NewTrillianDB.

codingllama · 2017-11-02T18:37:12Z

Fixes #456.

codecov-io · 2017-11-07T12:28:49Z

Codecov Report

Merging #886 into master will decrease coverage by 0.02%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #886      +/-   ##
==========================================
- Coverage   64.45%   64.43%   -0.03%     
==========================================
  Files          91       91              
  Lines        7577     7581       +4     
==========================================
+ Hits         4884     4885       +1     
- Misses       2237     2242       +5     
+ Partials      456      454       -2

Impacted Files	Coverage Δ
storage/mysql/tree_storage.go	`51.65% <ø> (-1.43%)`	⬇️
storage/mysql/admin_storage.go	`75.29% <ø> (ø)`	⬆️
storage/mysql/map_storage.go	`70% <100%> (ø)`	⬆️
storage/mysql/log_storage.go	`75.07% <77.77%> (-0.3%)`	⬇️
util/tracker.go	`100% <0%> (+3.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90b0e23...4d1911b. Read the comment docs.

AlCutter · 2017-11-08T15:44:07Z

.travis.yml

-  - cd $HOME/gopath/src/github.com/google/trillian
-  - HAMMER_OPTS="--operations=50" ./integration/maphammer.sh 3
+  - |
+      if [[ -n "${TRILLIAN_SQL_DRIVER}" && "${TRILLIAN_SQL_DRIVER}" != sqlite3 ]]; then


"sqlite3"?

or, just to capture an IRL chat, flip this to check for mysql since that what the scripts contained in the block are expecting?

Flipped and quoted.

AlCutter · 2017-11-08T15:55:29Z

integration/maptest/map_test.go

 )

 func TestMapIntegration(t *testing.T) {
+	if provider := testdb.Default(); !provider.IsMySQL() {


Would this be a useful test helper func to share with the other tests?

I think this is OK, it's not that verbose and both the Skip and the message are clearly visible in the test.

AlCutter · 2017-11-08T15:59:47Z

storage/mysql/log_storage_test.go

+			j, logID := j, logID
+			func() {
+				tx := beginLogTx(s, logID, t)
+				defer tx.Close()


Did you do this func(){}() to force the defer to run or was there another subtle reason?
If it's the former, maybe it's better to just explicitly do the tx.Close() below, 'cos the function is a bit confusing (it looks like it might go async or something).

One suggestion might be something like a func runWithTX(s, logID, f func(tx) err) err and you'd do here:

for ... { err := runWithTX(s, logID, func f(tx ...) err { numToAdd .. ... if bad { return errors.New(...) } ... return nil }) if err != nil { t.Error... } }

and runWithTX does the

tx := beginLog... defer tx.Close() return f(tx)

Yes, the funcs were because of the defer-close. I went with your first suggestion and explicitly closed the tx.

I have been meaning to add runInTX methods for a while now, but in a slightly different form. I'll save those to another PR.

AlCutter · 2017-11-08T16:01:39Z

storage/mysql/log_storage_test.go

-		if diff := pretty.Compare(expectedCount, got); diff != "" {
-			t.Errorf("GetUnsequencedCounts() = diff -want +got:\n%s", diff)
-		}
+		func() {


AlCutter · 2017-11-08T16:08:09Z

storage/mysql/storage.sql

  FOREIGN KEY(TreeId) REFERENCES Trees(TreeId) ON DELETE CASCADE
 );

+CREATE UNIQUE INDEX TreeHeadRevisionIdx


(curiosity) is there a material difference between this and how it was declared before?

Apart from having an explicit name, I think it's the same.

AlCutter · 2017-11-08T16:10:47Z

storage/testdb/testdb.go

+
+// Provider is an object capable of creating new test databases.
+type Provider struct {
+	Driver           string


Do these need comments too?

AlCutter · 2017-11-08T16:15:35Z

storage/testdb/testdb.go

+			continue // skip empty lines and comments
+		}
+		if p.Driver == sqliteDriver {
+			line = enumRegex.ReplaceAllString(line, "VARCHAR(50)")


I'll assume those are happy tears. ;)

codingllama · 2017-11-08T18:07:57Z

@AlCutter: all comments addressed, PTAL.

The testdb package creates new databases using either mysql and sqlite, allowing us to switch between drivers transparently. The default driver for tests can be overriden via the TRILLIAN_SQL_DRIVER environment variable.

…sql'

codingllama · 2017-11-08T18:51:13Z

Thanks! Rebased on master, that should fix Travis errors.

This partly-reverts commit 4023e5a ("Use sqlite3 as the default driver for unit tests (google#886)"), removing SQLite as a storage option. In practice, having a default DB that doesn't support much of the Trillian functionality (e.g. none of the map tests, none of the integration tests) has inconvenienced core developers. However, to allow the core `git clone <repo> ; go test ./...` scenario to still work without setup dependencies, all DB-using tests explicitly check for MySQL availability and are skipped if MySQL is not present.

This partly-reverts commit 4023e5a ("Use sqlite3 as the default driver for unit tests (#886)"), removing SQLite as a storage option. In practice, having a default DB that doesn't support much of the Trillian functionality (e.g. none of the map tests, none of the integration tests) has inconvenienced core developers. However, to allow the core `git clone <repo> ; go test ./...` scenario to still work without setup dependencies, all DB-using tests explicitly check for MySQL availability and are skipped if MySQL is not present. A single deliberate test failure is emitted in this situation as a reminder that many tests are being skipped.

googlebot added the cla: yes label Nov 2, 2017

codingllama force-pushed the sqlite branch 2 times, most recently from a320cff to 2e39e81 Compare November 3, 2017 18:07

codingllama added the WIP label Nov 3, 2017

codingllama force-pushed the sqlite branch from 2e39e81 to 51f13a5 Compare November 7, 2017 11:44

codingllama removed the WIP label Nov 7, 2017

codingllama requested a review from AlCutter November 7, 2017 12:33

AlCutter reviewed Nov 8, 2017

View reviewed changes

AlCutter self-assigned this Nov 8, 2017

AlCutter approved these changes Nov 8, 2017

View reviewed changes

Alan Parra added 10 commits November 8, 2017 18:49

Add testdb package

14db24d

The testdb package creates new databases using either mysql and sqlite, allowing us to switch between drivers transparently. The default driver for tests can be overriden via the TRILLIAN_SQL_DRIVER environment variable.

Fix storage tests for sqlite compatibility

342554a

Fix integration and MySQL-dependent tests

c7c4758

Fix integration scripts

7132144

Remove integration.GetTestDB

9e70d6a

Travis tests against both SQLite and MySQL

d3bb265

Update README.md

21ef563

Reinstate the 3rd parameter to NewLogEnv (referenced by ct-go)

d555ff7

Review comments: simplify Travis check to '$TRILLIAN_SQL_DRIVER == my…

2c4cb9e

…sql'

Review comments: manually close TXs, add comments

bee27e7

codingllama force-pushed the sqlite branch from 28eba50 to bee27e7 Compare November 8, 2017 18:49

Fix megacheck warnings

4d1911b

codingllama merged commit 4023e5a into google:master Nov 9, 2017

codingllama deleted the sqlite branch November 9, 2017 12:24

daviddrysdale mentioned this pull request Mar 21, 2018

Drop use of SQLite #1064

Merged

m0r3n0cabr0n94 approved these changes Aug 20, 2024

View reviewed changes

Conversation

codingllama commented Nov 2, 2017

Uh oh!

codingllama commented Nov 2, 2017

Uh oh!

codecov-io commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AlCutter Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codingllama commented Nov 8, 2017

Uh oh!

codingllama commented Nov 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Nov 7, 2017 •

edited

Loading

AlCutter Nov 8, 2017 •

edited

Loading