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.

Sign up for GitHub

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

Jump to bottom

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

Open
davidroth opened this issue Sep 8, 2020 · 5 comments · May be fixed by #1532
Open

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

davidroth opened this issue Sep 8, 2020 · 5 comments · May be fixed by #1532
Assignees
@davidroth
Labels
enhancement New feature or request test
Milestone
Backlog

Comments

@davidroth
Copy link

davidroth commented Sep 8, 2020 •
edited
Loading

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 roji added this to the 5.0.0 milestone Sep 8, 2020
@roji roji added blocked enhancement New feature or request test labels Sep 8, 2020
@roji
Copy link
Member

roji commented Oct 9, 2020

@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
Copy link
Author

@roji Sure, you can assign me 👍

@roji roji removed the blocked label Oct 9, 2020
@davidroth
Copy link
Author

@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
Copy link
Member

roji commented Oct 10, 2020

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. 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 😅 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
Copy link
Author

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 😄

davidroth added a commit to davidroth/efcore.pg that referenced this issue Oct 12, 2020
@davidroth
Rrun BuiltInDataTypesNpgsqlTest again with the NodaTime plugin activa…
ab3912b 
…ted to verify that all BCL date/time types are properly supported.

Closes npgsql#1490
davidroth added a commit to davidroth/efcore.pg that referenced this issue Oct 12, 2020
@davidroth
Run BuiltInDataTypesNpgsqlTest again with the NodaTime plugin activat…
01874ea 
…ed to verify that all BCL date/time types are properly supported.

Closes npgsql#1490
This was referenced Oct 12, 2020
Open
Merged
@roji roji modified the milestones: 5.0.0, 6.0.0 Nov 4, 2020
@roji roji modified the milestones: 6.0.0, 7.0.0 Oct 9, 2021
@roji roji modified the milestones: 7.0.0, 8.0.0 Oct 15, 2022
@roji roji modified the milestones: 8.0.0, Backlog Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davidroth davidroth

Labels
enhancement New feature or request test
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Test that BCL Types work with NodaTime plugin activated davidroth/efcore.pg
2 participants
@davidroth @roji