test(qe): Run all tests for vitess (only in driver adapters) #4423
Conversation
CodSpeed Performance ReportMerging #4423 will not alter performanceComparing Summary
|
miguelff
force-pushed
the
mff/tmp-planetscale-tests
branch
from
4a4f468
to
347e535
Compare
miguelff
force-pushed
the
mff/tmp-planetscale-tests
branch
6 times, most recently
from
fe3a34c
to
0a516f6
Compare
miguelff
force-pushed
the
mff/tmp-planetscale-tests
branch
from
806e620
to
bfe4811
Compare
miguelff
changed the title
miguelff
force-pushed
the
mff/tmp-planetscale-tests
branch
from
87e57dd
to
f2981ff
Compare
miguelff
requested review from
jkomyno and
Weakky
and removed request for
a team
miguelff
force-pushed
the
mff/tmp-planetscale-tests
branch
from
f2981ff
to
1626ab6
Compare
query-engine/connector-test-kit-rs/query-tests-setup/src/connector_tag/mod.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-tests-setup/src/connector_tag/mod.rs
Show resolved
Hide resolved
...gine/connector-test-kit-rs/query-engine-tests/tests/new/ref_actions/on_delete/set_default.rs
Outdated
Show resolved
Hide resolved
|
Thanks for the throrough review @Jolg42 🙌🏼 I will simplify the message in the validation of exclusion rules, remove the MySQL settings that we might not need, and leave the rest aside. |
miguelff
force-pushed
the
mff/tmp-planetscale-tests
branch
from
7373bd9
to
83ebbc8
Compare
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum SqliteVersion { | ||
| V3, | ||
| LibsqlJS, |
There was a problem hiding this comment.
Added versioning to sqlite, to be able to have a tag for libsql
query-engine/connector-test-kit-rs/query-tests-setup/src/connector_tag/vitess.rs
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-tests-setup/src/connector_tag/mod.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-tests-setup/src/connector_tag/mod.rs
Show resolved
Hide resolved
| { | ||
| "connector": "vitess", | ||
| "version": "planetscale.js", | ||
| "driver_adapter": "planetscale", | ||
| "driver_adapter_config": { | ||
| "proxy_url": "http://root:root@127.0.0.1:8085" | ||
| }, | ||
| "external_test_executor": "Wasm" | ||
| } |
There was a problem hiding this comment.
I'm a bit lost with the difference between the connector, the version and the driver_adapter field, now that we've added all driver adapters as connector versions. Could you elaborate how they differ and why we need all three? I feel like there's something wrong.
Specifically, it seems that we've merged the concept of connector and driver-adapater. Previously, you could choose which database & version you would want to run a specific driver-adapter against. Now that the version is pointing to driver-adapters, not only does it seem like we can't do what I just mentioned above (eg: run pg.js against postgres 10, 11, 12), but it looks confusing to me because as you have to specify redundant fields and I don't understand what each control
There was a problem hiding this comment.
Good catch! That's true. I left both separated only because neon has two potential driver adapters (HTTP and WS). But I can definitely derive the driver adapter from the connector 👍, because in the end, if we test neon-http, we can create a different connector version. For now they are, as you said, tightly coupled.
query-engine/connector-test-kit-rs/query-tests-setup/src/connector_tag/mod.rs
Show resolved
Hide resolved
Related to https://github.com/prisma/team-orm/issues/14
Part of https://github.com/prisma/team-orm/issues/541
TL:DR
This PR fixes the engines test suite by running tests for the planetscale driver adapters more exhaustively. Before, only the set of tests run for Vitess in driver adapters (8 tests) where run. Now the whole test suite except explicit exclusions runs.
How
Given that these tests run against the Planetscale proxy, and considering a different test suite will exist for Vitess with Rust drivers, we opted for the path of least friction when running the driver adapter tests. This involves using a single MySQL box behind the Planetscale proxy instead of a full vttest cluster. The reason is that I was unable to run the driver adapter tests with vitess behind the proxy in a reasonable amount of time when I started this PR.
This choice carried the following tradeoffs:
We don't exercise Vitess but MySQL. This is a close approximation, but there might be small differences in behavior (e.g., vttest may return different error messages than MySQL).
However, we do exercise the Planetscale proxy, and we use
relationMode=prisma, which resembles what actually happens within the query engine. In this the query-engine, Vitess does not exist as a provider, and as such, there isn't any particular capability or conditional code that makes the engine behave differently than when using MySQL. In the end, Vitess is just an abstraction existing in the test kit to: a) use the MySQL provider, b) run the suite withrelationMode=prisma, and c) be able to run or exclude specific tests for that configuration. But the existence of this testing connector is misleading, and it should probably be just a version of the MySQL testing connector instead.I consider the previous point, a good enough argument, to think that the planetscale driver adapter tests will provide enough confidence about the correctness of the driver adapters code.
Additional changes
I made some additional changes in this PR:
I created test connection versions for driver adapters, so the suite have those well represented and tests can be conditionally executed per driver adapter (which was not possible before) 0b802ab
I simplified driver adapter tests configuration after recent integration of the WASM query engine in engine tests: d2779e4
I tagged the failing tests in planescale (~70), clustered them, and created issues in the board for each of them.
There are two unrelated failures in schema engine that are also present in main, which I don't think I should fix as part of this PR.
https://github.com/prisma/engineer/pull/117 Has to be merged, and tagged before merging this PR