Bug 17742

Summary: Test t/db_dependent/Patrons.t can fail randomly
Product: Koha Reporter: Josef Moravec <josef.moravec>
Component: Test SuiteAssignee: Chris Cormack <chris>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: normal    
Priority: P5 - low CC: chris.kirby, jonathan.druart, katrin.fischer, kyle, mtj
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Attachments: Bug 17742: Fix t/db_dependent/Patrons.t
Bug 17742: Fix t/db_dependent/Patrons.t
Bug 17742: Fix t/db_dependent/Patrons.t
Bug 17742: Fix t/db_dependent/Patrons.t
Bug 17742: Fix t/db_dependent/Patrons.t
Bug 17742: Fix t/db_dependent/Patrons.t
Bug 17742: Use TestBuilder to create the library and patron category

Description Josef Moravec 2016-12-07 21:42:23 UTC
Tests on line 75 and 81 can sometimes, the failing line is:
is( dt_from_string($b1_new->updated_on), dt_from_string, "borrowers.updated_on should have been set to now on creating" );

And the problem could more often happen on slower machines...
We are testing the date and time of updated_on, but creating the test patrons, retrieving tested patron and test itself definitely takes some (short) time... which sometimes can be on the edge of a second and you can see this:

#   Failed test 'borrowers.updated_on should have been set to now on creating'
#   at t/db_dependent/Patrons.t line 75.
#          got: '2016-12-06T20:44:25'
#     expected: '2016-12-06T20:44:26'

and also, the both tests on line 75 and 81 are the same, which is not intended I think...
Comment 1 Josef Moravec 2016-12-07 22:00:42 UTC
Created attachment 58033 [details] [review]
Bug 17742: Fix t/db_dependent/Patrons.t

Moves the getting of testing date for updated_on just after the storing
the test patron data to make the gap between generating data and now
date as short as possible

Changes test #7 to the one which was probably originally intended

Test plan
1. prove t/db_dependent/Patrons.t
2. read the diff
Comment 2 Chris Kirby 2016-12-09 18:22:58 UTC
Created attachment 58085 [details] [review]
Bug 17742: Fix t/db_dependent/Patrons.t

Moves the getting of testing date for updated_on just after the storing
the test patron data to make the gap between generating data and now
date as short as possible

Changes test #7 to the one which was probably originally intended

Test plan
1. prove t/db_dependent/Patrons.t
2. read the diff

Signed-off-by: Chris Kirby <chris.kirby@ilsleypubliclibrary.org>
Tests passed.
Comment 3 Jonathan Druart 2016-12-12 11:08:03 UTC
(In reply to Chris Kirby from comment #2)
> Changes test #7 to the one which was probably originally intended

No, the original test was good. We want to check that the updated_on value has been updated when a patron is modified.
Comment 4 Josef Moravec 2016-12-12 13:04:55 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Chris Kirby from comment #2)
> > Changes test #7 to the one which was probably originally intended
> 
> No, the original test was good. We want to check that the updated_on value
> has been updated when a patron is modified.

But shouldn't it be tested on $b3_new?
Comment 5 Jonathan Druart 2016-12-12 13:28:27 UTC
(In reply to Josef Moravec from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > (In reply to Chris Kirby from comment #2)
> > > Changes test #7 to the one which was probably originally intended
> > 
> > No, the original test was good. We want to check that the updated_on value
> > has been updated when a patron is modified.
> 
> But shouldn't it be tested on $b3_new?

Yes it should.
Comment 6 Josef Moravec 2016-12-13 07:50:18 UTC
Created attachment 58148 [details] [review]
Bug 17742: Fix t/db_dependent/Patrons.t

Moves the getting of testing date for updated_on just after the storing
the test patron data to make the gap between generating data and now
date as short as possible

Fixes test 7

Use Koha::Database instead of C4::Context->dbh

Test plan
1. prove t/db_dependent/Patrons.t
2. read the diff

Signed-off-by: Chris Kirby <chris.kirby@ilsleypubliclibrary.org>
Tests passed.
Comment 7 Josef Moravec 2016-12-13 07:57:15 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Chris Kirby from comment #2)
> > Changes test #7 to the one which was probably originally intended
> 
> No, the original test was good. We want to check that the updated_on value
> has been updated when a patron is modified.

I change the back to original test, fix it (I hope), now it's tested on firstname, instead of surname, which is needed in later test... and maked the tests use Koha::Database instead of C4:Context->dbh;
Comment 8 Josef Moravec 2016-12-13 10:38:37 UTC
Created attachment 58150 [details] [review]
Bug 17742: Fix t/db_dependent/Patrons.t

Moves the getting of testing date for updated_on just after the storing
the test patron data to make the gap between generating data and now
date as short as possible

Fixes test 7

Use Koha::Database instead of C4::Context->dbh

Test plan
1. prove t/db_dependent/Patrons.t
2. read the diff
Comment 9 M. Tompsett 2016-12-27 23:28:59 UTC
Created attachment 58460 [details] [review]
Bug 17742: Fix t/db_dependent/Patrons.t

Moves the getting of testing date for updated_on just after the storing
the test patron data to make the gap between generating data and now
date as short as possible

Fixes test 7

Use Koha::Database instead of C4::Context->dbh

Test plan
1. prove t/db_dependent/Patrons.t
2. read the diff

NOTE: Only minor improvement could be using test builder to
      generate the category and branch codes, rather than assume
      data exists. However, that is beyond scope of this bug.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Jonathan Druart 2016-12-28 08:58:13 UTC
Created attachment 58462 [details] [review]
Bug 17742: Fix t/db_dependent/Patrons.t

Moves the getting of testing date for updated_on just after the storing
the test patron data to make the gap between generating data and now
date as short as possible

Fixes test 7

Use Koha::Database instead of C4::Context->dbh

Test plan
1. prove t/db_dependent/Patrons.t
2. read the diff

NOTE: Only minor improvement could be using test builder to
      generate the category and branch codes, rather than assume
      data exists. However, that is beyond scope of this bug.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 11 Jonathan Druart 2016-12-28 08:58:17 UTC
Created attachment 58463 [details] [review]
Bug 17742: Use TestBuilder to create the library and patron category

Moreover we do not need to remove the existing issues and patrons

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 Kyle M Hall 2016-12-28 13:34:31 UTC
Pushed to master for 17.05, thanks Josef, Jonathan!
Comment 13 Katrin Fischer 2017-01-04 07:27:32 UTC
These patches have been pushed to 16.11.x and will be in 16.11.02.
Comment 14 Mason James 2017-01-27 02:38:48 UTC
Pushed to 16.05.x, for 16.05.08 release