perlpunk / YAML-PP-p5

A YAML 1.2 processor in perl
https://metacpan.org/pod/YAML::PP
24 stars 8 forks source link

t/31.schema.t fails tests 238 and 3838 when nvtype is IBM DoubleDouble #33

Closed sisyphus closed 4 years ago

sisyphus commented 4 years ago

Hi,

This is a -Duselongdouble build of perl-5.32.0 (and 5.31.0 suffers the same problem) on Debian wheezy:

$ perl -V:longdblkind longdblkind='6';

The errors are reported in the test suite as follows:

t/31.schema.t .............. 20/? # Failed test '(yaml11) type float: load(!!float 190:20:30.15) eq '685230.15'' # at t/31.schema.t line 138. # got: 685230.15 # expected: 685230.15 t/31.schema.t .............. 3615/? # Failed test '(yaml11) type float: load(190:20:30.15) eq '685230.15'' # at t/31.schema.t line 138. # got: 685230.15 # expected: 685230.15 t/31.schema.t .............. 4123/? # Looks like you failed 2 tests of 4394. t/31.schema.t .............. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/4394 subtests

and, at the end of the test suite:

t/31.schema.t (Wstat: 512 Tests: 4394 Failed: 2) Failed tests: 238, 3838 Non-zero exit status: 2

The problem is that $data != $def{data} in this particular case, though they are both (obviously) stringifying to the same string of "685230.15". For this particular case scalar(reverse(unpack "h", pack("D<", $data))) is 4124e95c4ccccccdbdb999999999999a scalar(reverse(unpack "h", pack("D<", $def{data}))) is 4124e95c4ccccccdbdb9999999999998 which is a very minor discrepancy (of 2 units in the last place).

The former value (ie $data) is the correct representation of 685230.15 for this architecture, though both perl and C will inaccurately assign the latter value (which translates to 685230.150000000000000000000000005. (The least significant double is a negative value, so $def{data} is in fact greater than $data, despite the appearance to the contrary.)

Could the test be changed from "==" to "eq" ? (The diagnostic message suggests that the intention was to test "eq", not "==".)

Cheers, Rob

perlpunk commented 4 years ago

Thanks!

I actually want to compare via == and not eq ($def{data} could contain 300.00 for example). I just didn't think of floating point arithmetic problems, although I should have known it ;-)

I fixed the test simply by avoiding rounding errors with sprintf in https://github.com/perlpunk/YAML-PP-p5/commit/9e907d1a5f31cb83f5005d95b2f6d87584d6a6dd

Could you test it?

sisyphus commented 4 years ago

It seems that the first change where cmp_ok() is changed from performing an 'eq' test to an '==' might be the wrong thing. It produces thousands of non-numeric warnings of the form: Argument "*" isn't numeric in numeric eq (==) at (eval in cmp_ok) t/31.schema.t line 120. where "*" is such things as "true", "y", "yes", "no", "null", "off", "on", "~", to name a few.

That's not causing any tests to fail, but for me the second change, where $data is replaced by sprintf("%0.2f",$data), is alone sufficient to allow all tests to pass.

However, I don't think I'm using the exact same version of 31.schema.t as is presented at https://github.com/perlpunk/YAML-PP-p5/commit/9e907d1a5f31cb83f5005d95b2f6d87584d6a6dd . What seems to be line 128 of that file is actually line 120 on my copy of the file.

The interesting thing about that second change is that it fixes the failures by coercing the LHS of the '==' comparison to the same incorrect representation of 685230.15 as held by the RHS ($def{data}). That approach is a little unusual and threw me initially - though it's a totally sane, valid and most practical solution.

Cheers, Rob

perlpunk commented 4 years ago

Huh, weird. It looks like the change got accidentally applied to the $type eq 'str' condition on your side. Then I get the same warnings as you. Here's the direct link to the correct file (branch testfloat): https://github.com/perlpunk/YAML-PP-p5/blob/testfloat/t/31.schema.t

Thanks!

sisyphus commented 4 years ago

Yep - https://github.com/perlpunk/YAML-PP-p5/blob/testfloat/t/31.schema.t is fine.

Cheers, Rob