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

Restructures Go tests to use BaseTestSuite and PopTestSuite #1557

Merged
merged 7 commits into from Jan 9, 2019

Conversation

3 participants
@stangah
Copy link
Contributor

stangah commented Jan 5, 2019

Description

This PR expands on the earlier created testingsuite Go package as a place for all our Go tests to inherit useful methods from. There's two main exported structs from that package:

BaseTestSuite: Inherits from github.com/stretchr/testify/suite, adds methods that don't require db access (FatalNil, FatalNoError, etc.)
PopTestSuite: Inherits from BaseTestSuite and has access to a db, allowing methods like MustSave

The rest of this PR goes through our existing tests to standardize on this new format, using MustSave() and DB() to save and retrieve the db, respectively.

If this gets merged to master I'll go back and update docs/backend.md to point to our GoDoc documentation as a reference

@reggieriser
Copy link
Contributor

reggieriser left a comment

I like the refactoring and removing of the mustSave duplication! Ran a make server_test locally without any issues. Had a couple comments about possible further consolidation. Would like to get your thoughts on feasibility and if it's worth adding to this PR.

)

type authSuite struct {
suite.Suite
testingsuite.BaseTestSuite
logger *zap.Logger

This comment has been minimized.

@reggieriser

reggieriser Jan 7, 2019

Contributor

It looks like most of our test suites have a *zap.Logger, although we sometimes initialize it to NewDevelopment() and other times to NewNop() (not sure if that's intentional or not). Should we consider moving logger up to the BaseTestSuite?

This comment has been minimized.

@stangah

stangah Jan 7, 2019

Contributor

I wanted to leave BaseTestSuite purposefully kind of plain and single focused. I think logger is mostly used for passing into handler contexts and other child structs anyway, so I'm okay with leaving logger usage up to the test suite author

@@ -770,7 +772,7 @@ func equalSlice(a []int, b []int) bool {
}

type AwardQueueSuite struct {
suite.Suite
testingsuite.BaseTestSuite
db *pop.Connection

This comment has been minimized.

@reggieriser

reggieriser Jan 7, 2019

Contributor

I noticed that we didn't move over all test suites with DB connections to use the PopTestSuite, but maybe just the ones that previously had a mustSave? Wondering if it makes sense to have all DB-owning test suites be based on PopTestSuite in case they later need MustSave or other functions we may move there.

Also, could we do the initialization of the DB in the PopTestSuite itself as opposed to having the boilerplate code to look up the config, connect, and check errors in each test suite? It does look like we may have to pass in the config location since the path changes depending on where the test suite is located in the project hierarchy.

This comment has been minimized.

@stangah

stangah Jan 8, 2019

Contributor

Good catch, I totally missed those other suites

I'll give putting db init in PopTestSuite a shot and see how it goes

This comment has been minimized.

@stangah

stangah Jan 8, 2019

Contributor

I was able to grab the config using the testingsuite package path, give it a look: https://github.com/transcom/mymove/pull/1557/files#diff-e16d9c85a97240e0f9f8c1468288b4f8R19

This comment has been minimized.

@reggieriser

reggieriser Jan 8, 2019

Contributor

Nice!

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🚀

@reggieriser
Copy link
Contributor

reggieriser left a comment

This looks great! I know you've got some merge conflicts to resolve, but just want to give you a heads up to remove any additional mustSave functions when you do merge. Pretty sure this mustSave that I landed earlier today is new since your last merge with master:

func (suite *InvoiceServiceSuite) mustSave(model interface{}) {

:shipit:

@reggieriser

This comment has been minimized.

Copy link
Contributor

reggieriser commented Jan 8, 2019

Also, there's a Pivotal story I made for this back when we were talking about it on my PR #1520. You may want to grab/link it. https://www.pivotaltracker.com/story/show/162998650

stangah added some commits Jan 8, 2019

@stangah stangah merged commit cb183fd into master Jan 9, 2019

12 checks passed

ci/circleci: acceptance_tests 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_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp 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

@stangah stangah deleted the ps-test-suite branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment