BCDA-9392: Test Container Database

[laurenkrugen-navapbc]

🎫 Ticket

https://jira.cms.gov/browse/BCDA-9392

🛠 Changes

TestDatabaseContainer type that wraps golang testcontainers package. Why?

• BCDA api is using pgx, pgx pool, and sql/db for database connections; wrapper will create a connection for any of these, which eliminates the need to create a connection in each test suite/file.
• Calling NewTestDatabaseContainer ensures that the same exact container setup is called for each test, each test suite, or each file and removes the burden of configuring for the developer.
• The wrapper has methods such as ExecuteFile and ExecuteDir, which will help enforce the location and naming of testdata across the repository.
• README has been created for example and intended usage.

api_test.go used for initial tests to apply containers

• Example file/suite that was used to try the test container.
• TestJobStatusNotComplete was the primary test that was updated/tested for the containers

db/testdata directory creation

• Standard directory naming for test data.
• README created for example and intended usage

ℹ️ Context

The goal of this PR is to:

• set a standard that can be used across all tests, which will improve readability and speed up development time
• implement a solution that can be incrementally rolled out with other dev work with no disruption to current tests or CI/CD.
• reduce development burden when creating tests
• improve test reliability, since the database will not have potential state changes in between tests

Currently, a single container database is used across all tests, with some or no cleanup, depending on the tests. There is a risk that tests are not running in a "clean" state; the shared database could be altered from a previous test which can affect the accuracy and reliability of all tests.

Additionally, there is no standard of how we do database integration tests or unit tests. For database integration tests, we should have the same setup for the database every time, across all tests that will utilize the database. This will ensure that the tests are in a good state before running and their outcome will not affect other tests.

For unit tests, we should be mocking the database each time, but this PR only address integration test strategies. Because this container strategy would be implemented at whatever granularity is desired (test file, suite, sub test, etc) and does not change the current test database that is used across all tests, it will not affect our current test suite and our tests can be gradually updated over time, as developers are working in various package.

With the new changes, a test database container will be created and each time a new test runs, the database will be restored back to a good known state with a snapshot, which will ensure a clean state for each run. Testing data and additional snapshots can be created as needed, depending on the testing scenario.

🧪 Validation

Tests passin, v2/test_api.go file updated to use new test containers (arbitrarily chosen), and tests added for container.go.

[laurenkrugen-navapbc]

this is a copy of db/fixtures.sql; it's used in our reset-db makefile command. I will point that command to this file, to keep things consistent and remove the unneeded duplication.

[laurenkrugen-navapbc]

Learned something interesting about our table driven tests.

When iterating over the list of sub tests, if the Run command's func is invoked with testing.T, instead of left empty, it will not recognize the tests in the table as subtests.

// subtests not recognized
s.Run(string(tt.status), func(t *testing.T) {
...
}

// subtests recognized
s.Run(string(tt.status), func() {
...
}

[carlpartridge]

Can you go over sub tests? Is that when something is run through via a helper suite or when iterating through a range of tests? Is the goal of leaving it empty so that the suite recognizes it as a subtest and therefore runs the SetupSubTest code? If it doesnt recognize it as a subtest would it just run the SetupTest code (and therefore we wouldnt need both setup functions)?

It seems like we are doing the same thing in SetupTest and SetupSubTest. Thinking we will likely want to do these things across all test suites. Maybe we can break this off into a helper function?

[laurenkrugen-navapbc]

1. Subtests are just passed in functions to suite.Run().
2. Yup, the goal is if it's left empty, instead of *testing.T, it picks runs it as a subtest.
3. Correct - it will run the SetupTest and TeardownTest, which means the subtests will not have clean states.
4. Because the setup and teardown for tests and subtests will vary across the test suites and are required on a per suite basis, it seems like doing the test setup/teardown in those functions would make the most sense?

This also might be a good time to have a convo about separating out unit vs integration and talking about overall best practices. Because this suite has tests with subtests and some without, we have to do the setup in both vs separating out the test suite into components. We don't really have a set standard, but something for future discussion?

[carlpartridge]

Thinking this should be in a different dir. When I think of the db dir I think of db migrations, sql scripts, etc, not a go package aimed at testing. Maybe something like test_container?

[laurenkrugen-navapbc]

I put it in here because it does utilize the db migrations and sql scripts, but the primary reason was that db does not depend on any other local packages, so it would be a safe import.

I am up for putting it in another package, as long as the package does not import any other local packages.

[bhagatparwinder]

Do we need a new package? While I agree, this is testing, but it is specifically for the database. Would we ever see this being used outside of DB?

[laurenkrugen-navapbc]

If this were in it's own package, it would still need access to all the files under db/. Because we have some high level packages and this would be used in both the api and the worker, it made sense to have it here, I think.

[carlpartridge]

Can you go over sub tests? Is that when something is run through via a helper suite or when iterating through a range of tests? Is the goal of leaving it empty so that the suite recognizes it as a subtest and therefore runs the SetupSubTest code? If it doesnt recognize it as a subtest would it just run the SetupTest code (and therefore we wouldnt need both setup functions)?

It seems like we are doing the same thing in SetupTest and SetupSubTest. Thinking we will likely want to do these things across all test suites. Maybe we can break this off into a helper function?

[carlpartridge]

Related to previous comment, this isnt really a db package, but more focused on testing so I think we should acknowledge that in the package name as well.

[laurenkrugen-navapbc]

what do we think about testcontainer? or testdb?

[carlpartridge]

Package testdb in dir ./testdb directory?

[carlpartridge]

This feels fairly auto-magicky (can be good and/or excessive). How often do we actually need to seed data for tests? Is there a reason to seed data this way instead of as actual set up in a test itself? Maybe this is a unit-test vs integration test situation? In a unit-test I would expect things to be fairly explicit and narrowly focused. Whereas an integration test I could see needing to set up data in multiple tables.

I dont think I understand the difference between what feels like auto-magic seeding and the ExecuteFile/Dir helper functions. Is it just to allow you customization if need be?

[laurenkrugen-navapbc]

I have had a need to insert data for tests I was writing. Data would be setup in the actual test itself, or in the test setup func, wherever makes the most sense.

This would definitely be an integration test only situation. Golang recommends testdata at the package level, and we already implement that; several packages have a testdatafolder. Maybe this README would be better placed with the container readme, but wanted to have something for us to try and stick to.

Re: the difference between auto-magic seeding and executeFile/Dir:

1. The presence of the file in these directories doesn't actually do anything to seed data; it would have to be done with the executeFile/Dir.
   2.There's a similar magical func in databasetest that will automatically grab .yml files from testdata/ and insert directly into the database; I've historically had difficulty updating those, since it was unclear which tests they would impact if changed.
2. This proposed structure above in the README to the test data is so we can better create and maintain test data. More specifically, we can move away from shared data sets across tests, so that changes to that data won't impact more than the individual test we are creating or modifying.

[carlpartridge]

Have you done any performance testing? Just curious if there is a noticeable difference?

[laurenkrugen-navapbc]

Have you done any performance testing? Just curious if there is a noticeable difference?

I have not! I did notice that the initial image pull can take 1-2 seconds, but with the discussion we've had around separating unit vs integration tests and fixing the make docker-bootstrap command (which I have timed at ~11 minutes), this will probably still be an improvement to performance.

[laurenkrugen-navapbc]

A ticket has been created to address snyk findings; Parwinder and I have both independently done troubleshooting and there is no immediate solution to the latest finding without substantial project changes.