Introduce TestContainer interface to streamline test setup of supported databases #2053

sanyamsinghal · 2024-12-10T05:12:48Z

Describe the changes in this pull request

Any package in voyager code, for eg srcdb, tgtdb, connpool, yugabyted etc.. can spin up the required source db(oracle, pg, mysql) with a specific version to write unit tests.
Every call to TestContainer.Start() check if a required database(dbtype + version) is already run then reuse it, otherwise start a new one
In every package add a TestMain() function, to setup the environment needed specifically for that test.
Added a new Github actions workflow to run the integration tests separately(required some oracle instance client libraries).
Moving forward it will be good to have separation between go tests as unit or integration.

Challenges/Limitations:

Currently we are not calling TestContainer.TerminateAllContainers(), right now all the containers gets terminated at the end, but we should also call that. Challenge was i couldn't find the right point in code where i can call that.
In Golang, for each packages separate new process is spawned to run its tests, hence the containerRegistry is not shared across all the packages due to separate processes, hence with the current approach it is not possible to reuse the same containers across all packages but across tests in a single package.

One solution is maintain the containerRegistry info in some json file(act as a registry) which can be reused by all processes

Describe if there are any user-facing changes

NA

How was this pull request tested?

All the existing unit tests are working as expected.

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	No
Name registry json	No
Data File Descriptor Json	No
Export Snapshot Status Json	No
Import Data State	No
Export Status Json	No
Data .sql files of tables	No
Export and import data queue	No
Schema Dump	No
AssessmentDB	No
Sizing DB	No
Migration Assessment Report Json	No
Callhome Json	No
YugabyteD Tables	No
TargetDB Metadata Tables	No

makalaaneesh

Thanks for this! left a few comments

makalaaneesh · 2024-12-11T05:07:48Z

yb-voyager/src/srcdb/main_test.go

+	if err != nil {
+		utils.ErrExit("Failed to connect to mysql database: %w", err)
+	}
+	defer testMySQLSource.DB().Disconnect()


nit: since you're only connecting to test that the connections are working, you don't really need the "defer", you can disconnect at this point itself.

As far as i understand, the Connect() function also initialises the sql.DB in each case, which later on provides the connection used by functions like GetAllTableNamesRaw. Running Disconnect() would terminate that sql connection pool?

Hmm, good point. How is it working now then for each test that uses the test containers?

How is it working now then for each test that uses the test containers?

didn't get you exactly, i think all the tests(srcdb or tgtdb packages) i have added is following this.
Are there any existing tests which don't follow this but still works?

My understanding is that TestMain runs before any other test in the package.
And if TestMain has a defer to disconnect the DB, then how is it that in TestPostgresGetAllTableNames, you are able to use that container to execute SQLs? It should have been disconnected by then, right?

@makalaaneesh in TestMain we have to call m.Run() which executes all the tests of that package.
So here defer will be invoked after all the tests for that package has run.
like you can i added terminateAllContainers after that.`

I will add one comment in the code for this.

makalaaneesh · 2024-12-11T05:12:50Z

yb-voyager/src/srcdb/mysql_test.go

+
+	// Test GetAllTableNames
+	actualTables := testMySQLSource.DB().GetAllTableNames()
+	expectedTables := []*sqlname.SourceName{


Reading this test-case in isolation, it's confusing where these tables come from. I would be in favour of NOT having a default schema that is assumed with testcontainer startup. Don't really see a point of it.
I think it would be better for each testcase to be self-contained. clearly set up it's schema objects at the beginning of the test, run its tests, delete/drop the schema objects.

Another reason to avoid a single common schema is that over time, we will end up writing all the DDLs for our tests into that schema file itself, which beats the point of having self-container "unit" tests.

That init schema file can be used for various purposes like setting up something in source db, maybe creating user with specific privileges or anything. So i think we should have make use of it since it good to have the cluster/db precreated with those.

Regarding the schema/objects to be setup for each test separately.
I was thinking to do that but it was increasing the work significantly.
Maybe we can do that in separate PR or this PR would have to wait...

Done @makalaaneesh

Nice! ExecuteSqls is nice, and using it with defer for cleanup is very useful

yb-voyager/test/containers/testcontainers.go

CLAassistant · 2024-12-16T08:42:03Z

All committers have signed the CLA.

…ke sequences and pk constraint objects - fixed to return only table names

…ross all go packages in codebase

…kage - testcontainers interface and its implementations follows singleton pattern - implemented container registry to keep track of that and also added TerminateAllContainers() function using the registry - Using go:embed for storing the content of init schema files, and using the variable to make Start() load init file irrespective of wher the function is called from, in the project

…alised testcontainers package

This reverts commit 605e3a6.

…cript

…tead of depending on a common global init sql script

.github/workflows/issue-tests.yml

makalaaneesh

LGTM!

.github/workflows/integration-tests.yml

priyanshi-yb · 2024-12-23T13:50:35Z

yb-voyager/src/srcdb/postgres.go

@@ -940,6 +940,7 @@ var PG_QUERY_TO_CHECK_IF_TABLE_HAS_PK = `SELECT nspname AS schema_name, relname
 FROM pg_class c
 LEFT JOIN pg_namespace n ON n.oid = c.relnamespace
 LEFT JOIN pg_constraint con ON con.conrelid = c.oid AND con.contype = 'p'
