feat: use testcontainers-go for clickhouse tests #1506
Conversation
mdelapenya
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe recent changes enhance the ClickHouse testing suite and documentation by introducing dynamic containerized testing and improving local development guidelines. A new "Running the tests" section in the README provides clear instructions for using Docker for integration tests, while the test suite has been updated to use Testcontainers. This results in a more flexible and reliable testing environment that adapts to various setups and simplifies workflow configuration. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
clickhouse/go.modis excluded by!**/*.modclickhouse/go.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (2)
- clickhouse/README.md (1 hunks)
- clickhouse/clickhouse_test.go (11 hunks)
Additional comments not posted (6)
clickhouse/README.md (2)
34-37: LGTM: Clear instructions for running tests.The "Running the tests" section clearly explains the use of Testcontainers for Go and the requirement for Docker.
Line range hint
38-48: LGTM: Practical guidance for local development.The "Local development" section offers clear instructions on setting up a Clickhouse instance using Docker, which is useful for developers.
clickhouse/clickhouse_test.go (4)
4-14: LGTM: Necessary imports for Testcontainers integration.The added imports are appropriate for the new test setup using Testcontainers and handling connection details.
16-20: LGTM: Centralized configuration constants.Defining constants for Clickhouse configuration enhances readability and maintainability.
30-55: LGTM: Dynamic connection setup with Testcontainers.The
getTestConnectionfunction now uses Testcontainers to dynamically start a Clickhouse instance, enhancing test flexibility and reliability.
Line range hint
57-187: LGTM: Updated test functions for portability.The test functions now use the dynamic connection setup, ensuring they are not dependent on specific local configurations.
|
@mdelapenya Thank you for this, I'm a huge fan of TestContainers. In the workflow yaml for Clickhouse you need to remove the ClickHouse instance we are starting else there will be two running. Is there a way to get the clickhouse version from ENV? So that it's not hardcoded, or maybe use a major semver docker mage tag. |
Ah good catch! I'm fixing that immediately.
Indeed, it's totally possible, although I see more work to do in the benchmarks workflow. Is there a reason to not include it for each storage file. My concern is about defining lots of env vars for each storage in the same step, instead of having them self-contained in the test code. Here: - name: Run Benchmarks
run: |
set -o pipefail
for d in */ ; do
[[ $d == "tls/" ]] && continue
[[ $d == "node_modules/" ]] && continue
cd "$d"
echo "Bench dir: $d"
go test ./... -benchmem -run=^$ -bench . | tee -a ../output.txt
cd ..
done
shell: bash
env:
MSSQL_DATABASE: master
MSSQL_USERNAME: sa
MSSQL_PASSWORD: MsSql!1234
MYSQL_USERNAME: username
MYSQL_PASSWORD: password
MYSQL_DATABASE: fiber
POSTGRES_DATABASE: fiber
POSTGRES_USERNAME: username
POSTGRES_PASSWORD: "pass#w%rd"Do you think it's possible to move them to each storage's file? Will it affect the existing github checks, and their usages? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/benchmark.yml (2 hunks)
- .github/workflows/test-clickhouse.yml (1 hunks)
- clickhouse/clickhouse_test.go (11 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/benchmark.yml
Files skipped from review as they are similar to previous changes (1)
- clickhouse/clickhouse_test.go
Additional comments not posted (1)
.github/workflows/test-clickhouse.yml (1)
28-29: Ensure the environment variable is utilized correctly.The
TEST_CLICKHOUSE_IMAGEenvironment variable is introduced to specify the Docker image for Clickhouse. Verify that this variable is correctly utilized within the test code to ensure that the intended Docker image is used during testing.Ensure that the Clickhouse test code uses the
TEST_CLICKHOUSE_IMAGEenvironment variable. You can verify this by searching for its usage in the codebase:Verification successful
Environment variable
TEST_CLICKHOUSE_IMAGEis correctly utilized.The
TEST_CLICKHOUSE_IMAGEenvironment variable is defined and used in theclickhouse/clickhouse_test.gofile to retrieve the Docker image name from the environment. This ensures that the intended Docker image is used during testing, aligning with the changes made in the GitHub Actions workflow.
clickhouse/clickhouse_test.go: The environment variable is defined and used to fetch the image name.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of TEST_CLICKHOUSE_IMAGE in the Clickhouse test code. # Test: Search for the environment variable usage. Expect: Occurrences where the variable is used. rg --type go 'TEST_CLICKHOUSE_IMAGE'Length of output: 225
|
@gaby can you re-review |
|
I do not see that error while running the benchmarks in my machine (MacOS M1). Any clue? |
|
@mdelapenya We were just talking about that internally. The main The |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/benchmark.yml (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/benchmark.yml
|
@mdelapenya @ReneWerner87 CI is passing now. |
|
Cool, thanks for merging this! Do you have plan for adding testcontainers to the rest of the storages? I can create a tracking issue with the rest of them, so whoever wants to create a PR just needs to be assigned to it. Wdyt? |
Yes, go ahead and create the issue. Thank you! 💪 |
|
There it goes! #1507 |
Hello from the Testcontainers for Go community 👋!
This PR is a PoC for adopting testcontainers-go as the testing library for integration testing of the different storage integrations living in this project, for which we do have Go modules representing the majority of them. Please let me know if you have any concern about adding it, I'm really happy to assist in addressing them. Of course I'd understand any result for the PR.
What does this PR do?
This PR adds testcontainers-go to the Clickhouse module, updating the function that expects a local instance running on 127.0.0.1:9001 to be present on the system. Instead, testcontainers-go will start the instance from the test sourcecode.
Why is it important?
Less setup, as developers will just clone the repository and run the tests (given Docker or another container runtime 100% compatible is installed in the machine). Therefore, the CI experience is exactly the same as running the tests locally, which improves the debuggability.
As a side note, GH actions has Docker already installed, so it should work transparently.
How to test this PR?
Just go and do like the CI does:
Follow-up
If considered positively, add more PRs for the different storages.
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
README.mdwith new sections on testing and local development, including requirements for Docker and usage of Testcontainers for integration tests.Tests