Improve performance of the Go test suite #2060

mna · 2021-09-14T19:54:55Z

Closes #1805 .

This PR addresses two categories of slow tests: network requests and creating temporary mysql databases.

Network Requests

I introduced the NETWORK_TEST=1 environment variable, similar to how we have MYSQL_TEST and REDIS_TEST to enable those specific tests. I did enable them on CI as we do want to run them, but it is easier to skip when running tests locally (i.e. make test-go does not run them automatically).

It's not the ideal solution, I would've liked to mock them completely by starting a test server and mocking responses, but after a closer look this seems quite complex to setup as the data returned has a specific format (e.g. for vulnerabilities) and hashes that must match and the requests themselves are sometimes made quite deep in the code (even in 3rd-party code).

Temporary MySQL Databases

I refactored the tests in server/datastore/mysql to use sub-tests and drop the number of temporary databases created from 130 to 24. I opted to keep a top-level test (that creates the temp DB) for each "entity" (or .go file) to make it easier to identify the cleanup required and so that the test setup applies to the tests in the same file, a bit easier to reason about and to read the tests that way, I think.

The speedup is quite interesting too, comparing MYSQL_TEST=1 go test ./server/datastore/mysql/ -count=1 locally on this branch vs main, I get ~10s vs ~30s (though part of it is also in reducing the number of records created in "load" tests to verify mysql locking).

Also, the race detector (MYSQL_TEST=1 go test ./server/datastore/mysql/ -count=1 -race) now passes, there was a data race previously (in the test code, so nothing critical).

Checklist for submitter

If some of the following don't apply, please write a short explanation why.

~~Changes file added (if needed)~~
~~Documented any API changes~~
~~Added tests for all functionality~~
~~Manual QA for all functionality~~

codecov-commenter · 2021-09-14T20:27:19Z

Codecov Report

Merging #2060 (859e3e4) into main (1fa5ce1) will increase coverage by 0.11%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
+ Coverage   48.03%   48.15%   +0.11%     
==========================================
  Files         224      224              
  Lines       21266    21271       +5     
==========================================
+ Hits        10216    10243      +27     
+ Misses       9834     9809      -25     
- Partials     1216     1219       +3

Impacted Files	Coverage Δ
server/datastore/mysql/software.go	`74.44% <75.00%> (+0.48%)`	⬆️
server/sso/session_store.go	`59.45% <0.00%> (+59.45%)`	⬆️

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 1fa5ce1...859e3e4. Read the comment docs.

cmd/fleetctl/convert_test.go

mna · 2021-09-20T15:21:22Z

server/datastore/mysql/activities_test.go

@@ -10,10 +10,25 @@ import (
 	"github.com/stretchr/testify/require"
 )