+WHERE c.relkind = 'r'  -- Only consider ordinary tables


why do we need to do this, we should report partitioned tables not having pk as well right?

updated to have p and r both. Previously it was also considering objects like sequences since they also don't have PK on them.

sanyamsinghal self-assigned this Dec 10, 2024

makalaaneesh reviewed Dec 11, 2024

View reviewed changes

sanyamsinghal force-pushed the sanyam/integ-tests branch 2 times, most recently from fb5079b to 50ee8b2 Compare December 18, 2024 16:39

sanyamsinghal marked this pull request as ready for review December 18, 2024 16:55

sanyamsinghal requested review from makalaaneesh, hbhanawat, ShivanshGahlot, shubham-yb and priyanshi-yb December 18, 2024 16:55

Sanyam Singhal and others added 19 commits December 20, 2024 18:42

debugging the bug

1385612

Implement integration testing framework for PostgreSQL source

0257867

Extended integration testing framework for Oracle and MySQL database

73e0aca

Bug fix: GetNonPKTables() for PG and YB returned non table objects li…

b60ddc6

…ke sequences and pk constraint objects - fixed to return only table names

Extended integration testing framework for YugabyteDB source database

e2b9406

Setting log level during test execution

eff5288

Embed Source struct in TestDB struct for direct access of fields

21158a8

Implment integration test framework for tgtdb package

16bd3e2

Replaced embedded postgres with testcontainers postgres

440ee30

Added PostgreSQL tests

cb22247

temp commit for ExportData() test PG

e8d8fda

intermediate state: implementing a testscontainers packages usable ac…

6374969

…ross all go packages in codebase

code cleanup

7aa0d00

Intermediate fix: Remove old testcontainers package and use new centr…

40d351b

…alised testcontainers package

Refactored tgtdb package code to use testcontainers library

e80d858

code cleanup, refactoring missses

83a0e69

fixed saving of container in registry

1c383bc

Revert "debugging the bug"

f1ed32f

This reverts commit 605e3a6.

sanyamsinghal added 4 commits December 20, 2024 18:42

fixed nil map issue

ccd5b7c

removed old TODO

b7c2beb

minor change

f35e2a3

Introduced build tags for running integration tests separately

adcdaf5

sanyamsinghal force-pushed the sanyam/integ-tests branch from 7db3222 to adcdaf5 Compare December 20, 2024 13:33

sanyamsinghal added 12 commits December 20, 2024 19:09

Adding java17 in integration GHA workflow

37b4127

Fixed go test command for integration tests

e7d9536

Skipping TestDDLIssuesInYBVersion test in integration tests

7aeb942

Install Oracle Instant Clients instead of running voyager installer s…

d75762d

…cript

Implement ExecuteSqls() for each test container type

788ea74

Using ExecuteSqls() to setup schema/objects specific to each test ins…

5e6292c

…tead of depending on a common global init sql script

printing logs of all containers

01eaf14

temp commit for debugging

a0a7bda

increased timeout for the container startup time

40c9f79

Terminating containers in each package

6a1de32

Added ExecuteSqls() for mysql tests also

808f466

Updated startup time for mysql container

71009f9

makalaaneesh reviewed Dec 23, 2024

View reviewed changes

.github/workflows/issue-tests.yml Outdated Show resolved Hide resolved

skip integration test in issue-tests workflow

9981fb8

sanyamsinghal requested a review from makalaaneesh December 23, 2024 12:47

makalaaneesh approved these changes Dec 23, 2024

View reviewed changes

priyanshi-yb reviewed Dec 23, 2024

View reviewed changes

update the get non pk logic for PG

1e577af

sanyamsinghal merged commit 43aaeeb into main Dec 24, 2024
43 checks passed

sanyamsinghal deleted the sanyam/integ-tests branch December 24, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce TestContainer interface to streamline test setup of supported databases #2053

Introduce TestContainer interface to streamline test setup of supported databases #2053

sanyamsinghal commented Dec 10, 2024 •

edited

Loading

makalaaneesh left a comment

makalaaneesh Dec 11, 2024

sanyamsinghal Dec 16, 2024

makalaaneesh Dec 23, 2024

sanyamsinghal Dec 23, 2024

makalaaneesh Dec 23, 2024

sanyamsinghal Dec 23, 2024

makalaaneesh Dec 11, 2024

sanyamsinghal Dec 16, 2024

sanyamsinghal Dec 23, 2024

makalaaneesh Dec 23, 2024

CLAassistant commented Dec 16, 2024 •

edited

Loading

makalaaneesh left a comment

priyanshi-yb Dec 23, 2024

sanyamsinghal Dec 24, 2024

Introduce TestContainer interface to streamline test setup of supported databases #2053

Introduce TestContainer interface to streamline test setup of supported databases #2053

Conversation

sanyamsinghal commented Dec 10, 2024 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

makalaaneesh 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

Choose a reason for hiding this comment

CLAassistant commented Dec 16, 2024 • edited Loading

makalaaneesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanyamsinghal commented Dec 10, 2024 •

edited

Loading

CLAassistant commented Dec 16, 2024 •

edited

Loading