Update NodaTimeTests and add BCL types to verify BCL type handler forwarding

[davidroth]

Once BCL type handler forwarding support is merged, it makes sense to update noda time tests in efcore.pg. Ex. BCL datetime types should be added here so that they are tested as well: https://github.com/npgsql/efcore.pg/blob/dev/test/EFCore.PG.NodaTime.FunctionalTests/NodaTimeQueryNpgsqlTest.cs#L431

This has been discussed here.

[roji]

@davidroth the latest version of EFCore.PG now finally depends on the latest Npgsql 5.0.0, are you interested in updating the tests here?

[davidroth]

@roji Sure, you can assign me 👍

[davidroth]

@roji 2 questions before I start:

• After looking at NodaTimeQueryNpgsqlTest, I think it makes most sense to leave existing tests as is but add some additional tests based on a new DbSet<BclTimeTypes>. I`d just check if all BCL-Types can be stored and queried. Asserting SQL and testing individual component queries (like with the existing tests) is not necessary IMHO. Do you agree?
• There are several commented tests in NodaTimeQueryNpgsqlTest, starting at line 237. What was the reason for commenting them?

[roji]

After looking at NodaTimeQueryNpgsqlTest, I think it makes most sense to leave existing tests as is but add some additional tests based on a new DbSet<BclTimeTypes>. I`d just check if all BCL-Types can be stored and queried. Asserting SQL and testing individual component queries (like with the existing tests) is not necessary IMHO. Do you agree?

It sounds fine to just test storage/query of the BCL types. However, rather than coding new tests to do that, we could just run BuiltInDataTypesNpgsqlTest again with the NodaTime plugin activated - that already verifies that all BCL date/time types are properly supported. How does that sound?

There are several commented tests in NodaTimeQueryNpgsqlTest, starting at line 237. What was the reason for commenting them?

I honestly don't remember anymore :sweat_smile: If you'd like to have a go at running those tests, please go ahead although that's definitely not required as part of this...

[davidroth]

However, rather than coding new tests to do that, we could just run BuiltInDataTypesNpgsqlTest again with the NodaTime plugin activated - that already verifies that all BCL date/time types are properly supported. How does that sound?

Sounds very good 👍

I honestly don't remember anymore 😅 If you'd like to have a go at running those tests, please go ahead although that's definitely not required as part of this..

Ok, maybe I'll have a look 😄