Bug 27630 - TestBuilder real number formatting is different to DBI(x)
Summary: TestBuilder real number formatting is different to DBI(x)
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-04 19:27 UTC by Lari Taskula
Modified: 2021-02-09 11:57 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 27630: Add tests (1.64 KB, patch)
2021-02-04 19:41 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 27630: Allow more TestBuilder decimal places than 2 (1.05 KB, patch)
2021-02-04 19:41 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 27630: Add test to prove different behavior with floats (2.14 KB, patch)
2021-02-04 21:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 27630: Add test to prove different behavior with floats (2.50 KB, patch)
2021-02-04 22:01 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 27630: Add tests (4.24 KB, patch)
2021-02-05 17:48 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 27630: Update database generated values and format in TestBuilder (3.21 KB, patch)
2021-02-05 17:48 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 27630: Add tests (4.29 KB, patch)
2021-02-06 09:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 27630: Update database generated values and format in TestBuilder (3.26 KB, patch)
2021-02-06 09:17 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2021-02-04 19:27:32 UTC
t::lib::TestBuilder::_gen_real() always returns 2 decimal places no matter the column's size definition.

> TestBuilder.pm _gen_real()
> printf("%.2f", rand($max-0.1));

Our database has decimal type columns with higher than 2 decimal places. This makes comparing structures with Test::More::is_deeply impossible when the other object is built by TestBuilder, and the other fetched from database, as seen here:

#     Structures begin differing at:
#          $got->[0]{amount} = '90203.710000'
#     $expected->[0]{amount} = '90203.71'
Comment 1 Lari Taskula 2021-02-04 19:41:13 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2021-02-04 19:41:16 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2021-02-04 20:58:23 UTC
There's something I'm missing. When column type is "decimal", DBIx returns the trailing decimal zeroes, but when it's "float", it returns only the numbers until the significant precision.

So if column size for decimals is 4, and the number being 1.0100, I get the following results from DBIx:
decimal: 1.0100
float: 1.01
Comment 4 Lari Taskula 2021-02-04 21:08:22 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2021-02-04 22:01:20 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2021-02-05 15:21:43 UTC
Could we re-fetch the generated TestBuilder object at the end of TestBuilder::_storeColumnValues() to make it identical with what Koha::Object / DBIx / DBI returns?
Comment 7 Lari Taskula 2021-02-05 16:26:38 UTC
(In reply to Lari Taskula from comment #6)
> Could we re-fetch the generated TestBuilder object at the end of
> TestBuilder::_storeColumnValues() to make it identical with what
> Koha::Object / DBIx / DBI returns?
Related to Bug 20621
Comment 8 Lari Taskula 2021-02-05 17:48:39 UTC
Created attachment 116426 [details] [review]
Bug 27630: Add tests

This proves different behavior between floats and decimals for values
built by TestBuilder vs values returned by DBI/DBIx.

To test:
1. prove t/db_dependent/TestBuilder.t
2. Observe failure

 Subtest: _gen_real formatting
        1..4
        not ok 1 - There can be more decimals when the column size allows it.
           Failed test 'There can be more decimals when the column size allows it.'
           at t/db_dependent/TestBuilder.t line 112.
                  got: '10.10'
             expected: '10.100000'
        not ok 2 - decimal - Builder builds same format as DBIx returns.
           Failed test 'decimal - Builder builds same format as DBIx returns.'
           at t/db_dependent/TestBuilder.t line 118.
                  got: '10.10'
             expected: '10.100000'
        not ok 3 - float - Builder builds same precision as DBIx returns.
           Failed test 'float - Builder builds same precision as DBIx returns.'
           at t/db_dependent/TestBuilder.t line 124.
                  got: '10.10'
             expected: '10.1'
        not ok 4 - double - Builder builds same precision as DBIx returns.
           Failed test 'double - Builder builds same precision as DBIx returns.'
           at t/db_dependent/TestBuilder.t line 131.
                  got: '10.10'
             expected: '10.1'
         Looks like you failed 4 tests of 4.
    not ok 4 - _gen_real formatting
       Failed test '_gen_real formatting'

Sponsored-by: The National Library of Finland
Comment 9 Lari Taskula 2021-02-05 17:48:42 UTC
Created attachment 116427 [details] [review]
Bug 27630: Update database generated values and format in TestBuilder

To test:
1. prove t/db_dependent/TestBuilder.t
2. Observe success

TODO: Enable the commented-out test when statistics table gets a primary key

Sponsored-by: The National Library of Finland
Comment 10 David Nind 2021-02-06 09:17:24 UTC
Created attachment 116440 [details] [review]
Bug 27630: Add tests

This proves different behavior between floats and decimals for values
built by TestBuilder vs values returned by DBI/DBIx.

To test:
1. prove t/db_dependent/TestBuilder.t
2. Observe failure

 Subtest: _gen_real formatting
        1..4
        not ok 1 - There can be more decimals when the column size allows it.
           Failed test 'There can be more decimals when the column size allows it.'
           at t/db_dependent/TestBuilder.t line 112.
                  got: '10.10'
             expected: '10.100000'
        not ok 2 - decimal - Builder builds same format as DBIx returns.
           Failed test 'decimal - Builder builds same format as DBIx returns.'
           at t/db_dependent/TestBuilder.t line 118.
                  got: '10.10'
             expected: '10.100000'
        not ok 3 - float - Builder builds same precision as DBIx returns.
           Failed test 'float - Builder builds same precision as DBIx returns.'
           at t/db_dependent/TestBuilder.t line 124.
                  got: '10.10'
             expected: '10.1'
        not ok 4 - double - Builder builds same precision as DBIx returns.
           Failed test 'double - Builder builds same precision as DBIx returns.'
           at t/db_dependent/TestBuilder.t line 131.
                  got: '10.10'
             expected: '10.1'
         Looks like you failed 4 tests of 4.
    not ok 4 - _gen_real formatting
       Failed test '_gen_real formatting'

Sponsored-by: The National Library of Finland

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2021-02-06 09:17:27 UTC
Created attachment 116441 [details] [review]
Bug 27630: Update database generated values and format in TestBuilder

To test:
1. prove t/db_dependent/TestBuilder.t
2. Observe success

TODO: Enable the commented-out test when statistics table gets a primary key

Sponsored-by: The National Library of Finland

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2021-02-06 09:18:35 UTC
Testing notes:
- Applied first patch and ran tests, then applied second patch and ran tests
- Tests pass as per the test plan
Comment 13 Lari Taskula 2021-02-08 18:07:53 UTC
Jonathan suggested using build_object() instead of build(). It works in most cases.

build_object() already refreshes the object because it executes a search on the given Koha-objects class, thus returning values in the same format that Koha::Objets->search/find would have.

I found only two DBIx schema files that have decimal fields with higher number of allowed decimal places than two, and no corresponding Koha::Object. These are AqbudgetsPlanning and PendingOfflineOperation. So there are very few cases where build_object would not be sufficient to solve the decimal formatting issues.

Additionally, as pointed out by Jonathan, we should be first study the side effects this patch would have on the total execution time of Koha test suite. I'm gonna mark this as Failed QA until we have those numbers.