Bug 25638 - API related tests failing on comparing floats
Summary: API related tests failing on comparing floats
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Tomás Cohen Arazi
URL: https://github.com/mojolicious/json-v...
Keywords:
Depends on: 25513
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-01 09:01 UTC by Jonathan Druart
Modified: 2020-09-08 09:35 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.01


Attachments
Bug 25638: Revert "Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON" (1.17 KB, patch)
2020-06-02 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25638: Fix floag/decimal comparison in API related tests (2.16 KB, patch)
2020-06-02 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25638: Revert "Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON" (1.23 KB, patch)
2020-06-05 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25638: Fix floag/decimal comparison in API related tests (2.22 KB, patch)
2020-06-05 12:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25638: Fix failure on Koha/Object.t (1.03 KB, patch)
2020-06-05 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25638: Add tests for JSON conversion vs DBD::mysql (1.92 KB, patch)
2020-06-05 18:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25638: Revert "Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON" (1.29 KB, patch)
2020-06-05 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25638: Fix float/decimal comparison in API related tests (2.29 KB, patch)
2020-06-05 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25638: Fix regression (1.27 KB, patch)
2020-06-05 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25638: Update TODO (2.13 KB, patch)
2020-06-05 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25638: Make the tests pass - ignoring the actual issue (2.95 KB, patch)
2020-06-09 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-06-01 09:01:30 UTC
Failing on almost all jobs currently

23:42:46 koha_1       |     #   Failed test 'exact match for JSON Pointer ""'
23:42:46 koha_1       |     #   at t/db_dependent/api/v1/patrons_accounts.t line 95.
23:42:46 koha_1       |     #     Structures begin differing at:
23:42:46 koha_1       |     #          $got->{outstanding_debits}{lines}[0]{amount} = '50'
23:42:46 koha_1       |     #     $expected->{outstanding_debits}{lines}[0]{amount} = '50.000000'
23:42:46 koha_1       | 
23:42:46 koha_1       |     #   Failed test 'exact match for JSON Pointer ""'
23:42:46 koha_1       |     #   at t/db_dependent/api/v1/patrons_accounts.t line 139.
23:42:46 koha_1       |     #     Structures begin differing at:
23:42:46 koha_1       |     #          $got->{outstanding_credits}{lines}[0]{amount} = '-10'
23:42:46 koha_1       |     #     $expected->{outstanding_credits}{lines}[0]{amount} = '-10.000000'
23:42:46 koha_1       |     # Looks like you failed 2 tests of 12.
23:42:46 koha_1       | 
23:42:46 koha_1       | #   Failed test 'get_balance() tests'
23:42:46 koha_1       | #   at t/db_dependent/api/v1/patrons_accounts.t line 153.
23:42:47 koha_1       | # Looks like you failed 1 test of 2.
23:42:47 koha_1       | [21:42:47] t/db_dependent/api/v1/patrons_accounts.t ................................ 
23:42:47 koha_1       | Dubious, test returned 1 (wstat 256, 0x100)
23:42:47 koha_1       | Failed 1/2 subtests 

23:43:12 koha_1       | 
23:43:12 koha_1       |     #   Failed test 'SWAGGER3.3.2'
23:43:12 koha_1       |     #   at t/db_dependent/api/v1/acquisitions_orders.t line 129.
23:43:12 koha_1       |     #     Structures begin differing at:
23:43:12 koha_1       |     #          $got->{list_price} = '2.02412229767404e+21'
23:43:12 koha_1       |     #     $expected->{list_price} = '2024122297674040344576.000000'
23:43:14 koha_1       | 
23:43:14 koha_1       |     #   Failed test 'Number representation should be consistent'
23:43:14 koha_1       |     #   at t/db_dependent/api/v1/acquisitions_orders.t line 154.
23:43:14 koha_1       |     #     Structures begin differing at:
23:43:14 koha_1       |     #          $got->{unit_price_tax_included} = '2.32181072793363e+21'
23:43:14 koha_1       |     #     $expected->{unit_price_tax_included} = '2321810727933630742528.000000'
23:43:14 koha_1       |     # Looks like you failed 2 tests of 8.
23:43:14 koha_1       | 
23:43:14 koha_1       | #   Failed test 'get() tests'
23:43:14 koha_1       | #   at t/db_dependent/api/v1/acquisitions_orders.t line 158.
23:43:22 koha_1       | # Looks like you failed 1 test of 5.
23:43:22 koha_1       | [21:43:22] t/db_dependent/api/v1/acquisitions_orders.t ............................. 
23:43:22 koha_1       | Dubious, test returned 1 (wstat 256, 0x100)
23:43:22 koha_1       | Failed 1/5 subtests 

