Fix tests errors

[franciscojunior]

This pull request fixes some errors in the tests.
Please, don't merge it yet. WIP

[Emill]

Maybe you could fix the other places where this syntax is used as well?

[franciscojunior]

Sure! I'll do that.
Em 03/01/2015 18:40, "Emill" notifications@github.com escreveu:

Maybe you could fix the other places where this syntax is used as well?

—
Reply to this email directly or view it on GitHub
#449 (comment).

[franciscojunior]

Hi, @Emill

While checking why VerifyFunctionWithNoParametersWithDeriveParameters, wasn't working (besides a missing function creation problem which was triggered by running the test alone), I noticed the following code inside DoDeriveParameters()

if (types == null)
                        {
                            if (rdr.IsDBNull(1) || rdr.GetFieldValue<uint[]>(1).Length == 0)
                                return;  // Parameterless function
                            types = rdr.GetFieldValue<uint[]>(1);
                        }

It was giving problems in the rdr.GetFieldValue<uint[]>(1) with the exception The parameter is not of the correct format.

The problem seems to be the following:

npgsql_tests=# select proargnames is null, proargtypes is null, proallargtypes is null, proargmodes is null from pg_proc where proname = 'funcb';
 ?column? | ?column? | ?column? | ?column? 
----------+----------+----------+----------
 t        | f        | t        | t

Yes, it seems that for all other columns, proargnames, proallargtypes etc, the value returned for a parameterless function is null, except proargtypes! :)

It returns an empty string, and this was giving problems when the oid handler tried to get a number from it:

uintArr[i] = uint.Parse(stringArr[i]);

I thought fixing this problem in the DoDeriveParameter to handle this specific case, but I ended up fixing in the OidHandler itself. From what DoDeriveParameter expect, I think the idea of the GetFieldValue<uint[]> would be to really return an empty array in case of a empty string response.

So, this is the change I did to make it work:

diff --git a/Npgsql/TypeHandlers/InternalTypesHandlers/OIDVectorHandler.cs b/Npgsql/TypeHandlers/InternalTypesHandlers/OIDVectorHandler.cs
index b3060e2..88317a9 100644
--- a/Npgsql/TypeHandlers/InternalTypesHandlers/OIDVectorHandler.cs
+++ b/Npgsql/TypeHandlers/InternalTypesHandlers/OIDVectorHandler.cs
@@ -15,6 +15,8 @@ internal class OIDVectorHandler : TypeHandler<uint[]>
         static readonly NpgsqlDbType?[] _npgsqlDbTypes = { NpgsqlDbType.Oidvector };
         internal override NpgsqlDbType?[] NpgsqlDbTypes { get { return _npgsqlDbTypes; } }

+        static readonly uint[] _zeroLengthArray = new uint[0];
+
         public override bool AllowAutoInferring { get { return false; } }

         public override uint[] Read(NpgsqlBuffer buf, Messages.FieldDescription fieldDescription, int len)
@@ -22,7 +24,10 @@ public override uint[] Read(NpgsqlBuffer buf, Messages.FieldDescription fieldDes
             switch (fieldDescription.FormatCode)
             {
                 case FormatCode.Text:
-                    string[] stringArr = buf.ReadString(len).Split(' ');
+                    string dataFromBuffer = buf.ReadString(len);
+                    if (dataFromBuffer.Length == 0) { return _zeroLengthArray;}
+
+                    string[] stringArr = dataFromBuffer.Split(' ');
                     uint[] uintArr = new uint[stringArr.Length];
                     for (var i = 0; i < stringArr.Length; i++)
                         uintArr[i] = uint.Parse(stringArr[i]);

What do you think?

If you are ok with it, I can create a pull request for it.

I also think we should add some tests for those cases where an empty string is returned from the database where an uint[] was expected. Not only for uint[] but for other array types as well.

[Emill]

Ah nice. An empty string should result in an empty array. You can apply that patch of you want, but @roji is currently refactoring a lot of stuff so I'm not sure how long that code will survive anyway...

[roji]

Nice catch @franciscojunior.

But @Emill is right, it's better not to touch that area of the code - I'm really changing things there right now. The end of that refactor is probably not so far away though.

I think the best think is for you to make sure this is checked in a test (which will fail in master for now) and we'll get around to it soon...

[franciscojunior]

Ah nice. An empty string should result in an empty array. You can apply that patch of you want, but @roji is currently refactoring a lot of stuff so I'm not sure how long that code will survive anyway...

Nice catch @franciscojunior.

But @Emill is right, it's better not to touch that area of the code - I'm really changing things there right now. The end of that refactor is probably not so far away though.

Ahh, ok. I'll let this problem for now, then.

I'll keep fixing the errors I find in the tests and will add commits to this PR.

Thanks for the feedback, guys! :)

[roji]

There's been lots of refactoring since this PR so I went ahead and removed parentheses in unit tests in a7d9f4a, am closing. Note that a lot of function-related tests still fail, I'll be doing a general cleanup of all that as some point.