Bug 28632 - patrons.t fragile on slow boxes
Summary: patrons.t fragile on slow boxes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 25551 29884
  Show dependency treegraph
 
Reported: 2021-06-25 15:23 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
7 users (show)

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


Attachments
Bug 28632: Make patrons.t robust in terms of elapsed time between tests (3.23 KB, patch)
2021-06-25 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28632: Make patrons.t robust in terms of elapsed time between tests (3.28 KB, patch)
2021-06-26 00:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 28632: Make patrons.t robust in terms of elapsed time between tests (3.33 KB, patch)
2021-07-09 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28632: Prevent api/v1/patrons.t to fail on dates comparison (1.86 KB, patch)
2021-07-28 10:25 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 Tomás Cohen Arazi 2021-06-25 15:23:54 UTC
Playing some music and a Netflix movie on my (not that speedy) box, made t/db_dependent/api/v1/patrons.t fail consistently because of a 1 sec difference on the borrowers.updated_on attribute (set to current_timestamp on UPDATE).
Comment 1 Tomás Cohen Arazi 2021-06-25 15:30:50 UTC
Created attachment 122447 [details] [review]
Bug 28632: Make patrons.t robust in terms of elapsed time between tests

This patch changes the tests so they don't try to compare updated_on
from many tests ago, so it is less fragile.

I added a warn on the unmatched fields for convenience.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Make your computer very slow
3. Repeat 1
=> FAIL: You get things like:
t/db_dependent/api/v1/patrons.t .. 3/7
        #   Failed test 'Returned patron from update matches expected'
        #   at t/db_dependent/api/v1/patrons.t line 513.
        #     Structures begin differing at:
        #          $got->{updated_on} = '2021-06-25T14:26:20+00:00'
        #     $expected->{updated_on} = '2021-06-25T14:26:19+00:00'
        # Looks like you failed 1 test of 42.
4. Apply this patch
5. Repeat 1
=> SUCCESS: Tests pass again!
6. Sign off :-D
Comment 2 David Nind 2021-06-26 00:32:34 UTC
Created attachment 122460 [details] [review]
Bug 28632: Make patrons.t robust in terms of elapsed time between tests

This patch changes the tests so they don't try to compare updated_on
from many tests ago, so it is less fragile.

I added a warn on the unmatched fields for convenience.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Make your computer very slow
3. Repeat 1
=> FAIL: You get things like:
t/db_dependent/api/v1/patrons.t .. 3/7
        #   Failed test 'Returned patron from update matches expected'
        #   at t/db_dependent/api/v1/patrons.t line 513.
        #     Structures begin differing at:
        #          $got->{updated_on} = '2021-06-25T14:26:20+00:00'
        #     $expected->{updated_on} = '2021-06-25T14:26:19+00:00'
        # Looks like you failed 1 test of 42.
