Binary backend format support

[glenebob]

Hi Francisco,

Since we're all synced up now, I made another branch for the binary backend format support I've been working on. The bytea and int support is in and all tests pass, so it should be ready for evaluation and merge.

-Glen

[glenebob]

facepalm

Fixed...

[franciscojunior]

I just started to test here. I noticed that if you try to run the FunctionCallWithParametersPrepareReturnSingleValueNpgsqlDbType_SuppressBinary test, it fails with the following stacktrace:

NullReferenceException
at
NpgsqlTests.CommandTests.FunctionCallWithParametersPrepareReturnSingleValueNpgsqlDbType_SuppressBinary() 
in
C:\Users\FranciscoJunior\Desenvolvimento\GitHub\Npgsql2\testsuite\noninteractive\NUnit20\CommandTests.cs:linha 418

I think this is because the BackendBinarySuppressor constructor expects an initialized SuppressBinaryBackendEncoding which is the case if I run the test suite but not if I run a single test.
And run a single test is a common scenario because sometimes you want to fix a bug running only that test and not the whole suite.

If I initialize the SuppressBinaryBackendEncoding variable directly, it works ok. The single test and the test suite.

protected FieldInfo SuppressBinaryBackendEncoding = System.Reflection.Assembly.Load("Npgsql").GetType("NpgsqlTypes.NpgsqlTypesHelper").GetField("SuppressBinaryBackendEncoding", BindingFlags.Static | BindingFlags.NonPublic);

[franciscojunior]

Or we could add a call to ResolveSuppressBinaryBackendEncoding() inside the SetUp method. It would be called before any test. We would need to take care of not having too much reflection calls though.

[glenebob]

That's why I had the initializer being called every time.

[franciscojunior]

I'm +1 to declare SuppressBinaryBackendEncoding with initializer directly. This would remove the need for _BindTo... method and would allow the tests to be independent from each other.

[glenebob]

If you do it in BuildUp(), and it throws, no tests will run at all. Maybe a good thing, maybe not?

Not sure what you mean by eliminating _BindTo?

This is why it was so complicated before, because every attempt to suppress included a check to see if init was yet needed on the FieldInfo :)

[glenebob]

I simplified the code a lot, so I think if I put back the init check into SuppressBackendBinary(), it will still be quite simple, and once again allow single tests to run properly.

[franciscojunior]

If you put the initializer in the variable, you won't need to keep the _BindTo_NpgsqlTypes_NpgsqlTypesHelper_SuppressBinaryBackendEncoding as it is being used as an initializer for the SuppressBinaryBackendEncoding field.

"This is why it was so complicated before, because every attempt to suppress included a check to see if init was yet needed on the FieldInfo :)"

This is why I'm saying that you can be a little bit loose regarding the SuppressBinaryBackendEncoding initialization. It can't be null. This is an assumption of the test suite code you can make. There won't be any need to do checking if it is null or not. If the test fail because this field is null, there is something fundamentally wrong and should be fixed.

[franciscojunior]

And btw, your simplification is awesome. That's why I'm discussing with you in order to not put back those not null checks :)

[glenebob]

If the FieldInfo is null when a test tries to use it, and a test is not done to see if it's null, how will it get initialized? I think we're missing each other again somehow :(

As I see it, we can either initialize on demand (what it did originally), or we can initialize in BuildUp() (which stops all tests from running on failure.) Any other way, and individual tests cannot use it. I can't tell which of those you're suggesting.

Or maybe you mean to do the bind in BackendBinarySuppressor() constructor? But that seems expensive to me, which is why I avoided it in the first place.

[franciscojunior]

If instead of this line: https://github.com/franciscojunior/Npgsql2/pull/41/files#L6R74

protected FieldInfo SuppressBinaryBackendEncoding = null;

you use

protected FieldInfo SuppressBinaryBackendEncoding = System.Reflection.Assembly.Load("Npgsql").GetType("NpgsqlTypes.NpgsqlTypesHelper").GetField("SuppressBinaryBackendEncoding", BindingFlags.Static | BindingFlags.NonPublic);

It will always be not null and will be able to be used in any test. There won't be any need for setup calls or other initializers.
The only way it would be null is if there is any problem in the reflection call which will means we have a big problem to fix inside Npgsql.

[glenebob]

Ooooooohh lol...

So that would mean a reflection failure would cause BaseClassTests construction to fail. Since I can't do try/catch there, I can't provide a more meaningful error. But if I write a default constructer and do it there, maybe it would be better. I will test failure modes and see how it looks.

[franciscojunior]

:)

It is true that I'm assuming there won't be errors in the reflection call. I can't see any scenario this would happen unless a disruption in Npgsql like some change in the field name or access modifiers. And in this case, I think that a failure in the test would be enough. I think Nunit would be able to show any text the reflection call failure would generate, or if it silently returns an error and sets the field null, a lot of null reference exceptions will be thrown in the tests and we would be able to check what is the source of the problem.

[franciscojunior]

Just thought about another approach :
You could initialize the variable as I said and then in the setup method
you could put an assert not null. This way if the filed is null you will
flag it in the tests.
What do you think?
Em 14/08/2013 19:02, "Glen Parker" notifications@github.com escreveu:

Ooooooohh lol...

So that would mean a reflection failure would cause BaseClassTests
construction to fail. Since I can't do try/catch there, I can't provide a
more meaningful error. But if I write a default constructer and do it
there, maybe it would be better. I will test failure modes and see how it
looks.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-22670694
.

