Refactor clickhouse admin integration tests#7017
Refactor clickhouse admin integration tests#7017karencfv merged 20 commits intooxidecomputer:mainfrom
Conversation
karencfv
changed the title
andrewjstone
left a comment
andrewjstone
left a comment
There was a problem hiding this comment.
LGTM. All seems safe since we are just testing query functions for upgrade safety wrt parsing.
| 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.
| .with_log(log); | ||
|
|
||
| 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
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Thanks for the review @andrewjstone 🙇♀️
2 participants
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