Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run go tests in a package-specific database #1776

Draft
wants to merge 15 commits into
base: master
from

Conversation

2 participants
@jim
Copy link
Contributor

jim commented Feb 21, 2019

Description

I have been doing some work on speeding up our Go tests. For reference, it's creeping up on 12 minutes to run make server_test on my development machine.

By default go test will run the tests of multiple packages concurrently. In the early days of the project, though, we setup the testing make targets to use go test -p 1 to prevent multiple packages from colliding by since they were all using the test_db database. This forced our tests into a serial execution scheme where the total test execution time will be the sum of each individual test's run time.

This PR is an experiment in creating a database (CREATE DATABASE in Postgres) for each Go package and configuring each package's test suite to use its own database. This allows the tests from multiple packages to run concurrently while still avoiding collisions.

Some very preliminary numbers:

On master (d95cd53 to be precise):

$ time make server_test
go test -p 1 -count 1 -short $(go list ./... | grep -v \\/pkg\\/gen\\/ | grep -v \\/cmd\\/)
      694.68 real        99.37 user        45.54 sys

This PR:

$ time make server_test
go test -count 1 -short $(go list ./... | grep -v \\/pkg\\/gen\\/ | grep -v \\/cmd\\/)
      371.99 real       130.87 user        61.30 sys

So far this is promising, but it is worth noting that running the tests in this way doubles the CPU usage of the com.docker.hyperkit process, which seemed to affect the responsiveness of my machine.

I intentionally did not take CI into account, so this build is going to fail.

In the future, there are a few other things to look into, including optimizing the docker database container and re-enabling go test's caching.

@jim jim requested review from chrisgilmerproj and stangah Feb 21, 2019

}

// #nosec G204
dump := exec.Command("pg_dump", "-U", "postgres", "-h", "localhost", "-F", "c", source)

This comment has been minimized.

@jim

jim Feb 21, 2019

Author Contributor

This section of cloneDatabase needs to be cleaned up. Some of this is from me troubleshooting a deadlock resulting from test suite that was still using the test_db database (turns out that pg_dump locks each table as it dumps it).

log.Panicf("failed to clone database '%s' to '%s': %#v", "testdb", dbName, err)
}
fmt.Println("success")
db, err := pop.NewConnection(&pop.ConnectionDetails{

This comment has been minimized.

@jim

jim Feb 21, 2019

Author Contributor

It seems that there isn't a nice way to keep using the database config file and overwriting the database name. I'm wondering if it might be worth just going all in on ENV variables and getting rid of the config file altogether.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 22, 2019

Contributor

I'm pretty sure the config file is just a template that pulls together a bunch of ENV vars anyway. So probably worth just doing that.

https://github.com/transcom/mymove/blob/master/config/database.yml

And of course we could expand that file if needed.

@@ -57,6 +57,7 @@ export MYMOVE_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
export SECURE_MIGRATION_DIR="${MYMOVE_DIR}/local_migrations"
export SECURE_MIGRATION_SOURCE="local"
export DB_PASSWORD=mysecretpassword
export PGPASSWORD=$DB_PASSWORD

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 22, 2019

Contributor

Setting this explicitly should only cause a problem if we have multiple DB instances with different passwords, right?

db *pop.Connection
}

func cloneDatabase(source, destination string) error {
// #nosec G204
drop := exec.Command("dropdb", "-U", "postgres", "-h", "localhost", "--if-exists", destination)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 22, 2019

Contributor

List the port here. My other PR is going to use 5433 for test dbs and it would be nice if I could just modify the value because the flag already exists.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 22, 2019

Contributor

Same comment for all the pg commands :)

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I'm really liking this PR so anything I can do to help get you over the finish line let me know.

I've got just a couple questions that are inline but specifically how the DB config file is being used or not-used. Happy to chat in person.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #1776 into master will decrease coverage by 15.75%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1776       +/-   ##
===========================================
- Coverage   49.59%   33.84%   -15.76%     
===========================================
  Files         429      246      -183     
  Lines       18491     7033    -11458     
  Branches     1632     1632               
===========================================
- Hits         9170     2380     -6790     
+ Misses       8517     4653     -3864     
+ Partials      804        0      -804
user: {{ env "DB_USER" }}
password: {{ env "DB_PASSWORD" }}
options:
sslmode: "require"

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 4, 2019

Contributor

I'm pretty sure this file gets used by our migrations container in AWS. I'd leave it alone or we might see problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.