search

elastic / go-elasticsearch

The official Go client for Elasticsearch
https://github.com/elastic/go-elasticsearch#go-elasticsearch
Apache License 2.0
5.65k stars 613 forks source link

feat: use testcontainers-go for the integration tests #824

Closed mdelapenya closed 4 months ago

mdelapenya commented 6 months ago

Hi :wave: 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 containertest internal 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!

mdelapenya commented 6 months ago

That said, integration tests in the client, bulkindexer and several examples could still profit from this.

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 🙋

mdelapenya commented 5 months ago

@Anaethelion I've resolved the conflicts here. Feel free to ping me for anything you need with this PR.

Cheers!

Anaethelion commented 4 months ago

@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. I'm going to take over that PR and move things around a bit to push the integration testing in a private package in internal.

Won't change much on what has been done so far, just moving things around!