Refactor clickhouse admin integration tests #7017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karencfv
changed the title
andrewjstone
approved these changes
Nov 13, 2024
| let config = DeploymentConfig { | ||
| path, | ||
| base_ports, | ||
| cluster_name: "oximeter_cluster".to_string(), |
There was a problem hiding this comment.
I think we have a constant defined for oximeter_cluster somewhere, although maybe not accessible to clickhouse-admin.
|
|
||
| let lgif = clickhouse_cli.lgif().await.unwrap(); | ||
| let lgif = clickhouse_cli.lgif().await?; |
There was a problem hiding this comment.
It's totally fine to use ? here, but using unwrap will give you the proper line number for the failure, which may help debugging later.
There was a problem hiding this comment.
Huh, I wonder why I changed this. I didn't change the .unwrap() for a ? in any of the other tests 😄
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
There was a problem hiding this comment.
Maybe add a doc comment here about how this should only be used for testing that doesn't write data to clickhouse, as that could make tests flaky.
karencfv
commented
Nov 13, 2024
Merged
iliana
added a commit
that referenced
this pull request
Nov 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This commit changes the way the clickhouse clusters are spun up for clickhouse-admin integration tests. Instead of multiple clusters being spun up for every test, now we start up a single cluster via nextest's "setup scripts" functionality.
This way, we no longer need to manually hard code so many ports per test.
Caveats
Sadly, we still have to hardcode some ports, but at least this way it's only a set of ports, and we won't need to add more every time we want to test something new.
In order to get the teardown function working, I had to limit the tests to run on a single thread. They're a bit slower than before, but nothing too terrible. Setting up the cluster, running the tests and tearing down takes ~7s. The time to run the tests themselves is ~3s