[glenebob]

If I do what you suggested, and it fails, all tests fail, but no error information is provided. In don't understand that.

If I do it in a default constructor wrapped in a try/catch/throw, all tests fail with the same exception. Pretty ugly in my opinion but at least that way a clue is provided.

Nunit's behavior seems really odd here...

I still like it better the way it is because one test with a descriptive name fails with a descriptive error, and then only the tests that try to suppress binary fail. And in fact, in SuppressBackendBinary(), we have the opportunity to privide yet another descriptive error (not just null reference).

I'm kinda big on descriptive error reporting :)

Sorry, didn't see that last comment.

[glenebob]

An exception thrown in SetUp() don't do what you think. No error is provided to the UI.

Maybe you could do some testing. Add a throw or assert or something in SetUp(), run tests, and see what you think is going on there. It doesn't make any sense to me.

[franciscojunior]

Sure! I'll do it.
I'm not in my dev machine now. I'll make the tests later and will let you
know.
Em 14/08/2013 20:04, "Glen Parker" notifications@github.com escreveu:

An exception thrown in SetUp() don't do what you think. No error is
provided to the UI.

Maybe you could do some testing. Add a throw or assert or something in
SetUp(), run tests, and see what you think is going on there. It doesn't
make any sense to me.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-22674231
.

[franciscojunior]

I did some tests here and I think the idea of assert in the setup provides the information we are looking for.

This is what I did:

Added this line to SetUp:

Assert.IsNotNull(SuppressBinaryBackendEncoding, "SuppressBinaryBackendEncoding shouldn't be null. Check NpgsqlTypes.NpgsqlTypesHelper.SuppressBinaryBackendEncoding field");

And then I run the tests and got this error message:

NpgsqlTests.CommandTests.FunctionCallWithParametersPrepareReturnSingleValueNpgsqlDbType:
SetUp : SuppressBinaryBackendEncoding shouldn't be null. Check NpgsqlTypes.NpgsqlTypesHelper.SuppressBinaryBackendEncoding field

Then I added the line which initialize the SuppressBinaryBackendEncoding variable and could confirm it worked ok.

Now, to simulate a problem where for some reason the field is not initialized correctly, I changed the line to this:

System.Reflection.Assembly.Load("Npgsql").GetType("NpgsqlTypes.NpgsqlTypesHelper").GetField("SuppressBinaryBackendEnc", BindingFlags.Static | BindingFlags.NonPublic);

Note that I changed the name of the field to simulate a problem in the reflection call.

Then I could test that the message is very clear and shows exactly where the problem is:

NpgsqlTests.CommandTests.FunctionCallWithParametersPrepareReturnSingleValueNpgsqlDbType_SuppressBinary:
SetUp : SuppressBinaryBackendEncoding shouldn't be null. Check NpgsqlTypes.NpgsqlTypesHelper.SuppressBinaryBackendEncoding field

Of course this code won't tell you why the reflection code failed, but I think that if this happens, and unless it was a change by mistake of the field name, we would have a bigger problem than Npgsql tests.

What do you think?

[glenebob]

Doesn't that cause every test to fail, even tests that don't attempt to suppress binary encoding? That's what happens when I do similar here.

If I put the assert() in SuppressBackendBinary(), I get the somewhat-descriptive error, but only for tests that try to suppress. All others are unaffected.

This also requires to out the binding within a try/catch that does not throw, which is why the detailed error info is lost

I plagiarized your error message and will commit what I have now :)

[glenebob]

"By putting this initialization in SetUp, it will slow down all tests with reflection call. Even the ones which don't use BackendBinarySuppressor."

Correct me if I'm wrong, but SetUp() is called just once per text fixture, right? So for CommandTests, it would be called once, and then nearly 200 tests would run without calling it again.

[franciscojunior]

Nope. That would be the SetUpFixtureAttribute
http://www.nunit.org/index.php?p=setupFixture&r=2.4
SetUp will be run before each test is run.

I had a look at the SetUpFixtureAttribute but it requires the attribute to
be applied to another class and this would complicate even more the
dependency between this class and the baseclasstests. It is more indicated
if you have a class which does initialization without having to depend on
other classes.

I learned about SetUpFixtureAttribute in this thread:
http://stackoverflow.com/questions/3619735/nunit-global-initialization-bad-idea

while checking the documentation I found something which can help us
achieve the one time initialization:
http://www.nunit.org/index.php?p=fixtureSetup&r=2.4
TestFixtureSetUpAttribute (NUnit 2.1)

This attribute is used inside a TestFixture to provide a single set of
functions that are performed once prior to executing any of the tests in
the fixture.
I think this would enable us to put all initializations of the testsuite in
one place.
What do you think?

On Thu, Aug 15, 2013 at 12:31 PM, Glen Parker notifications@github.comwrote:

"By putting this initialization in SetUp, it will slow down all tests with
reflection call. Even the ones which don't use BackendBinarySuppressor."

Correct me if I'm wrong, but SetUp() is called just once per text fixture,
right? So for CommandTests, it would be called once, and then nearly 200
tests would run without calling it again.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-22710671
.

Regards,

Francisco Figueiredo Jr.
Npgsql Lead Developer
http://www.npgsql.org
http://gplus.to/franciscojunior
http://fxjr.blogspot.com
http://twitter.com/franciscojunior