23:43:50 koha_1       | 
23:43:50 koha_1       |     #   Failed test 'SWAGGER3.3.2'
23:43:50 koha_1       |     #   at t/db_dependent/api/v1/items.t line 131.
23:43:50 koha_1       |     #     Structures begin differing at:
23:43:50 koha_1       |     #          $got->{purchase_price} = '742834.7'
23:43:50 koha_1       |     #     $expected->{purchase_price} = '742834.70'
23:43:50 koha_1       |     # Looks like you failed 1 test of 9.
23:43:50 koha_1       | 
23:43:50 koha_1       | #   Failed test 'get() tests'
23:43:50 koha_1       | #   at t/db_dependent/api/v1/items.t line 143.
23:43:50 koha_1       | # Looks like you failed 1 test of 2.
23:43:50 koha_1       | [21:43:50] t/db_dependent/api/v1/items.t
Comment 1 Tomás Cohen Arazi 2020-06-01 12:16:56 UTC
It is urgent because it makes noise, but it is not critical, as the resulting float representations on the API are valid. This is more of a way we test and how things are compared on the tests.
I will be spending time on trying to solve this.
I think we will end up reverting the removal of the Koha::Object->TO_JSON code, and adding a coercion on the decimal representation... But it is just a guess...
Comment 2 Jonathan Druart 2020-06-02 12:53:17 UTC
Created attachment 105484 [details] [review]
Bug 25638: Revert "Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON"

This reverts commit a79cf4150e8a08e743acc7d7147af91757d7a9b8.
Comment 3 Jonathan Druart 2020-06-02 12:53:25 UTC
Created attachment 105485 [details] [review]
Bug 25638: Fix floag/decimal comparison in API related tests

The previous fix from bug 25513 does not work in all versions we are
supporting.

Let's hack the float/decimal columns in the same way we did for integer.

prove t/db_dependent/api/v1/acquisitions_vendors.t t/db_dependent/api/v1/patrons_accounts.t t/db_dependent/api/v1/acquisitions_orders.t

Note that there is another error on U20 for acquisitions_orders.t
Comment 4 Martin Renvoize 2020-06-05 12:38:59 UTC
Created attachment 105582 [details] [review]
Bug 25638: Revert "Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON"

This reverts commit a79cf4150e8a08e743acc7d7147af91757d7a9b8.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-06-05 12:39:03 UTC
Created attachment 105583 [details] [review]
Bug 25638: Fix floag/decimal comparison in API related tests

The previous fix from bug 25513 does not work in all versions we are
supporting.

Let's hack the float/decimal columns in the same way we did for integer.

prove t/db_dependent/api/v1/acquisitions_vendors.t t/db_dependent/api/v1/patrons_accounts.t t/db_dependent/api/v1/acquisitions_orders.t

Note that there is another error on U20 for acquisitions_orders.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-06-05 12:40:00 UTC
Works as expected.. this is a reasonable interim measure whilst we work on converting to use MariaDB::DBD
Comment 7 Jonathan Druart 2020-06-05 14:09:59 UTC
Created attachment 105590 [details] [review]
Bug 25638: Fix failure on Koha/Object.t
Comment 8 Tomás Cohen Arazi 2020-06-05 18:55:56 UTC
Created attachment 105595 [details] [review]
Bug 25638: Add tests for JSON conversion vs DBD::mysql

We have the situation of needing to cast numbers so they are not
confused in JSON with strings by DBD::mysql returning them with the
wrong internal flags.

We have also recently removed the cast, and adding it back now.
This test could help us detect this situation in case we make changes in
the area, and even on specific libraries versions.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2020-06-05 18:56:07 UTC
Created attachment 105596 [details] [review]
Bug 25638: Revert "Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON"

This reverts commit a79cf4150e8a08e743acc7d7147af91757d7a9b8.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2020-06-05 18:56:18 UTC
Created attachment 105597 [details] [review]
Bug 25638: Fix float/decimal comparison in API related tests

The previous fix from bug 25513 does not work in all versions we are
supporting.

Let's hack the float/decimal columns in the same way we did for integer.

prove t/db_dependent/api/v1/acquisitions_vendors.t \
      t/db_dependent/api/v1/patrons_accounts.t \
      t/db_dependent/api/v1/acquisitions_orders.t

Note that there is another error on U20 for acquisitions_orders.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2020-06-05 18:56:30 UTC
Created attachment 105598 [details] [review]
Bug 25638: Fix regression

Now that float/decimal columns are not treated similar to integer ones,
the default value handling we had set for numeric values is not trapping
the float/decimal cases as it should.

We love tests!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2020-06-05 18:56:41 UTC
Created attachment 105599 [details] [review]
Bug 25638: Update TODO

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Jonathan Druart 2020-06-07 10:03:25 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 14 Jonathan Druart 2020-06-09 14:16:18 UTC
Created attachment 105665 [details] [review]
Bug 25638: Make the tests pass - ignoring the actual issue

The API related tests still don't pass with the previous modifications.
They pass on D10 but fail on U18 and I did not manage to find where the
problems come from.
Comment 15 Jonathan Druart 2020-06-09 14:17:28 UTC
Pushing this last patch to turn Jenkins green and move on.

However this still need to be addressed in my opinion.
Comment 16 Lucas Gass 2020-06-16 21:55:29 UTC
All test pass! 

Backported to 20.05.x for 20.05.01
Comment 17 Aleisha Amohia 2020-08-17 22:13:01 UTC
missing dependencies, not backported to 19.11.x
Comment 18 Jonathan Druart 2020-09-08 09:35:20 UTC
(In reply to Jonathan Druart from comment #15)
> Pushing this last patch to turn Jenkins green and move on.
> 
> However this still need to be addressed in my opinion.

Additional work must be done on a separate bug report.