-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Improve SqlDataRecord test coverage #25171
Comments
(dotnet/corefx@master...RemiBou:master) Added 2 tests, is that what you are expecting ? It seems that the first test (SqlRecordFillTest) is quite big and my tests are testing way less things, what do you prefer ? |
@RemiBou I think you are on the right track. I prefer smaller tests. They are more focussed and easier to debug |
@saurabh500 on GetOrdinal, can I change the initialisation of _fieldNameLookup with a Lazy<> ? Or is that out of scope ? Or even not a good idea (it'll make this method thread safe) |
I would say it is out of scope for now until we figure out that Thread Safety is really needed here. If you find along the way, during your involvement that Thread Safety can be utilized here, I would go for it. |
Given that GetOrdinal is called on the [] accessor, someone might Parallelize some calls (that would be interesting only on record with a lot of columns), but that's not obvious though |
dotnet/corefx@master...RemiBou:master +43 lines covered, is that still on phase with your policy/pratice ? |
Good stuff @RemiBou You are on point. |
Improve SqlDataRecord test coverage.
cc @RemiBou
The text was updated successfully, but these errors were encountered: