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

CASSANDRA-19978 Testcontainers integration #1780

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

ribaraka
Copy link

@ribaraka ribaraka commented Jul 15, 2024

Closes #1686

In this PR, the CCM integration test runner has been replaced with the testcontainers-go library.

I utilized a main_test.go file to provide a centralized way to manage test setup and teardown logic. This approach initializes shared resources before any tests are run and cleans them up afterward, ensuring a consistent and efficient testing environment. The main_test.go file serves as the entry point for the integration tests and uses tags to omit building a cluster for unit tests.

Additionally, a bash script was added to dynamically change Cassandra's config file on the fly for a specific Cassandra version. I aimed to maintain consistency in the code so the configuration would look familiar.

Now, the GitHub Actions workflow uses the new testcontainers-go integration test runner instead of CCM. The Go version has been bumped up to 1.23 to ensure compatibility with the testcontainers-go package.

Added documentation on how to run an integration test runner locally.

Fixed flaky test. Closes #1795

I also took the liberty of removing all CCM and related components

@ribaraka ribaraka changed the title Testcontainers integration [WIP]Testcontainers integration Jul 15, 2024
@ribaraka ribaraka changed the title [WIP]Testcontainers integration Testcontainers integration Jul 31, 2024
@ribaraka ribaraka mentioned this pull request Aug 12, 2024
@ribaraka ribaraka force-pushed the testcontainers branch 2 times, most recently from 7667070 to 4797f5a Compare September 4, 2024 16:14
go.mod Outdated
github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed
github.com/kr/pretty v0.1.0 // indirect
github.com/stretchr/testify v1.3.0 // indirect
github.com/testcontainers/testcontainers-go v0.32.0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to use the latest v0.33.0. just in case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upgraded to v0.33.0

main_test.go Outdated
Name: "cassandra",
},
}
cassandraNetwork, err := testcontainers.GenericNetwork(ctx, networkRequest)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GenericNetwork is deprecated. I'd use network.New instead. Please see https://golang.testcontainers.org/features/creating_networks/

Copy link
Author

@ribaraka ribaraka Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fixing the flacke integration tests, and the whole setup has been refactored, including the part you've suggested. I hope you will be able to review it as well, once it's pushed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A commit fixing the flaky tests has already been pushed, and all integration replacements have been completed. Please take a look

main_test.go Outdated
defer cassandraNetwork.Remove(ctx)

// Function to create a Cassandra container (node)
createCassandraContainer := func(number int) (string, error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a Cassandra expert, so you probably want to take a look and customise the existing cassandra Go module with your code: https://golang.testcontainers.org/modules/cassandra/ (even contributing to the module with updates would be great!)

Copy link
Author

@ribaraka ribaraka Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version of gocql is 1.13, and the decision on which version to upgrade to hasn't been finalized yet. For now, I’m keeping things simple by not using the modules.

Initially, I started with the cass module, but I didn't find it handy. Managing an additional import also felt unnecessary since the generic structs provided most of the required functionality.

Once the Go version is decided, I plan to revise the Cassandra model and see what I can do to enhance the functionality that I might have been missing and make it handy 😄

main_test.go Outdated
Comment on lines 84 to 100
LifecycleHooks: []testcontainers.ContainerLifecycleHooks{{
PostStarts: []testcontainers.ContainerHook{
func(ctx context.Context, c testcontainers.Container) error {
// wait for cassandra config.yaml to initialize
time.Sleep(100 * time.Millisecond)

code, _, err := c.Exec(ctx, []string{"bash", "./update_container_cass_config.sh"})
if err != nil {
return err
}
if code != 0 {
return fmt.Errorf("script ./update_container_cass_config.sh exited with code %d", code)
}
return nil
},
},
}},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have a wait.ForAll strategy in which you add the wait for log below, and a wait.ForExec https://golang.testcontainers.org/features/wait/exec/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually could be a nice way to handle it. Thank you! I'll try it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, your advice didn’t resolve the issue. Waiting for any logs didn’t yield useful results, as I was attempting to change certain Cassandra configuration properties on the fly. These properties can also vary depending on the Cassandra version in use.

My solution using a hook also had flaws, as the hook couldn’t pause the bootstrapping process to allow the execution of custom code at the necessary point. As a result, my custom script couldn’t be executed in time.

To address this, I made some adjustments to the docker-entrypoint, which successfully handled the required tasks during the initialization process.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback, I'm a total newbie in Cassandra and you better than me know how ti start it 😊 In the current Cassandra module we do this: https://github.com/testcontainers/testcontainers-go/blob/main/modules/cassandra/cassandra.go#L97, although looking at this PR, it seems it's creating a rock-solid implementation for cassandra. Would you like to, at some point, contribute to the module? 🙏 Then the entire community would benefit from your changes. Of course, only if you want to.

@mdelapenya
Copy link

Hi @ribaraka, this is Manu, testcontainers-go maintainer! 👋 Thanks for adding the library to this project! My colleague told me about this PR so I took the chance to add a few suggestions in case you want to enhance the experience of using the library. Please let me know your thoughts about them, thanks!

@ribaraka
Copy link
Author

ribaraka commented Sep 5, 2024

Hi @mdelapenya!
Thank you so much for taking the time to review my PR and for your suggestions. They’re quite interesting, I’ll definitely use some of them. 😊

@ribaraka
Copy link
Author

ribaraka commented Sep 12, 2024

added fix for flaky tests to this PR.

Closes #1795

@Zariel, could you please review it?

@ribaraka
Copy link
Author

@martin-sucha, after integrating the new test runner for the driver and fixing all tests marked as a flake, I ran coverage tests for all existing unit and integration tests, merged the results into one report, and the current test coverage stands at 76%.

I can send you the coverage report via Slack or attach it here. Alternatively, you can fetch this branch and generate it yourself. Here’s how I do it

// generate a report for all integration tests
go test -v -covermode=atomic -coverprofile=coverage_integration.out -tags "integration cassandra tc gocql_debug" -timeout=30m -gocql.timeout=60s -runssl -proto=4 -rf=3 -clusterSize=3 -autowait=2000ms -compressor=snappy -gocql.cversion=5.0.0  ./...

// generate a report for auth test
go test -v -covermode=atomic -coverprofile=coverage_auth.out -run=TestAuthentication -tags ""integration" gocql_debug" -timeout=2m -runauth -gocql.timeout=60s -runssl -proto=4 -rf=3 -clusterSize=1 -autowait=2000ms -runauth -compressor=snappy -gocql.cversion=5.0.0

// generate a report for all unit tests
 go test -v -covermode=atomic -coverprofile=coverage_unit.out -tags unit -race
 
 
 // once all reports have been generated, use the gocovmerge tool to run the merge command and retrieve the complete report for the entire driver.
  gocovmerge coverage_auth.out coverage_integration.out coverage_unit.out > coverage_merged.out

// view the merged coverage report
go tool cover -func=coverage_merged.out

Copy link

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Testcontainers standpoint, LGTM

As a follow-up, I'd like to work with you in using the cassandra tc-go module, or contributing to it to receive the enhancements in this PR.

@joao-r-reis joao-r-reis changed the title Testcontainers integration CASSANDRA-19978 Testcontainers integration Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix flaky tests Rewrite integration test setup in Go
2 participants