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

Implement Precompiled Query Tests #3289

Merged
merged 6 commits into from
Sep 23, 2024
Merged

Conversation

ChrisJollyAU
Copy link
Contributor

  • Enable support for precompiled queries
  • Adds the precompiled query tests in

Most work except for a few exceptions: Concat, Except,Intersect,Union

Looks like we need to find a way to add a reference to Npgsql when its doing the compilation as the error is of the style: The type 'NpgsqlTsVector' is defined in an assembly that is not referenced. You must add a reference to assembly 'Npgsql, Version=8.0.3.0, Culture=neutral, PublicKeyToken=5d8b90d52f46fda7'.

Contributes to #3257

@roji
Copy link
Member

roji commented Sep 21, 2024

@ChrisJollyAU thanks for working on this, much appreciated!

Looks like we need to find a way to add a reference to Npgsql when its doing the compilation [...]

I think it's simply a matter of including a reference to the assembly in NpgsqlPrecompiledQueryTestHelpers, just like you're already doing for the EF provider, no?

@ChrisJollyAU
Copy link
Contributor Author

@roji Yep. That works fine. Thanks

Removed a test Do_not_cache_is_respected. It was failing and I don't believe its applicable for Npgsql?

@ChrisJollyAU
Copy link
Contributor Author

@roji In PrecompiledSqlPregenerationQueryNpgsqlTest.cs there is a bunch of tests commented out. Think they got copied over originally from the SQL Server version. Think they are applicable for postgresql?

Got one remaining test failing Two_captured_variables_in_different_lambdas with error System.InvalidOperationException : Sequence contains no elements.

…QueryTestHelpers.cs

Co-authored-by: Shay Rojansky <roji@roji.org>
@ChrisJollyAU
Copy link
Contributor Author

@roji The above mentioned test is failing due to case sensitivity. The base is set up to expect a case-insensitive startswith (aka LIKE) but postgresql defaults to case sensitive

Change the seeded data from "Blog" to "blog" or completely override the test and use "Blog" as the starts parameter?

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good! We probably need to add test coverage for Npgsql-specific expressions (and also actually implement the quotability for all of them!), but that can be done in a separate PR... Am happy to accept a PR if you're interested in that work!

Re test Two_captured_variables_in_different_lambdas, that's a bug in the EF test - I submitted #34731 to fix this - for 10. In the meantime, can you implement the Npgsql override by doing Assert.Throws() when calling the test, this way we're alerted when the upstream fix is synced in 10 and fix it?

@roji
Copy link
Member

roji commented Sep 22, 2024

BTW note that there has been some flakiness in the CI, where some builds hang... I still need to look at it, but we currently aren't getting fully green builds (and that's OK).

@ChrisJollyAU
Copy link
Contributor Author

Fixed up the last test. Looks like they are all passing now.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your help on this @ChrisJollyAU!

@roji roji merged commit 87a1a7e into npgsql:main Sep 23, 2024
15 of 16 checks passed
return builder;
}

protected override async Task SeedAsync(PrecompiledQueryContext context)
Copy link
Member

Choose a reason for hiding this comment

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

@ChrisJollyAU noticed after merging that this is still overridden - is that needed for some reason, or is the base implementation good here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The seeding does need to be overridden. EFCore.PG doesn't like DateTime when the DateTimeKind is Unspecified

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

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