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

Unskip tests for driver adapters core features, skip tests for low fidelity cases #4400

Merged
merged 12 commits into from
Nov 13, 2023

Conversation

miguelff
Copy link
Contributor

@miguelff miguelff commented Oct 31, 2023

Copy link

codspeed-hq bot commented Oct 31, 2023

CodSpeed Performance Report

Merging #4400 will not alter performance

Comparing mff/472 (92fcec6) with main (2963919)

Summary

✅ 11 untouched benchmarks

@miguelff miguelff force-pushed the mff/472 branch 2 times, most recently from 03e0d0a to 67a2559 Compare November 1, 2023 10:59
@miguelff miguelff marked this pull request as ready for review November 10, 2023 14:18
Comment on lines -34 to -35
- name: 'planetscale'
setup_task: 'dev-planetscale-vitess8'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally dropping something that is currently in main?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this doesn't provide any value at all until fituring out the vitess test situation for driver adapters. Which has it's own issue. As of now, the feedback on this test suite doesn't provide any kind of value.

BTW: PS tests pass on prisma/prisma, but I expect a lot of tests broken for planetscale in the engines.

runner,
r#"query { findManyTestModel { field } }"#,
2023,
"Inconsistent column data: Conversion failed: number must be an integer in column 'field'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can not easily make this error message similar to the Quaint one?

(It's fine, just and error edge case, but double checking as of course less variants would be better)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly the same. No.

Comment on lines +8 to +11
// The following tests are excluded for driver adapters. The underlying
// driver rejects queries where the values of the positional arguments do
// not match the expected types. As an example, the following query to the
// driver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that our raw query API with driver adapters is less accepting than our Rust one?

(Not a problem at all, just interesting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a little bit more strict.

@janpio janpio added this to the 5.6.0 milestone Nov 10, 2023
@miguelff
Copy link
Contributor Author

This merge commit will fail, because it requires changes from prisma/prisma#21889, it's OK.

@miguelff miguelff merged commit 7711046 into main Nov 13, 2023
55 checks passed
@miguelff miguelff deleted the mff/472 branch November 13, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants