-
Notifications
You must be signed in to change notification settings - Fork 622
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
base: trunk
Are you sure you want to change the base?
Conversation
7667070
to
4797f5a
Compare
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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
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 | ||
}, | ||
}, | ||
}}, |
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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! |
Hi @mdelapenya! |
bc39d7f
to
c9e43bd
Compare
696af25
to
12ba1c5
Compare
@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
|
There was a problem hiding this 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.
12ba1c5
to
737f415
Compare
a9e7e8a
to
9c34737
Compare
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. Themain_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