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

Tear down each cloned DB instance at the end of each test suite run #2774

Merged
merged 27 commits into from Oct 7, 2019

Conversation

@chrisgilmerproj
Copy link
Contributor

commented Oct 3, 2019

Description

I'm positing that the reason we're having problems in CircleCI with our golang unit tests right now is due to a strange combination of things:

  1. Test suites are not doing cleanup by calling Close() on the connection.
  2. The postgres container we're running tests against may have a limited volume size meaning we can't just make N databases and not drop them
  3. The service layer tests (and maybe others) have multiple PopTestSuite's. Because each suite creates a DB based on the name of the package and the package name is not unique its possible for name collision if we run in parallel. See pkg/service/invoice for an example of 3 suites.
  4. The operation which clones the DB to create new DBs for each test is not safe to run in parallel on resource constrained machines like in CircleCI. Implementing a file-lock on this resource prevents this kind of resource contention.

This PR tries to solve all of these.

Setup

make server_run

If you log into the postgres instance to look at the DB's you'll see something like this now:

psql-test

Screen Shot 2019-10-03 at 4 56 14 PM

ServePublicAPIlFlag string = "serve-api-internal"
// ServeAPIExternalFlag is the external api service flag
ServeInternalAPIFlag string = "serve-api-public"
ServeAPIInternalFlag string = "serve-api-internal"

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 4, 2019

Author Contributor

These were backwards!

@codecov

This comment has been minimized.

Copy link

commented Oct 4, 2019

Codecov Report

Merging #2774 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #2774     +/-   ##
========================================
+ Coverage    57.8%   57.8%   +0.1%     
========================================
  Files         280     278      -2     
  Lines       12635   12623     -12     
========================================
- Hits         7296    7293      -3     
+ Misses       4591    4582      -9     
  Partials      748     748
Impacted Files Coverage Δ
pkg/cli/services.go 83.4% <100%> (-1.8%) ⬇️
Copy link
Contributor

left a comment

Small nitpick and a question.

@@ -25,9 +25,6 @@ func (suite *HandlerSuite) SetupTest() {

// AfterTest completes tests by trying to close open files
func (suite *HandlerSuite) AfterTest() {
for _, file := range suite.TestFilesToClose() {
file.Data.Close()

This comment has been minimized.

Copy link
@gidjin

gidjin Oct 4, 2019

Contributor

What are "test files" in this context? Also, we should remove the comment above this function since it no longer applies. Alternatively, why leave the method if it's a no-op?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 4, 2019

Author Contributor

No idea. This appears to be stale code. I'll update the comment.

@gidjin
gidjin approved these changes Oct 4, 2019
Copy link
Contributor

left a comment

Assuming you match up the number of threads to the comment, or just remove the number from the comment

Makefile Show resolved Hide resolved
@lynzt
lynzt approved these changes Oct 4, 2019
Copy link
Contributor

left a comment

looks good and thanks for cleaning up the publicapi endpoint.
I'm curious if this fixes some of the server_test flakiness... 🤞

question: when looking at the db, are we creating a separate db per test?
this is a subset of what I'm seeing when I do a list in psql:
test_handlers_internalapi_7nqy3d, test_migrate_eer4hs, test_services_fuelprice_llmu79

if err := dropDB(suite.dbName); err != nil {
return err
}
return nil

This comment has been minimized.

Copy link
@lynzt

lynzt Oct 4, 2019

Contributor

would it make sense to panic here vs in each _test.go?
just wondering if we can get rid of some of the duplication, idk:

if err := hs.PopTestSuite.TearDown(); err != nil {
	panic(err)
}

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 4, 2019

Author Contributor

We're just talking about that in Slack 🙃

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

looks good and thanks for cleaning up the publicapi endpoint.
I'm curious if this fixes some of the server_test flakiness... 🤞

I hope it does!

question: when looking at the db, are we creating a separate db per test?

Yes, its been doing that for months now. And its per-testsuite and named after the package.

this is a subset of what I'm seeing when I do a list in psql:
test_handlers_internalapi_7nqy3d, test_migrate_eer4hs, test_services_fuelprice_llmu79

Yep, that's what you ought to see. But hopefully the test suite TearDown() removes this stuff.

This reverts commit 443306b.

func cloneDatabase(source, destination string) error {
// Try to obtain the lock in this method within 10 minutes
lockCtx, cancel := context.WithTimeout(context.Background(), 600*time.Second)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 4, 2019

Author Contributor

It's not clear if you see the error when you hit the cancel timeout. It feels like you get build failed instead of something useful.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

One note for myself: Can we use CREATE DATABASE targetdb WITH TEMPLATE sourcedb; instead of the psql tools? TBD.

@chrisgilmerproj chrisgilmerproj requested a review from reggieriser Oct 7, 2019
@chrisgilmerproj chrisgilmerproj merged commit 6ef6ec7 into master Oct 7, 2019
18 checks passed
18 checks passed
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_storybook_app Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: check_generated_code Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch/go 100% of diff hit (target 57.6%)
Details
codecov/project/go 57.6% (+0.1%) compared to 6952506
Details
@chrisgilmerproj chrisgilmerproj deleted the cg_db_test_teardown branch Oct 7, 2019
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.