Skip to content

Commit

Permalink
query-tests-setup: lift constraints on ConnectorTag (#4159)
Browse files Browse the repository at this point in the history
In the effort to run connector-test-kit-rs with the new node-based
drivers, we want to dispatch requests to an external process when
running the query engine test suite. This is naturally implemented with
a new type that shells out to that external process to implement
`ConnectorTagInterface`.

Since we do not want to start a new process every time
`ConnectorTag::try_from()` is called (at least once per test) and we are
going to want asynchronous communication with the child process
(required by some tests), we have to change the entry points that
construct `ConnectorTag`s. We will need either a static method on
`ConnectorTagInterface` (forbidden by enum_dispatch) or separate
connector instantiation (sync, from the tag strings) from initialization
(async, will involve IO).

Changes in this PR
==================

The first entry point is the `TryFrom<(&str, Option<&str>)>` impl for
`ConnectorTag`. It is replaced by a `TryFrom<(&str, Option<&str>)>` impl
for `ConnectorVersion`. There is no longer a shared responsibility:
`ConnectorVersion` is the exhaustive view of the connectors to test that
the test suite knows about, and `ConnectorTagInterface` is a bag of
connector-specific functions. This lets us remove the `enum_dispatch`
dependency, which helps because it was another thing preventing defining
static methods on `ConnectorTagInterface`.

The second entrypoint is `ConnectorTag::all()`, which turned out to be
dead code, so this commit removes it.

If you look at the diff, these two changes add up to a nice amount of
deleted code in query-tests-setup, and more importantly, a less
constraining interface to implement for a connector tag that will need
async initialization.
  • Loading branch information
tomhoule authored Aug 23, 2023
1 parent 3627f6a commit 36f92fe
Show file tree
Hide file tree
Showing 28 changed files with 397 additions and 733 deletions.
13 changes: 0 additions & 13 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ macro_rules! match_connector_result {

let connector = $runner.connector_version();

let mut results = match connector {
let mut results = match &connector {
$(
$( $matcher )|+ $( if $pred )? => $result
),*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ mod interactive_tx {

let res = runner.commit_tx(tx_id.clone()).await?;

if matches!(runner.connector(), ConnectorTag::MongoDb(_)) {
if matches!(runner.connector_version(), ConnectorVersion::MongoDb(_)) {
assert!(res.is_err());
let err = res.err().unwrap();
let known_err = err.as_known().unwrap();
Expand All @@ -248,7 +248,7 @@ mod interactive_tx {
&runner,
"query { findManyTestModel { id }}",
// Postgres and Mongo abort transactions, data is lost.
Postgres(_) | MongoDb(_) | CockroachDb => vec![r#"{"data":{"findManyTestModel":[]}}"#],
Postgres(_) | MongoDb(_) | CockroachDb(_) => vec![r#"{"data":{"findManyTestModel":[]}}"#],
// Partial data still there because a batch will not be auto-rolled back by other connectors.
_ => vec![r#"{"data":{"findManyTestModel":[{"id":1},{"id":2}]}}"#]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ mod metrics {
Sqlite => assert_eq!(total_queries, 9),
SqlServer(_) => assert_eq!(total_queries, 17),
MongoDb(_) => assert_eq!(total_queries, 5),
CockroachDb => (), // not deterministic
CockroachDb(_) => (), // not deterministic
MySql(_) => assert_eq!(total_queries, 12),
Vitess(_) => assert_eq!(total_queries, 11),
Postgres(_) => assert_eq!(total_queries, 7),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ mod occ {

// MongoDB is different here and seems to only do one create with all the upserts
// where as all the sql databases will do one create and one upsert
let expected = if matches!(runner.connector(), ConnectorTag::MongoDb(_)) {
let expected = if matches!(runner.connector_version(), ConnectorVersion::MongoDb(_)) {
serde_json::json!({
"data": {
"findFirstResource": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

use indoc::indoc;
use query_engine_tests::{
query_core::TxId, render_test_datamodel, setup_metrics, test_tracing_subscriber, ConnectorTag, LogEmit,
QueryResult, Runner, TestError, TestLogCapture, TestResult, TryFrom, WithSubscriber, CONFIG, ENV_LOG_LEVEL,
query_core::TxId, render_test_datamodel, setup_metrics, test_tracing_subscriber, LogEmit, QueryResult, Runner,
TestError, TestLogCapture, TestResult, WithSubscriber, CONFIG, ENV_LOG_LEVEL,
};
use std::future::Future;
use tokio::sync::mpsc;
Expand Down Expand Up @@ -61,8 +61,7 @@ impl Actor {

let (query_sender, mut query_receiver) = mpsc::channel(100);
let (response_sender, response_receiver) = mpsc::channel(100);

let tag = ConnectorTag::try_from(("sqlserver", None))?;
let (tag, version) = query_tests_setup::CONFIG.test_connector()?;

let datamodel = render_test_datamodel(
"sql_server_deadlocks_test",
Expand All @@ -73,7 +72,7 @@ impl Actor {
Some("READ COMMITTED"),
);

let mut runner = Runner::load(datamodel, &[], tag, setup_metrics(), log_capture).await?;
let mut runner = Runner::load(datamodel, &[], version, tag, setup_metrics(), log_capture).await?;

tokio::spawn(async move {
while let Some(message) = query_receiver.recv().await {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ mod json_filter {

fn json_path(runner: &Runner) -> &'static str {
match runner.connector_version() {
ConnectorVersion::Postgres(_) | ConnectorVersion::CockroachDb => r#"path: ["a", "b"]"#,
ConnectorVersion::Postgres(_) | ConnectorVersion::CockroachDb(_) => r#"path: ["a", "b"]"#,
ConnectorVersion::MySql(_) => r#"path: "$.a.b""#,
x => unreachable!("JSON filtering is not supported on {:?}", x),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ mod insensitive {
r#"query { findManyTestModel(where: { str: { gte: "aÆB", mode: insensitive } }) { str }}"#,
MongoDb(_) => vec![r#"{"data":{"findManyTestModel":[{"str":"æ"},{"str":"Æ"},{"str":"bar"},{"str":"aÆB"},{"str":"AÆB"},{"str":"aæB"}]}}"#],
// Cockroach, https://github.com/cockroachdb/cockroach/issues/71313
CockroachDb => vec![r#"{"data":{"findManyTestModel":[{"str":"æ"},{"str":"Æ"},{"str":"bar"},{"str":"aÆB"},{"str":"AÆB"},{"str":"aæB"}]}}"#],
CockroachDb(_) => vec![r#"{"data":{"findManyTestModel":[{"str":"æ"},{"str":"Æ"},{"str":"bar"},{"str":"aÆB"},{"str":"AÆB"},{"str":"aæB"}]}}"#],
_ => vec![r#"{"data":{"findManyTestModel":[{"str":"æ"},{"str":"Æ"},{"str":"bar"},{"str":"aÆB"},{"str":"AÆB"},{"str":"aæB"},{"str":"aB"}]}}"#]
);

Expand All @@ -154,7 +154,7 @@ mod insensitive {
r#"query { findManyTestModel(where: { str: { lt: "aÆB", mode: insensitive } }) { str }}"#,
MongoDb(_) => vec![r#"{"data":{"findManyTestModel":[{"str":"A"},{"str":"aB"}]}}"#],
// https://github.com/cockroachdb/cockroach/issues/71313
CockroachDb => vec![r#"{"data":{"findManyTestModel":[{"str":"A"},{"str":"aB"}]}}"#],
CockroachDb(_) => vec![r#"{"data":{"findManyTestModel":[{"str":"A"},{"str":"aB"}]}}"#],
_ => vec![r#"{"data":{"findManyTestModel":[{"str":"A"}]}}"#]
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use query_engine_tests::*;
#[test_suite(schema(schemas::json), capabilities(JsonFiltering), exclude(MySql(5.6)))]
mod json_filters {
use indoc::indoc;
use query_engine_tests::{assert_error, is_one_of, run_query, MySqlVersion, Runner};
use query_engine_tests::{assert_error, is_one_of, run_query, Runner};

fn pg_json() -> String {
let schema = indoc! {
Expand Down Expand Up @@ -895,7 +895,7 @@ mod json_filters {

fn json_path(runner: &Runner) -> &'static str {
match runner.connector_version() {
ConnectorVersion::Postgres(_) | ConnectorVersion::CockroachDb => r#"path: ["a", "b"]"#,
ConnectorVersion::Postgres(_) | ConnectorVersion::CockroachDb(_) => r#"path: ["a", "b"]"#,
ConnectorVersion::MySql(_) => r#"path: "$.a.b""#,
x => unreachable!("JSON filtering is not supported on {:?}", x),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ mod order_by_dependent {
}
}"#,
MongoDb(_) | Sqlite => vec![r#"{"data":{"findManyModelA":[{"id":3,"b":null},{"id":4,"b":null},{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":2,"b":{"c":{"a":{"id":4}}}}]}}"#],
MySql(_) | CockroachDb => vec![
MySql(_) | CockroachDb(_) => vec![
r#"{"data":{"findManyModelA":[{"id":4,"b":null},{"id":3,"b":null},{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":2,"b":{"c":{"a":{"id":4}}}}]}}"#,
r#"{"data":{"findManyModelA":[{"id":3,"b":null},{"id":4,"b":null},{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":2,"b":{"c":{"a":{"id":4}}}}]}}"#,
],
Expand Down Expand Up @@ -276,7 +276,7 @@ mod order_by_dependent {
}
}"#,
MongoDb(_) | Sqlite => vec![r#"{"data":{"findManyModelA":[{"id":2,"b":{"c":{"a":{"id":4}}}},{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":3,"b":null},{"id":4,"b":null}]}}"#],
MySql(_) | CockroachDb => vec![
MySql(_) | CockroachDb(_) => vec![
r#"{"data":{"findManyModelA":[{"id":2,"b":{"c":{"a":{"id":4}}}},{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":4,"b":null},{"id":3,"b":null}]}}"#,
r#"{"data":{"findManyModelA":[{"id":2,"b":{"c":{"a":{"id":4}}}},{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":3,"b":null},{"id":4,"b":null}]}}"#,
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ mod order_by_dependent_pag {
}
}"#,
// Depends on how null values are handled.
MongoDb(_) | Sqlite | MySql(_) | CockroachDb => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"id":1}},{"id":2,"b":{"id":2}}]}}"#],
MongoDb(_) | Sqlite | MySql(_) | CockroachDb(_) => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"id":1}},{"id":2,"b":{"id":2}}]}}"#],
_ => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"id":1}},{"id":2,"b":{"id":2}},{"id":3,"b":null}]}}"#]
);

Expand Down Expand Up @@ -166,7 +166,7 @@ mod order_by_dependent_pag {
}
}"#,
// Depends on how null values are handled.
MongoDb(_) | Sqlite | MySql(_) | CockroachDb => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"c":{"id":1}}}]}}"#],
MongoDb(_) | Sqlite | MySql(_) | CockroachDb(_) => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"c":{"id":1}}}]}}"#],
_ => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"c":{"id":1}}},{"id":2,"b":{"c":null}},{"id":3,"b":null}]}}"#]
);

Expand Down Expand Up @@ -248,7 +248,7 @@ mod order_by_dependent_pag {
}
}"#,
// Depends on how null values are handled.
MongoDb(_) | MySql(_) | Sqlite | CockroachDb => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":2,"b":{"c":{"a":{"id":4}}}}]}}"#],
MongoDb(_) | MySql(_) | Sqlite | CockroachDb(_) => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":2,"b":{"c":{"a":{"id":4}}}}]}}"#],
_ => vec![r#"{"data":{"findManyModelA":[{"id":1,"b":{"c":{"a":{"id":3}}}},{"id":2,"b":{"c":{"a":{"id":4}}}},{"id":3,"b":null},{"id":4,"b":null}]}}"#]
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,23 +136,23 @@ mod views {

// schema name must be the name of the test in which it's called.
async fn migrate_view_sql(runner: &Runner, schema_name: &str) -> String {
match runner.connector() {
ConnectorTag::Postgres(_)
| ConnectorTag::Cockroach(_)
match runner.connector_version() {
ConnectorVersion::Postgres(_)
| ConnectorVersion::CockroachDb(_)
=> {
r#"CREATE VIEW "TestView" AS SELECT "TestModel".id, "TestModel"."firstName", "TestModel"."lastName", CONCAT("TestModel"."firstName", ' ', "TestModel"."lastName") as "fullName" From "TestModel""#.to_owned()
}
ConnectorTag::MySql(_) | ConnectorTag::Vitess(_)
ConnectorVersion::MySql(_) | ConnectorVersion::Vitess(_)
=> {
r#"CREATE VIEW TestView AS SELECT TestModel.*, CONCAT(TestModel.firstName, ' ', TestModel.lastName) AS "fullName" FROM TestModel"#.to_owned()
},
ConnectorTag::Sqlite(_) => {
ConnectorVersion::Sqlite => {
r#"CREATE VIEW TestView AS SELECT TestModel.*, TestModel.firstName || ' ' || TestModel.lastName AS "fullName" FROM TestModel"#.to_owned()
}
ConnectorTag::SqlServer(_) => {
ConnectorVersion::SqlServer(_) => {
format!(r#"CREATE VIEW [views_{schema_name}].[TestView] AS SELECT [views_{schema_name}].[TestModel].[id], [views_{schema_name}].[TestModel].[firstName], [views_{schema_name}].[TestModel].[lastName], CONCAT([views_{schema_name}].[TestModel].[firstName], ' ', [views_{schema_name}].[TestModel].[lastName]) as "fullName" FROM [views_{schema_name}].[TestModel];"#)
},
ConnectorTag::MongoDb(_) => unreachable!(),
ConnectorVersion::MongoDb(_) => unreachable!(),
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use query_engine_tests::*;
#[test_suite(schema(schema))]
mod update_many {
use indoc::indoc;
use query_engine_tests::{is_one_of, run_query, run_query_json, ConnectorTag};
use query_engine_tests::{is_one_of, run_query, run_query_json};

fn schema() -> String {
let schema = indoc! {
Expand Down Expand Up @@ -297,7 +297,7 @@ mod update_many {
let count = &res["data"]["updateManyTestModel"]["count"];

// MySql does not count incrementing a null so the count is different
if !matches!(runner.connector(), ConnectorTag::MySql(_)) {
if !matches!(runner.connector_version(), ConnectorVersion::MySql(_)) {
assert_eq!(count, 3);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ edition = "2021"
serde_json.workspace = true
prisma-models = { path = "../../prisma-models" }
once_cell = "1"
enum_dispatch = "0.3"
qe-setup = { path = "../qe-setup" }
request-handlers = { path = "../../request-handlers" }
tokio.workspace = true
Expand Down
39 changes: 32 additions & 7 deletions query-engine/connector-test-kit-rs/query-tests-setup/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::{ConnectorTag, ConnectorTagInterface, TestResult};
use crate::{
CockroachDbConnectorTag, ConnectorTag, ConnectorVersion, MongoDbConnectorTag, MySqlConnectorTag,
PostgresConnectorTag, SqlServerConnectorTag, SqliteConnectorTag, TestResult, VitessConnectorTag,
};
use serde::Deserialize;
use std::{convert::TryFrom, env, fs::File, io::Read, path::PathBuf};

Expand Down Expand Up @@ -117,11 +120,22 @@ impl TestConfig {
exit_with_message("A test connector is required but was not set.");
}

match self.test_connector_tag() {
Ok(tag) if tag.is_versioned() && self.connector_version.is_none() => {
match self.test_connector().map(|(_, v)| v) {
Ok(ConnectorVersion::Vitess(None))
| Ok(ConnectorVersion::MySql(None))
| Ok(ConnectorVersion::SqlServer(None))
| Ok(ConnectorVersion::MongoDb(None))
| Ok(ConnectorVersion::CockroachDb(None))
| Ok(ConnectorVersion::Postgres(None)) => {
exit_with_message("The current test connector requires a version to be set to run.");
}
Ok(_) => (),
Ok(ConnectorVersion::Vitess(Some(_)))
| Ok(ConnectorVersion::MySql(Some(_)))
| Ok(ConnectorVersion::SqlServer(Some(_)))
| Ok(ConnectorVersion::MongoDb(Some(_)))
| Ok(ConnectorVersion::CockroachDb(Some(_)))
| Ok(ConnectorVersion::Postgres(Some(_)))
| Ok(ConnectorVersion::Sqlite) => (),
Err(err) => exit_with_message(&err.to_string()),
}
}
Expand All @@ -130,16 +144,27 @@ impl TestConfig {
self.connector.as_str()
}

pub fn connector_version(&self) -> Option<&str> {
pub(crate) fn connector_version(&self) -> Option<&str> {
self.connector_version.as_ref().map(AsRef::as_ref)
}

pub fn is_ci(&self) -> bool {
self.is_ci
}

pub fn test_connector_tag(&self) -> TestResult<ConnectorTag> {
ConnectorTag::try_from((self.connector(), self.connector_version()))
pub fn test_connector(&self) -> TestResult<(ConnectorTag, ConnectorVersion)> {
let version = ConnectorVersion::try_from((self.connector(), self.connector_version()))?;
let tag = match version {
ConnectorVersion::SqlServer(_) => &SqlServerConnectorTag as ConnectorTag,
ConnectorVersion::Postgres(_) => &PostgresConnectorTag,
ConnectorVersion::MySql(_) => &MySqlConnectorTag,
ConnectorVersion::MongoDb(_) => &MongoDbConnectorTag,
ConnectorVersion::Sqlite => &SqliteConnectorTag,
ConnectorVersion::CockroachDb(_) => &CockroachDbConnectorTag,
ConnectorVersion::Vitess(_) => &VitessConnectorTag,
};

Ok((tag, version))
}
}

Expand Down
Loading

0 comments on commit 36f92fe

Please sign in to comment.