Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DA] Planetscale engine tests: multi schema #4470

Closed
miguelff opened this issue Nov 21, 2023 · 5 comments
Closed

[DA] Planetscale engine tests: multi schema #4470

miguelff opened this issue Nov 21, 2023 · 5 comments
Labels
topic: driver adapters formerly phase 1

Comments

@miguelff
Copy link
Contributor

miguelff commented Nov 21, 2023

This is a subcluster of failures after having enabled planetscale tests in #4423

Failing tests in this cluster:

new::multi_schema::multi_schema::create_and_get_many_relations
new::multi_schema::multi_schema::create_and_get_many_to_many_relations
new::multi_schema::multi_schema::crud_many_simple
new::multi_schema::multi_schema::crud_relations
new::multi_schema::multi_schema::crud_simple
new::multi_schema::multi_schema::implicit_m2m_simple
new::multi_schema::multi_schema::test_filter_in

Follow the process in This notion scratchpad page

@jkomyno
Copy link
Contributor

jkomyno commented Feb 29, 2024

The multiSchema preview feature explicitly panics when used with MySQL-adjacent providers, by design, as it never got implemented.

Hence, PlanetScale is excluded from these tests because of a limitation of our implementation on its underlying Quaint+PSL connector.


You may have noticed that the header of the multi_schema tests is as follows:

// "multiSchema migrations and introspection are not implemented on MySQL yet"
#[test_suite(
    capabilities(MultiSchema),
    exclude(Mysql, Vitess("planetscale.js", "planetscale.js.wasm"))
)]

For a more appropriate clarity to developers touching these tests, we'd only need to declare that the MultiSchema capability is required, so that we don't need to manually exclude the providers/flavors that wouldn't succeed on test cases related to this preview feature:

- // "multiSchema migrations and introspection are not implemented on MySQL yet"
#[test_suite(
  capabilities(MultiSchema),
-   exclude(Mysql, Vitess("planetscale.js", "planetscale.js.wasm"))
)]

Unfortunately, this will only be possible once we understand the impact of removing these lines:

// why is this here, considering that using multiple schemas in MySQL leads to the
// "multiSchema migrations and introspection are not implemented on MySQL yet" error?
MultiSchema |

@janpio
Copy link
Member

janpio commented Mar 1, 2024

For our users PlanetScale is just MySQL. So if we support multiSchema for MySQL, they also expect it to work on a database hosted at PlanetScale.

@jkomyno
Copy link
Contributor

jkomyno commented Mar 4, 2024

For our users PlanetScale is just MySQL. So if we support multiSchema for MySQL, they also expect it to work on a database hosted at PlanetScale.

I'd agree with you in theory, but we do NOT support multiSchema on MySQL. Please see: prisma/prisma#16943.

In other words, it's totally normal for PlanetScale to fail on multiSchema tests, and that's by design, because the MySQL connector itself fails on them even when using the Rust driver.

@janpio
Copy link
Member

janpio commented Mar 4, 2024

Why do we run the tests for it then? We do not have equivalent failing tests on the Rust side.

@jkomyno
Copy link
Contributor

jkomyno commented Mar 5, 2024

Why do we run the tests for it then? We do not have equivalent failing tests on the Rust side.

We have equivalent tests on the Rust side which would fail, were they toggled off (or, so to speak, "commented out").

With this message above in mind, we see that the multi_schema tests start with:

#[test_suite(
    capabilities(MultiSchema),
    exclude(Mysql, Vitess("planetscale.js", "planetscale.js.wasm"))
)]

In plain English, this translates to:

  • Run each test case wrapped by this test_suite macro only in those connectors that support the MultiSchema capability, i.e., that are able to perform multi-schema queries without crashing.
  • From this set of connectors, we exclude Mysql (i.e., MySQL / MariaDB databases using a Rust driver), and planetscale.js* (Napi/Wasm driver adapters for PlanetScale/Vitess databases).

A reasonable question/doubt would be:

Why do we need to exclude these connectors? Didn't we just say that Prisma MySQL-adjacent connectors do NOT support the multiSchema preview feature?

That's correct. However, for some reason, MySQL is marked with the MultiSchema capability, which imho is a mistake.

The immediate choice, in my opinion, is to get rid of this line, so that we can transform the test header from

// "multiSchema migrations and introspection are not implemented on MySQL yet"
#[test_suite(
    capabilities(MultiSchema),
    exclude(Mysql, Vitess("planetscale.js", "planetscale.js.wasm"))
)]

to

- // "multiSchema migrations and introspection are not implemented on MySQL yet"
#[test_suite(
  capabilities(MultiSchema),
-   exclude(Mysql, Vitess("planetscale.js", "planetscale.js.wasm"))
)]

@apolanc apolanc closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: driver adapters formerly phase 1
Projects
None yet
Development

No branches or pull requests

5 participants