-func TestActivityUsernameChange(t *testing.T) {
+func TestActivity(t *testing.T) {


@chiiph as discussed in #1821 , this is how I see "test suites" being implemented in standard Go tests/sub-tests:

the "suite" is the top-level test function (for mysql I created one by test file, with the name being the name of the file/entity).

the suite setup/teardown is any code executed and deferred (respectively) before the sub-tests (can also be t.Cleanup instead of defer, as is the case in CreateMySQLDS).

I used a table-driven approach to run the suite's tests, as it tends to minimize code repetition.

Per-test setup/teardown is just the code inside the t.Run closure that runs before calling the test function.

If some test has specific setup/teardown, of course it can simply be done in that test's function.

In this case I did not need any shared state so I didn't use a struct to hold the suite's tests, but it is a possibility (I could've used a struct and store e.g. the datastore there so its methods would've accessed it there instead of as an argument).

Yeah, makes sense and it's simple enough.

server/datastore/mysql/software_test.go

server/datastore/mysql/software.go

server/datastore/mysql/sessions_test.go

server/datastore/mysql/activities_test.go

mna · 2021-09-20T15:37:14Z

server/datastore/mysql/hosts_test.go

@@ -1584,7 +1558,9 @@ func TestSaveTonsOfUsers(t *testing.T) {
 				errCh <- err
 				return
 			}
-			atomic.AddInt32(&count1, 1)
+			if atomic.AddInt32(&count1, 1) >= 100 {


@chiiph let me know if this is still good enough for the goal of the test (which I believe was related to locking when adding many users concurrently). I tried to find a balance between the batch size and the test duration.

It's probably fine, the locking happened pretty much right away.

server/datastore/mysql/hosts_test.go

chiiph

Looks great! Left a few minor comments.

chiiph · 2021-09-20T17:04:34Z

.github/workflows/test-go.yaml

@@ -41,7 +41,7 @@ jobs:

    - name: Run Go Tests
      run: |
-        MYSQL_TEST=1 make test-go
+        NETWORK_TEST=1 REDIS_TEST=1 MYSQL_TEST=1 make test-go


Not a huge fan of these, but I get it.

We did remove REDIS_TEST, so we should remove it from here. I would also like to remove MYSQL_TEST entirely and just keep working on making things better.

Yeah, I agree with you, it's not very discoverable, but as mentioned in the description the approach I would've preferred was quite an undertaking, especially without fully understanding that part of the code. I believe the REDIS_TEST env var is still used in the server/sso tests.

Oh... darn, missed that :( Allllrighty then

chiiph · 2021-09-20T17:08:23Z

server/datastore/mysql/activities_test.go

@@ -10,10 +10,25 @@ import (
 	"github.com/stretchr/testify/require"
 )

-func TestActivityUsernameChange(t *testing.T) {
+func TestActivity(t *testing.T) {


Yeah, makes sense and it's simple enough.

chiiph · 2021-09-20T17:17:43Z

server/datastore/mysql/hosts_test.go

@@ -1584,7 +1558,9 @@ func TestSaveTonsOfUsers(t *testing.T) {
 				errCh <- err
 				return
 			}
-			atomic.AddInt32(&count1, 1)
+			if atomic.AddInt32(&count1, 1) >= 100 {


It's probably fine, the locking happened pretty much right away.

chiiph · 2021-09-20T17:19:00Z

server/datastore/mysql/invites_test.go

+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			defer TruncateTables(t, ds, "teams", "invites", "invite_teams")


Would truncating all the tables be much slower?

Probably not, though some are seeded by the sql schema file and should be left alone. It's usually quite clear when one is missing (as a subsequent test will fail with a duplicate error or wrong number of elements, etc.), but yeah I don't think it'd be significantly slower. I'll try it out.

Updated the PR, it seems roughly as fast (10-12 seconds on my machine).

chiiph · 2021-09-20T17:23:50Z

server/datastore/mysql/testing_utils.go

@@ -233,7 +233,9 @@ func createMySQLDSWithOptions(t *testing.T, opts *DatastoreTestOptions) *Datasto
 		strings.TrimPrefix(details.Name(), "github.com/fleetdm/fleet/v4/"), "/", "_",
 	)
 	cleanName = strings.ReplaceAll(cleanName, ".", "_")
-	return initializeDatabase(t, cleanName, opts)
+	ds := initializeDatabase(t, cleanName, opts)
+	t.Cleanup(func() { ds.Close() })


mna force-pushed the mna-1805-faster-tests branch 2 times, most recently from 1b81577 to e44b2b1 Compare September 20, 2021 14:42

mna added 17 commits September 20, 2021 10:45

Remove Parallel call in tests, seems to make them slower

a75e144

Actually enable redis tests on CI, document the env var

a2cf104

Move network tests behind an env var

324c8ce

Make slowest tests a bit faster

f0454c3

Make hosts test faster, fix data race

baad9eb

Implement shared mysql db for all activities tests

7a39966

Refactor app_configs tests to use shared top-level db

61cba20

Refactor campaigns tests to use shared top-level db

185f6ab

Refactor carves tests to use shared top-level db

9bae77e

Refactor delete tests to use shared top-level db

c17bc77

Refactor email_changes, hosts tests to use shared top-level db

17c280e

Refactor invites tests to use shared top-level db

8f54796

Refactor labels tests to use shared top-level db

99d10a4

Refactor packs tests to use shared top-level db

28cac37

Remove printf debugging

07ff39f

Refactor more tests to use shared top-level db

660a119

Refactor more tests to use shared top-level db

77a9725

mna force-pushed the mna-1805-faster-tests branch from e44b2b1 to 77a9725 Compare September 20, 2021 14:48

mna commented Sep 20, 2021

View reviewed changes

mna marked this pull request as ready for review September 20, 2021 15:38

mna requested review from chiiph, zwass and edwardsb September 20, 2021 15:39

mna changed the title ~~[wip] Improve performance of the Go test suite~~ Improve performance of the Go test suite Sep 20, 2021

chiiph reviewed Sep 20, 2021

View reviewed changes

Truncate all tables by default

bc4bf9e

chiiph approved these changes Sep 20, 2021

View reviewed changes

mna merged commit 86dce78 into fleetdm:main Sep 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of the Go test suite #2060

Improve performance of the Go test suite #2060

mna commented Sep 14, 2021 •

edited

Loading

codecov-commenter commented Sep 14, 2021 •

edited

Loading

mna Sep 20, 2021

chiiph Sep 20, 2021

mna Sep 20, 2021

chiiph Sep 20, 2021

chiiph left a comment

chiiph Sep 20, 2021

mna Sep 20, 2021

chiiph Sep 20, 2021

chiiph Sep 20, 2021

chiiph Sep 20, 2021

chiiph Sep 20, 2021

mna Sep 20, 2021

mna Sep 20, 2021

chiiph Sep 20, 2021

Improve performance of the Go test suite #2060

Improve performance of the Go test suite #2060

Conversation

mna commented Sep 14, 2021 • edited Loading

Network Requests

Temporary MySQL Databases

Checklist for submitter

codecov-commenter commented Sep 14, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiiph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mna commented Sep 14, 2021 •

edited

Loading

codecov-commenter commented Sep 14, 2021 •

edited

Loading