From 85e5e706b039ad5bf5474928cbbdf516b6fcdb12 Mon Sep 17 00:00:00 2001 From: bstrausser Date: Sat, 30 Mar 2024 11:25:24 -0400 Subject: [PATCH 1/8] Fixes cockroachdb wait strategy handling --- modules/cockroachdb/cockroachdb.go | 38 +++++++++++++------------ modules/cockroachdb/cockroachdb_test.go | 17 +++++++++++ 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/modules/cockroachdb/cockroachdb.go b/modules/cockroachdb/cockroachdb.go index 14cd78292f..4c83735a54 100644 --- a/modules/cockroachdb/cockroachdb.go +++ b/modules/cockroachdb/cockroachdb.go @@ -167,24 +167,26 @@ func addWaitingFor(req *testcontainers.GenericContainerRequest, opts options) er tlsConfig = cfg } - req.WaitingFor = wait.ForAll( - wait.ForHTTP("/health").WithPort(defaultAdminPort), - wait.ForSQL(defaultSQLPort, "pgx/v5", func(host string, port nat.Port) string { - connStr := connString(opts, host, port) - if tlsConfig == nil { - return connStr - } - - // register TLS config with pgx driver - connCfg, err := pgx.ParseConfig(connStr) - if err != nil { - panic(err) - } - connCfg.TLSConfig = tlsConfig - - return stdlib.RegisterConnConfig(connCfg) - }), - ) + if req.WaitingFor == nil { + req.WaitingFor = wait.ForAll( + wait.ForHTTP("/health").WithPort(defaultAdminPort), + wait.ForSQL(defaultSQLPort, "pgx/v5", func(host string, port nat.Port) string { + connStr := connString(opts, host, port) + if tlsConfig == nil { + return connStr + } + + // register TLS config with pgx driver + connCfg, err := pgx.ParseConfig(connStr) + if err != nil { + panic(err) + } + connCfg.TLSConfig = tlsConfig + + return stdlib.RegisterConnConfig(connCfg) + }), + ) + } return nil } diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index 38d4a1388c..e3663c7e77 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -3,9 +3,11 @@ package cockroachdb_test import ( "context" "errors" + "fmt" "net/url" "strings" "testing" + "time" "github.com/jackc/pgx/v5" "github.com/stretchr/testify/require" @@ -13,6 +15,7 @@ import ( "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/modules/cockroachdb" + "github.com/testcontainers/testcontainers-go/wait" ) func TestCockroach_Insecure(t *testing.T) { @@ -144,6 +147,20 @@ func (suite *AuthNSuite) TestQuery() { suite.Equal(523123, id) } +// TestWithWaitStrategyAndDeadline is expected to fail as it covers a previous regression. +func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { + + contextDeadlineExceeded := fmt.Errorf("failed to start container: context deadline exceeded") + + ctx := context.Background() + + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) + _, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().Error(err) + suite.Require().Error(err, contextDeadlineExceeded) + +} + func conn(ctx context.Context, container *cockroachdb.CockroachDBContainer) (*pgx.Conn, error) { cfg, err := pgx.ParseConfig(container.MustConnectionString(ctx)) if err != nil { From 4663d379f34f8452a8380f365d01e6bfebdc0c75 Mon Sep 17 00:00:00 2001 From: bstrausser Date: Sat, 30 Mar 2024 11:30:25 -0400 Subject: [PATCH 2/8] Clarify test intent --- modules/cockroachdb/cockroachdb_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index e3663c7e77..527964db0a 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -147,7 +147,7 @@ func (suite *AuthNSuite) TestQuery() { suite.Equal(523123, id) } -// TestWithWaitStrategyAndDeadline is expected to fail as it covers a previous regression. +// TestWithWaitStrategyAndDeadline covers a previous regression, container creation needs to fail to cover that path. func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { contextDeadlineExceeded := fmt.Errorf("failed to start container: context deadline exceeded") @@ -156,7 +156,6 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) _, err := cockroachdb.RunContainer(ctx, suite.opts...) - suite.Require().Error(err) suite.Require().Error(err, contextDeadlineExceeded) } From c12cb33e386235253094b264641346b16b81efb7 Mon Sep 17 00:00:00 2001 From: bstrausser Date: Sat, 30 Mar 2024 12:13:04 -0400 Subject: [PATCH 3/8] Merge in default strategy to get connection --- modules/cockroachdb/cockroachdb.go | 40 ++++++++++++++----------- modules/cockroachdb/cockroachdb_test.go | 27 +++++++++++++++++ 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/modules/cockroachdb/cockroachdb.go b/modules/cockroachdb/cockroachdb.go index 4c83735a54..0658ce41c2 100644 --- a/modules/cockroachdb/cockroachdb.go +++ b/modules/cockroachdb/cockroachdb.go @@ -167,25 +167,29 @@ func addWaitingFor(req *testcontainers.GenericContainerRequest, opts options) er tlsConfig = cfg } + defaultStrategy := wait.ForAll( + wait.ForHTTP("/health").WithPort(defaultAdminPort), + wait.ForSQL(defaultSQLPort, "pgx/v5", func(host string, port nat.Port) string { + connStr := connString(opts, host, port) + if tlsConfig == nil { + return connStr + } + + // register TLS config with pgx driver + connCfg, err := pgx.ParseConfig(connStr) + if err != nil { + panic(err) + } + connCfg.TLSConfig = tlsConfig + + return stdlib.RegisterConnConfig(connCfg) + }), + ) + if req.WaitingFor == nil { - req.WaitingFor = wait.ForAll( - wait.ForHTTP("/health").WithPort(defaultAdminPort), - wait.ForSQL(defaultSQLPort, "pgx/v5", func(host string, port nat.Port) string { - connStr := connString(opts, host, port) - if tlsConfig == nil { - return connStr - } - - // register TLS config with pgx driver - connCfg, err := pgx.ParseConfig(connStr) - if err != nil { - panic(err) - } - connCfg.TLSConfig = tlsConfig - - return stdlib.RegisterConnConfig(connCfg) - }), - ) + req.WaitingFor = defaultStrategy + } else { + req.WaitingFor = wait.ForAll(req.WaitingFor, defaultStrategy) } return nil diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index 527964db0a..ba412259cd 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -154,10 +154,37 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { ctx := context.Background() + // This will never match a log statement suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) _, err := cockroachdb.RunContainer(ctx, suite.opts...) suite.Require().Error(err, contextDeadlineExceeded) + ctx = context.Background() + + // This will timeout as we didn't give enough time for intialization, but would have succeeded otherwise + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*20, wait.ForLog("node startup completed"))) + _, err = cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().Error(err, contextDeadlineExceeded) + + ctx = context.Background() + + // This will succeed + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Second*60, wait.ForLog("node startup completed"))) + container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().NoError(err) + + suite.T().Cleanup(func() { + err := container.Terminate(ctx) + suite.Require().NoError(err) + }) + + conn, err := conn(ctx, container) + suite.Require().NoError(err) + defer conn.Close(ctx) + + _, err = conn.Exec(ctx, "CREATE TABLE test (id INT PRIMARY KEY)") + suite.Require().NoError(err) + } func conn(ctx context.Context, container *cockroachdb.CockroachDBContainer) (*pgx.Conn, error) { From d131370f83bc1c0d87c269b179dace3147b31d19 Mon Sep 17 00:00:00 2001 From: bstrausser Date: Sat, 30 Mar 2024 15:19:00 -0400 Subject: [PATCH 4/8] Use subtests --- modules/cockroachdb/cockroachdb_test.go | 71 +++++++++++++++++-------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index ba412259cd..26f896a260 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -151,39 +151,64 @@ func (suite *AuthNSuite) TestQuery() { func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { contextDeadlineExceeded := fmt.Errorf("failed to start container: context deadline exceeded") + nodeStartUpCompleted := "node startup completed" + suite.Run("Expected Failure To Run", func() { - ctx := context.Background() + ctx := context.Background() - // This will never match a log statement - suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) - _, err := cockroachdb.RunContainer(ctx, suite.opts...) - suite.Require().Error(err, contextDeadlineExceeded) + // This will never match a log statement + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) + container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().Error(err, contextDeadlineExceeded) + suite.T().Cleanup(func() { + if container != nil { + err := container.Terminate(ctx) + suite.Require().NoError(err) + } + }) - ctx = context.Background() + }) - // This will timeout as we didn't give enough time for intialization, but would have succeeded otherwise - suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*20, wait.ForLog("node startup completed"))) - _, err = cockroachdb.RunContainer(ctx, suite.opts...) - suite.Require().Error(err, contextDeadlineExceeded) + suite.Run("Expected Failure To Run But Would Succeed ", func() { - ctx = context.Background() + ctx := context.Background() - // This will succeed - suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Second*60, wait.ForLog("node startup completed"))) - container, err := cockroachdb.RunContainer(ctx, suite.opts...) - suite.Require().NoError(err) + // This will timeout as we didn't give enough time for intialization, but would have succeeded otherwise + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*20, wait.ForLog(nodeStartUpCompleted))) + container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().Error(err, contextDeadlineExceeded) + suite.T().Cleanup(func() { + if container != nil { + err := container.Terminate(ctx) + suite.Require().NoError(err) + } + }) - suite.T().Cleanup(func() { - err := container.Terminate(ctx) - suite.Require().NoError(err) }) - conn, err := conn(ctx, container) - suite.Require().NoError(err) - defer conn.Close(ctx) + suite.Run("Succeeds And Executes Commands", func() { - _, err = conn.Exec(ctx, "CREATE TABLE test (id INT PRIMARY KEY)") - suite.Require().NoError(err) + ctx := context.Background() + + // This will succeed + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Second*60, wait.ForLog(nodeStartUpCompleted))) + container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().NoError(err) + + conn, err := conn(ctx, container) + suite.Require().NoError(err) + defer conn.Close(ctx) + + _, err = conn.Exec(ctx, "CREATE TABLE test (id INT PRIMARY KEY)") + suite.Require().NoError(err) + suite.T().Cleanup(func() { + if container != nil { + err := container.Terminate(ctx) + suite.Require().NoError(err) + } + }) + + }) } From 9789f92fef25c3d7621acc97c0b0e2e47c0bf448 Mon Sep 17 00:00:00 2001 From: bstrausser Date: Mon, 1 Apr 2024 21:07:31 -0400 Subject: [PATCH 5/8] Separate out Connection logic --- modules/cockroachdb/cockroachdb.go | 33 +++++++++++++------------ modules/cockroachdb/cockroachdb_test.go | 24 ++++++++++++++++++ 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/modules/cockroachdb/cockroachdb.go b/modules/cockroachdb/cockroachdb.go index 0658ce41c2..e3fe2f6df2 100644 --- a/modules/cockroachdb/cockroachdb.go +++ b/modules/cockroachdb/cockroachdb.go @@ -167,29 +167,30 @@ func addWaitingFor(req *testcontainers.GenericContainerRequest, opts options) er tlsConfig = cfg } + sqlWait := wait.ForSQL(defaultSQLPort, "pgx/v5", func(host string, port nat.Port) string { + connStr := connString(opts, host, port) + if tlsConfig == nil { + return connStr + } + + // register TLS config with pgx driver + connCfg, err := pgx.ParseConfig(connStr) + if err != nil { + panic(err) + } + connCfg.TLSConfig = tlsConfig + + return stdlib.RegisterConnConfig(connCfg) + }) defaultStrategy := wait.ForAll( wait.ForHTTP("/health").WithPort(defaultAdminPort), - wait.ForSQL(defaultSQLPort, "pgx/v5", func(host string, port nat.Port) string { - connStr := connString(opts, host, port) - if tlsConfig == nil { - return connStr - } - - // register TLS config with pgx driver - connCfg, err := pgx.ParseConfig(connStr) - if err != nil { - panic(err) - } - connCfg.TLSConfig = tlsConfig - - return stdlib.RegisterConnConfig(connCfg) - }), + sqlWait, ) if req.WaitingFor == nil { req.WaitingFor = defaultStrategy } else { - req.WaitingFor = wait.ForAll(req.WaitingFor, defaultStrategy) + req.WaitingFor = wait.ForAll(req.WaitingFor, sqlWait) } return nil diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index 26f896a260..0ccff49cd9 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -210,6 +210,30 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { }) + suite.Run("Succeeds And Executes Commands Waiting on HTTP Endpoint", func() { + + ctx := context.Background() + + // This will succeed + suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Second*60, wait.ForHTTP("/health").WithPort("8080/tcp"))) + container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().NoError(err) + + conn, err := conn(ctx, container) + suite.Require().NoError(err) + defer conn.Close(ctx) + + _, err = conn.Exec(ctx, "CREATE TABLE test (id INT PRIMARY KEY)") + suite.Require().NoError(err) + suite.T().Cleanup(func() { + if container != nil { + err := container.Terminate(ctx) + suite.Require().NoError(err) + } + }) + + }) + } func conn(ctx context.Context, container *cockroachdb.CockroachDBContainer) (*pgx.Conn, error) { From 9b663539cb927125598a4691605e8baa74a8c18b Mon Sep 17 00:00:00 2001 From: bstrausser Date: Tue, 2 Apr 2024 11:46:58 -0400 Subject: [PATCH 6/8] Feedback from PR --- modules/cockroachdb/cockroachdb_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index 0ccff49cd9..4fd85f78d2 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -149,11 +149,10 @@ func (suite *AuthNSuite) TestQuery() { // TestWithWaitStrategyAndDeadline covers a previous regression, container creation needs to fail to cover that path. func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { - contextDeadlineExceeded := fmt.Errorf("failed to start container: context deadline exceeded") nodeStartUpCompleted := "node startup completed" - suite.Run("Expected Failure To Run", func() { + suite.Run("Expected Failure To Run", func() { ctx := context.Background() // This will never match a log statement @@ -170,7 +169,6 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { }) suite.Run("Expected Failure To Run But Would Succeed ", func() { - ctx := context.Background() // This will timeout as we didn't give enough time for intialization, but would have succeeded otherwise @@ -187,7 +185,6 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { }) suite.Run("Succeeds And Executes Commands", func() { - ctx := context.Background() // This will succeed @@ -207,11 +204,9 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.Require().NoError(err) } }) - }) suite.Run("Succeeds And Executes Commands Waiting on HTTP Endpoint", func() { - ctx := context.Background() // This will succeed @@ -231,9 +226,7 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.Require().NoError(err) } }) - }) - } func conn(ctx context.Context, container *cockroachdb.CockroachDBContainer) (*pgx.Conn, error) { From 29926895216c6be2707fb0e3db0024d7394d7014 Mon Sep 17 00:00:00 2001 From: bstrausser Date: Mon, 10 Jun 2024 14:28:05 -0400 Subject: [PATCH 7/8] Honor default settings --- modules/cockroachdb/cockroachdb.go | 2 +- modules/cockroachdb/cockroachdb_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/cockroachdb/cockroachdb.go b/modules/cockroachdb/cockroachdb.go index e3fe2f6df2..503df87fd5 100644 --- a/modules/cockroachdb/cockroachdb.go +++ b/modules/cockroachdb/cockroachdb.go @@ -190,7 +190,7 @@ func addWaitingFor(req *testcontainers.GenericContainerRequest, opts options) er if req.WaitingFor == nil { req.WaitingFor = defaultStrategy } else { - req.WaitingFor = wait.ForAll(req.WaitingFor, sqlWait) + req.WaitingFor = wait.ForAll(req.WaitingFor, defaultStrategy) } return nil diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index 4fd85f78d2..e073596426 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -158,6 +158,7 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { // This will never match a log statement suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().Error(err, contextDeadlineExceeded) suite.T().Cleanup(func() { if container != nil { @@ -165,7 +166,6 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.Require().NoError(err) } }) - }) suite.Run("Expected Failure To Run But Would Succeed ", func() { @@ -174,6 +174,7 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { // This will timeout as we didn't give enough time for intialization, but would have succeeded otherwise suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*20, wait.ForLog(nodeStartUpCompleted))) container, err := cockroachdb.RunContainer(ctx, suite.opts...) + suite.Require().Error(err, contextDeadlineExceeded) suite.T().Cleanup(func() { if container != nil { @@ -181,7 +182,6 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.Require().NoError(err) } }) - }) suite.Run("Succeeds And Executes Commands", func() { From c1851a02e1e2b0bc210da2f6fc25ac1684d3bf67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20de=20la=20Pe=C3=B1a?= Date: Tue, 11 Jun 2024 09:57:09 +0200 Subject: [PATCH 8/8] fix: make lint --- modules/cockroachdb/cockroachdb_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/modules/cockroachdb/cockroachdb_test.go b/modules/cockroachdb/cockroachdb_test.go index e073596426..f332c9d013 100644 --- a/modules/cockroachdb/cockroachdb_test.go +++ b/modules/cockroachdb/cockroachdb_test.go @@ -3,7 +3,6 @@ package cockroachdb_test import ( "context" "errors" - "fmt" "net/url" "strings" "testing" @@ -149,7 +148,6 @@ func (suite *AuthNSuite) TestQuery() { // TestWithWaitStrategyAndDeadline covers a previous regression, container creation needs to fail to cover that path. func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { - contextDeadlineExceeded := fmt.Errorf("failed to start container: context deadline exceeded") nodeStartUpCompleted := "node startup completed" suite.Run("Expected Failure To Run", func() { @@ -159,7 +157,7 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*250, wait.ForLog("Won't Exist In Logs"))) container, err := cockroachdb.RunContainer(ctx, suite.opts...) - suite.Require().Error(err, contextDeadlineExceeded) + suite.Require().ErrorIs(err, context.DeadlineExceeded) suite.T().Cleanup(func() { if container != nil { err := container.Terminate(ctx) @@ -175,7 +173,7 @@ func (suite *AuthNSuite) TestWithWaitStrategyAndDeadline() { suite.opts = append(suite.opts, testcontainers.WithWaitStrategyAndDeadline(time.Millisecond*20, wait.ForLog(nodeStartUpCompleted))) container, err := cockroachdb.RunContainer(ctx, suite.opts...) - suite.Require().Error(err, contextDeadlineExceeded) + suite.Require().ErrorIs(err, context.DeadlineExceeded) suite.T().Cleanup(func() { if container != nil { err := container.Terminate(ctx)