Speedup local tests #171

alovak · 2020-09-21T10:58:49Z

The goal of this PR is to decrease time we spend waiting for a test results and make us more productive. Currently on my machine running all tests takes ~2m:50s. This makes me run tests less frequently or rely on CI (which takes even more time because it runs linters, etc.). Main time consuming part of our system is testing with MySQL. Specifically: run docker container with MySQL and run migrations. We do this multiple times if we run full test suite.

Here is how we can speedup our tests:

Run MySQL database instance locally and use it for all our tests
Create database and run migrations for this MySQL instance
Use transaction isolated sql driver for our tests to rollback all changes after db connection is closed.
Run Paygate and Watchman instances locally and use if for tests

This PR does all this changes. On my machine running all tests takes 9s now.

Here is how you can get it:

COMPOSE_FILE=docker-compose.dev.yml docker-compose up -d
make setup_test_db
WATCHMAN_ENDPOINT=http://localhost:8084 PAYGATE_ENDPOINT=http://localhost:8082 MYSQL_TEST=1 go test ./...

Let's discuss what's the best way for us to make it convenient to run tests with local instances vs with dockertest.

Also, as a side effect new binary that allows us to manage db was introduced:

./bin/db setup - drop, create and run db migrations
./bin/db drop - drop db
./bin/db create - create db
./bin/db migrate - run migrations

All commands works with default MySQL config and may be updated with environment variables:

root password (MYSQL_ROOT_PASSWORD): secret
user (MYSQL_USER): moov
password (MYSQL_PASSWORD): secret
address (MYSQL_ADDRESS): tcp(localhost:3306)
database (MYSQL_DATABASE): paygate_test

This is used for testing, but I truly believe it should be used in production as well. Why? Currently we run migrations when we start server (inside Connect method). When we have multiple instances of customers (for HA) each of them will run migrations and in case of simultaneous run it may result in an error and failure to start server.

codecov-commenter · 2020-09-21T11:05:08Z

Codecov Report

Merging #171 into master will decrease coverage by 0.27%.
The diff coverage is 42.10%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   62.38%   62.11%   -0.28%     
==========================================
  Files          43       43              
  Lines        2289     2307      +18     
==========================================
+ Hits         1428     1433       +5     
- Misses        592      602      +10     
- Partials      269      272       +3

adamdecaf

I'm confused about a lot of this and if we really need to do this. Our apps need to be uniform and work under go test with minimal configuration otherwise.

adamdecaf · 2020-09-22T00:23:06Z

cmd/db/main.go

+func runCmd(cmd string, config *Config) error {
+	switch cmd {
+	case "setup":
+		err := dropDB(config)


We cannot drop the database. This needs to never be an option in production in fact we will not grant a service DROP.

For tests this may be useful. For example if you want to test some specific migrations, etc. We can add -force flag for drop and make it work only if database has _test.

adamdecaf · 2020-09-22T00:35:30Z

Sorry this is a big comment. I'm against this right now.

I understand we have slow tests when running go test ./... but there are several things need to investigate first.

Using -short for go test ./... works for most use-cases. Why can't we use that? If you're not modifying remote service calls those tests won't matter.

Questions

How many MySQL containers are being launched?
Which specific Customers tests are the slowest? Why?
Can we consolidate tests so we start less MySQL containers?
What would it look like to spin up one instance of MySQL and create random databases for each test that needs it?
Are PayGate and Watchman that slow in tests? Can we look at consolidating those tests or use a shared instance?
1. Running apps locally (instead of their docker images) makes Customers worry about how those apps operate.
  - e.g. Customers having a paygate.yaml config file is a big red flag.
Should we look at gorm for this project? We're clearly fighting my half-baked setup in this app, so is that the problem?

Summary
Can we make an issue with the biggest problems to tackle here? We're trying to put a lot of changes in Customers each week going forward so big changes need to be pretty carefully done.

We need to keep our services fairly similar amongst each other. Otherwise our repositories will sprawl with how they operate and that causes additional developer headaches.

alovak · 2020-09-22T08:25:36Z

internal/database/mysql_test.go

@@ -31,6 +33,10 @@ func TestMySQL__basic(t *testing.T) {
 	if conn != nil || err == nil {
 		t.Fatalf("conn=%#v expected error", conn)
 	}
+
+	fmt.Println("Hello")


I'll remove this :)

alovak added 2 commits September 21, 2020 11:17

speedup mysql tests by using external db and tansactons

fb3c1fe

create ./bin/db to manage database (at least for testing)

1d26964

alovak force-pushed the speedup-tests branch from 8ea6227 to 1d26964 Compare September 21, 2020 11:01

alovak added 5 commits September 21, 2020 14:46

run paygate client test with local paygate instance

13317b2

run watchman test with local watchman instance

66640d0

add db, paygate images into dev compose file

a90dfde

remove debug statements

d1969a0

use dev compose for github actions

511d387

alovak requested review from adamdecaf and InfernoJJ September 21, 2020 12:17

alovak added 5 commits September 21, 2020 15:23

try to run docker compose inside github actions

9e54fbd

try to run compose

d116a3a

try to run compose only on linux

89b485f

try to use mysql as a github action service

3f7a40e

fix go flow

64e82b4

alovak changed the title ~~Speedup tests~~ Speedup local tests Sep 21, 2020

alovak added 2 commits September 21, 2020 15:55

revert running containers on CI

7c0d04a

rename method and test for err (to increase test cov)

e9f7c40

adamdecaf suggested changes Sep 21, 2020

View reviewed changes

alovak requested a review from adamdecaf September 21, 2020 15:48

adamdecaf suggested changes Sep 22, 2020

View reviewed changes

alovak commented Sep 22, 2020

View reviewed changes

alovak linked an issue Sep 23, 2020 that may be closed by this pull request

Testing speedup #183

Open

alovak closed this Oct 19, 2020

alovak mentioned this pull request Oct 31, 2020

database: optimizing mysql moov-io/base#120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speedup local tests #171

Speedup local tests #171

alovak commented Sep 21, 2020 •

edited

codecov-commenter commented Sep 21, 2020 •

edited

adamdecaf left a comment

adamdecaf Sep 22, 2020

alovak Sep 22, 2020

adamdecaf commented Sep 22, 2020 •

edited

alovak Sep 22, 2020

Speedup local tests #171

Speedup local tests #171

Conversation

alovak commented Sep 21, 2020 • edited

codecov-commenter commented Sep 21, 2020 • edited

Codecov Report

adamdecaf left a comment

Choose a reason for hiding this comment

adamdecaf Sep 22, 2020

Choose a reason for hiding this comment

alovak Sep 22, 2020

Choose a reason for hiding this comment

adamdecaf commented Sep 22, 2020 • edited

alovak Sep 22, 2020

Choose a reason for hiding this comment

alovak commented Sep 21, 2020 •

edited

codecov-commenter commented Sep 21, 2020 •

edited

adamdecaf commented Sep 22, 2020 •

edited