Bug 17733 - Members.t is still failing randomly
Summary: Members.t is still failing randomly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 17713
Blocks: 25551 17759
  Show dependency treegraph
 
Reported: 2016-12-06 16:08 UTC by Jonathan Druart
Modified: 2020-05-20 12:26 UTC (History)
3 users (show)

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


Attachments
Bug 17733: Fix Members.t (1.44 KB, patch)
2016-12-06 16:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 17733: Fix Members.t (1.51 KB, patch)
2016-12-06 20:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17733: Fix Members.t (1.64 KB, patch)
2016-12-12 11:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-12-06 16:08:13 UTC

    
Comment 1 Jonathan Druart 2016-12-06 16:12:43 UTC
Created attachment 58005 [details] [review]
Bug 17733: Fix Members.t

In some dirty DB (Jenkins), the borrowers.guarantorid field is not set
to a valid patron id.
That make some tests fail if we create a patron with that patron id.

In my DB, auto increment for borrowers is set to 52 before the tests
On the 4th run of the tests, some tests fail.
Before I got a patron with a guarantorid=112 (I let you do the math).

Test plan:
The easiest is to trust me

Todo: Add a FK!
Comment 2 Katrin Fischer 2016-12-06 20:09:49 UTC
Created attachment 58006 [details] [review]
[SIGNED OFF] Bug 17733: Fix Members.t

In some dirty DB (Jenkins), the borrowers.guarantorid field is not set
to a valid patron id.
That make some tests fail if we create a patron with that patron id.

In my DB, auto increment for borrowers is set to 52 before the tests
On the 4th run of the tests, some tests fail.
Before I got a patron with a guarantorid=112 (I let you do the math).

Test plan:
The easiest is to trust me

Todo: Add a FK!
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 Katrin Fischer 2016-12-06 20:10:22 UTC
We need this to make Jenkins go green! :)
Comment 4 Marcel de Rooy 2016-12-09 08:10:16 UTC
(In reply to Jonathan Druart from comment #1)
> Test plan:
> The easiest is to trust me

Best plan ever?
Comment 5 Marcel de Rooy 2016-12-09 08:28:41 UTC
Hmm.
I manipulated guarantorid of some borrower (with your calculation ;)
And the first time I ran the test, it failed:

not ok 65 - TrackLastPatronActivity - 2 patrons must be deleted
#   Failed test 'TrackLastPatronActivity - 2 patrons must be deleted'
#   at t/db_dependent/Members.t line 363.
#          got: '1'
#     expected: '2'
not ok 66 - TrackLastPatronActivity - 3 patrons must be deleted
#   Failed test 'TrackLastPatronActivity - 3 patrons must be deleted'
#   at t/db_dependent/Members.t line 365.
#          got: '2'
#     expected: '3'

As expected, the test did not fail when running it again.
So you need to remove some guarantorids here too. GetBorrowersToExpunge checks for this column.
Comment 6 Jonathan Druart 2016-12-12 10:30:22 UTC
This is something different.
I would not mind fixing it in the same patch (especially to avoid yet another bug report), but I do not recreate it and so it's hard to fix :)

Otherwise we could just set all the guarantorid to null
  Koha::Patrons->search->update( { guarantorid => null } );

That should do the job.
Comment 7 Marcel de Rooy 2016-12-12 10:41:48 UTC
(In reply to Jonathan Druart from comment #6)
> This is something different.
> I would not mind fixing it in the same patch (especially to avoid yet
> another bug report), but I do not recreate it and so it's hard to fix :)
> 
> Otherwise we could just set all the guarantorid to null
>   Koha::Patrons->search->update( { guarantorid => null } );
> 
> That should do the job.

Would not mind doing so outside sections where guarantorid is relevant.
Comment 8 Marcel de Rooy 2016-12-12 11:51:59 UTC
Moving the problem from comment5 to a new report: bug 17759
Comment 9 Marcel de Rooy 2016-12-12 11:54:03 UTC
Created attachment 58111 [details] [review]
Bug 17733: Fix Members.t

In some dirty DB (Jenkins), the borrowers.guarantorid field is not set
to a valid patron id.
That make some tests fail if we create a patron with that patron id.

In my DB, auto increment for borrowers is set to 52 before the tests
On the 4th run of the tests, some tests fail.
Before I got a patron with a guarantorid=112 (I let you do the math).

Test plan:
The easiest is to trust me

Todo: Add a FK!
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Opened a new report for special case: Bug 17759.
Comment 10 Kyle M Hall 2016-12-16 11:23:49 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Katrin Fischer 2016-12-18 20:30:36 UTC
This patch has been pushed to 16.11.x, will be in 16.11.01.