Create mock DB setup, postgres integration tests #76
Conversation
bobheadxi
force-pushed
the
test-db
branch
2 times, most recently
from
fe01004
to
9eeb70b
Compare
bobheadxi
force-pushed
the
test-db
branch
4 times, most recently
from
ccf5ae3
to
d23defe
Compare
bobheadxi
changed the title
There was a problem hiding this comment.
This looks good, but I really think we should containerize the Postgres setup for integration tests for a few reasons:
1.) Having to set up Postgres on your machine just to run integration tests sucks.
2.) If someone is running Postgres on their machine they probably don't want Rocket's setup and tear-down code to mess with it.
3.) Containerizing integration tests greatly reduces the odds that tests fail because of unrelated issues (like someone screwing up their local Postgres, not having Postgres installed etc.), and it makes the tests more reliable reproducible.
4.) Having clean-up logic run at the end of the tests doesn't guarantee that stuff always gets cleaned up. A simple example is a case where there's a SIGSEGV mid way through the test so it exits before clean-up. Running this in a container would require no clean-up at all.
5.) Postgres already runs in a container in production so it's weird to do it differently in the tests.
Makefile
Outdated
| clean: | ||
| rm rocket | ||
| rm -f rocket | ||
| pg_ctl -D /usr/local/var/postgres stop -s -m fast |
There was a problem hiding this comment.
I don't think it's safe for a Make target to me shutting down Postgres on people's machines. I think that if you're going to shut down someone's Postgres you should at least make it obvious that you're doing so by making it a separate target like shutdown-db. With this setup your Postgres server gets shut down whenever your run tests.
| err = dal.GetMemberBySlackID(memberGet) | ||
| assert.Nil(t, err) | ||
| assert.False(t, memberGet.IsAdmin) | ||
| } |
mock_db.sh
Outdated
| createuser -s rocket_test | ||
| createdb rocket_test_db | ||
| psql -d rocket_test_db -a -f ./schema/tables.sql | ||
| echo "MAKE: Local database ready to go!" |
There was a problem hiding this comment.
I think we could get rid of most of this logic by just containerizing our DB for tests. Also, we don't need to wait for the DB to start with a script like this, go-pg does it for you: https://github.com/ubclaunchpad/rocket/blob/master/data/dal.go#L24.
mock_db.sh
Outdated
|
|
||
| createuser -s rocket_test | ||
| createdb rocket_test_db | ||
| psql -d rocket_test_db -a -f ./schema/tables.sql |
There was a problem hiding this comment.
If this were containerized you could take care of some of this by simply copying the schema to the right location in the Dockerfile:
COPY schema.sql /docker-entrypoint-initdb.d/
There was a problem hiding this comment.
tbh I've found Docker sucks even more for testing - starting anything is much slower than a simple pg instance, but I'll put this in docker
|
rebased |
| if !ok { | ||
| return errors.New("dal.db not of type *pg.DB") | ||
| } | ||
| return database.Close() |
There was a problem hiding this comment.
This is fine, but maybe we should just panic if this doesn't work. I say that because usually you do checks like this when you've received something and you're not quite sure what it is. But in this case an error would mean that we've fundamentally broken our DB integration so a panic might be appropriate. Up to you, let me know what you think.
There was a problem hiding this comment.
That's a good point, I'll make it panic
There was a problem hiding this comment.
@bfbachmann actually apparently .Close() is never called anywhere in the codebase 😅
And come to think of it, in the event .Close() is called, Rocket should be in the process of shutting down already - it might be fine to leave this as is. I'll try to find a place to put a .Close() somewhere. (edit: see db7583c)
| // Create a new member | ||
| member := &model.Member{ | ||
| SlackID: "1234", | ||
| Name: "Big Bruno", |
There was a problem hiding this comment.
Minor: We should probably insert and update as many values for this member as possible, just to make sure that all our columns are configured correctly in the DB and Rocket. If you want you can leave this as-is for now and I can write more tests for this stuff later since you've already done the hard part of setting up the test DB in this PR.
There was a problem hiding this comment.
Yeah I wrote these tests just to make sure the test setup and rollback and everything was working fine, like you say all these tests could use some dedicated love in a separate PR
Added mock DB setup, with
func newTestDBConnection() (*DAL, func(), error)that begins transactions and rolls back after each test.Bump postgres driver version (current one is more than a year old), update
Update()functions accordingly with newWherePK()requirement. Read up on it a bit and there doesn't seem to be any other breaking changes.Change
data.dal.dbtype from*pg.DBtoorm.DB, an interface shared betweenpg.Txandpg.DBfor testing purposes.Updated README documentation.
Coverage dropped a bit because a new package has been introduced to Coveralls (it doesn't automatically calculate coverage against all packages, only those that have at least one test)