4. Apply this patch
5. Repeat 1
=> SUCCESS: Tests pass again!
6. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2021-06-26 00:34:54 UTC
Testing notes (koha-testing-docker):
- I can normally get things to slow right down by playing YouTube, using koha-testing-docker, and then opening another browser or two (such as Google Chrome)
- Managed to do this for this bug before (got tests to fail) and after (didn't manage to get it to fail)
- (not very scientific!)
Comment 4 Nick Clemens 2021-07-09 12:41:35 UTC
Created attachment 122739 [details] [review]
Bug 28632: Make patrons.t robust in terms of elapsed time between tests

This patch changes the tests so they don't try to compare updated_on
from many tests ago, so it is less fragile.

I added a warn on the unmatched fields for convenience.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
2. Make your computer very slow
3. Repeat 1
=> FAIL: You get things like:
t/db_dependent/api/v1/patrons.t .. 3/7
        #   Failed test 'Returned patron from update matches expected'
        #   at t/db_dependent/api/v1/patrons.t line 513.
        #     Structures begin differing at:
        #          $got->{updated_on} = '2021-06-25T14:26:20+00:00'
        #     $expected->{updated_on} = '2021-06-25T14:26:19+00:00'
        # Looks like you failed 1 test of 42.
4. Apply this patch
5. Repeat 1
=> SUCCESS: Tests pass again!
6. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2021-07-09 12:42:16 UTC
I couldn't recreate failures but changes make sense and tests continue to pass
Comment 6 Jonathan Druart 2021-07-09 13:01:31 UTC
Why a warn? And why don't you just delete $result->{update_on} then compare with is_deeply?
Comment 7 Tomás Cohen Arazi 2021-07-13 13:04:29 UTC
(In reply to Jonathan Druart from comment #6)
> Why a warn? And why don't you just delete $result->{update_on} then compare
> with is_deeply?

What we want to test here is that the passed parameters are present on the response (and on the DB). Those parameters can change because we are basically using a Koha::Patron as a basis (there's a bug adding a new column to 'borrowers', for example) so it felt like we needed to loop through the passed parameters and see they all match with the response.

Maybe we could assign the miss-matched attributes to a string, and make the test compare to empty? What do you think?
Comment 8 Jonathan Druart 2021-07-16 09:24:46 UTC
We could have a t::lib::Compare module (we already have t::lib::Dates::compare), with a t::lib::Compare::hashrefs subroutine.

It will deeply compare the 2 hashrefs passed in parameter. With a 'skip' option we could pass a list of keys to skip.
As it does not answer your need, we may better have a 'datatype' option with a list of key to treat as dates.

t::lib::Compare::hashrefs($hashref1, $hashref2, { datatype => { updated_on => 'date' } } )

We loop over the keys and when we find 'updated_on' we compare using t::lib::Compare::Dates.

What do you think?
Comment 9 Jonathan Druart 2021-07-26 08:49:31 UTC
Tomas?
Comment 10 Jonathan Druart 2021-07-28 10:25:49 UTC
Created attachment 123245 [details] [review]
Bug 28632: Prevent api/v1/patrons.t to fail on dates comparison

#   Failed test 'Returned patron from update matches expected'
 #   at t/db_dependent/api/v1/patrons.t line 537.
 #     Structures begin differing at:
 #          $got->{updated_on} = '2021-07-27T13:33:53+00:00'
 #     $expected->{updated_on} = '2021-07-27T13:33:52+00:00'
 # Looks like you failed 1 test of 42.
Comment 11 Jonathan Druart 2021-07-28 10:26:22 UTC
I think this patch is simply doing what we want to do, Tomas do you confirm?
Comment 12 Jonathan Druart 2021-07-30 05:19:16 UTC
(In reply to Jonathan Druart from comment #11)
> I think this patch is simply doing what we want to do, Tomas do you confirm?

Assuming it's a "yes".
Comment 13 Jonathan Druart 2021-07-30 05:22:30 UTC
Patch pushed to master for 21.11.00
Comment 14 Kyle M Hall 2021-07-30 17:57:20 UTC
Pushed to 21.05.x for 21.05.03
Comment 15 Fridolin Somers 2021-08-03 04:59:03 UTC
Pushed to 20.11.x for 20.11.09
Comment 16 Victor Grousset/tuxayo 2021-08-16 19:11:28 UTC
Backported: Pushed to 20.05.x branch for 20.05.15
Comment 17 wainuiwitikapark 2021-08-17 00:34:44 UTC
Should this be backported to 19.11.x for 19.11.21?

Or if it contains string changes, backported to 19.11.22? As we are currently in a string freeze.
Comment 18 wainuiwitikapark 2021-09-07 01:47:54 UTC
Should this be backported to 19.11.x for 19.11.22?
Comment 19 Jonathan Druart 2021-09-07 09:03:55 UTC
(In reply to wainuiwitikapark from comment #18)
> Should this be backported to 19.11.x for 19.11.22?

If it applies, yes please.
Comment 20 wainuiwitikapark 2021-09-08 02:31:59 UTC
Backported to 19.11.x for 19.11.22