feat: use testcontainers-go for the integration tests #824
Conversation
There was a problem hiding this comment.
Hi @mdelapenya glad to see you here!
Thank you for the great write-up about testcontainers. I've been meaning to use it for some time, this is the perfect occasion to do that.
I agree that integration tests are a good fit for a first step!
However the buildkite shell script is here to stay as not all clients have access to a testcontainers implementation and this is the cornerstone of our mutual compatibility within the team.
That said, integration tests in the client, bulkindexer and several examples could still profit from this.
I've left a question on which we can iterate other than that it looks good!
|
|
||
| // SetupElasticsearch starts an Elasticsearch container and sets the environment variables | ||
| // ELASTICSEARCH_URL, ELASTICSEARCH_USERNAME, and ELASTICSEARCH_PASSWORD in the test context. | ||
| func SetupElasticsearch(t *testing.T) (*tces.ElasticsearchContainer, error) { |
There was a problem hiding this comment.
I would wrap this in a structure which could both allow to stop the container once the test has ended and would return the base config.
This would avoid recasting the config for each requests and would allow to reuse that in esutil and _examples.
wdyt ?
There was a problem hiding this comment.
Sounds good to me. I tried to address it in the latest commits
Yeah, sure. I do not have the knowledge of the codebase to know where testcontainers-go fits the best, so whatever you see, fine for me 🙋 |
* main: Bump golang.org/x/text from 0.3.2 to 0.3.8 in /_examples/bulk/kafka (elastic#828) Bump to go1.21 (elastic#827)
* main: adding changelog for 8.13.1 (elastic#847) Add put_synonym_rule fix (elastic#843) add PoolCompressor option and pass it to elastictransport.Config (elastic#840) adding changelog for 8.13.0 (elastic#838) Bump elastictransport to v8.5.0 (elastic#835) Fixes elastic#830 (elastic#833) Update apis for 8.14 (elastic#831)
|
@Anaethelion I've resolved the conflicts here. Feel free to ping me for anything you need with this PR. Cheers! |
|
@mdelapenya Sorry this has been put on the back-burner. Picking this up I've ultimately decided that I don't want users to tract the test dependencies by simply using the lib. Won't change much on what has been done so far, just moving things around! |
There was a problem hiding this comment.
Thank you! 🚀
For the snyk related error: this will be augmented by #855
* feat: use testcontainers-go for the integration tests * chore: bump to latest release * chore: wrap the ES container into a struct
* feat: use testcontainers-go for the integration tests (#824) * feat: use testcontainers-go for the integration tests * chore: bump to latest release * chore: wrap the ES container into a struct * [Testcontainers] move integration test to internal (#855) * feat: use testcontainers-go for the integration tests * chore: bump to latest release * chore: wrap the ES container into a struct * adapt github action to testcontainers * use the same container for all the integration subtests * use testcontainers for esapi integration tests * use testcontainers for esutil integration tests * update testcontainers-go dependency to main branch as a wip * move custom transport test and insecure default to insecure integration part * update to latest testcontainers-go 0.31.0 * reorder imports * move integration test to internal package with testcontainers * adapt makefile to use the new internal/testing package * fix bad version transport regression * update examples dependencies * reinstate server launch for example testing --------- Co-authored-by: Manuel de la Peña <mdelapenya@gmail.com> --------- Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com> Co-authored-by: Manuel de la Peña <mdelapenya@gmail.com>
Hi 👋 from Testcontainers Go community! I'm sending this PR as way to improve the developer experience running the tests in this project, using testcontainers-go.
Of course, it's up to the maintainers whether to include it or not, and I'm happy with any of them, although I'd like to see it included for the reasons explained in the
Why is it important?of this PR.What does this PR do?
This PR adds testcontainers-go as a means to start Elasticsearch containers while running the integration tests.
I'm only adding it to one test function to demonstrate the benefits of declaring the dependencies in the code, compared to use a big set of shell scripts to start the test-time dependencies.
The
containertestinternal package could be used across different test files containing integration tests, and won't be included in any final binary if not imported from any non-test file.Why is it important?
It will help contributors to run the integration tests with more ease: just cloning the repo and running
make test-integ. The same would apply to the CI.The code is also self-contained, which means that there is no need to start containers in the CI (Buildkite) in a way that developers need to learn. Here, the dependencies are declared programatically next to the tests, so it's easier to maintain.
Besides that, the plumbing scripts will get reduced, as everything will be done in code, which is easier to test.
Other considerations
I did not configured the Elasticsearch instance as described in the CI shell scripts, and it was on purpose, to showcase how it's possible to start the ES instance with a few lines of code. All the configurations in there can be added to the module thanks to the module configuration API. Please see https://golang.testcontainers.org/modules/elasticsearch/ for more information about the module.
I'm not removing any shell script code (yet), as I'm not that familiar with the codebase, but this PR would be an initial step of a few of them. For that, I'd thank any feedback from your review.
Cheers!