Bug 21798

Summary: We need t::lib::TestBuilder::build_sample_biblio
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, jonathan.druart, lucas, m.de.rooy, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 21816, 21971, 23821    
Attachments: Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio
Bug 21798: Unify the creation of bibliographic record in tests
Bug 21798: Fix a wrong test
Bug 21798: Fix another test
Bug 21798: Replace 2 more occurrences
Bug 21798: Replace 2 more occurrences
Bug 21798: replace gimme_a_biblio with build_sample_biblio
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio
Bug 21798: Unify the creation of bibliographic record in tests
Bug 21798: Fix a wrong test
Bug 21798: Fix another test
Bug 21798: Replace 2 more occurrences
Bug 21798: replace gimme_a_biblio with build_sample_biblio
Bug 21798: (QA follow-up) Fix Biblios.t
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio
Bug 21798: Unify the creation of bibliographic record in tests
Bug 21798: Fix a wrong test
Bug 21798: Fix another test
Bug 21798: Replace 2 more occurrences
Bug 21798: replace gimme_a_biblio with build_sample_biblio
Bug 21798: (QA follow-up) Fix Biblios.t
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio
Bug 21798: Unify the creation of bibliographic record in tests
Bug 21798: Fix a wrong test
Bug 21798: Fix another test
Bug 21798: Replace 2 more occurrences
Bug 21798: replace gimme_a_biblio with build_sample_biblio
Bug 21798: (QA follow-up) Fix Biblios.t

Description Tomás Cohen Arazi 2018-11-08 19:55:52 UTC

    
Comment 1 Tomás Cohen Arazi 2018-11-08 20:07:18 UTC
Created attachment 82116 [details] [review]
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2018-11-08 21:13:18 UTC
Had to leave, will provide tests tomorrow
Comment 3 Marcel de Rooy 2018-11-09 06:36:43 UTC
The word gimme is not conform our naming standards imo :)
And gimme_a_biblio should imo not be in TestBuilder. We can already get a biblio from TestBuilder. What you actually want is a simple MARC record in the metadata table. I think your code should follow that design and keep TestBuilder cleaner. Just my thought ;)
Comment 4 Tomás Cohen Arazi 2018-11-09 12:36:59 UTC
(In reply to Marcel de Rooy from comment #3)
> The word gimme is not conform our naming standards imo :)
> And gimme_a_biblio should imo not be in TestBuilder. We can already get a
> biblio from TestBuilder. What you actually want is a simple MARC record in
> the metadata table. I think your code should follow that design and keep
> TestBuilder cleaner. Just my thought ;)

I filed a bug to gather ideas on what to name the lib. I found it cool that we already have a $builder objecet in many test files, so
$builder->gimme_a_biblio seemed reasonable. I know neither the name or the place are fine. He.
Comment 5 Jonathan Druart 2018-11-12 21:09:57 UTC
I am quite ok to create such methods in TestBuilder.pm

This new method will replace the multiple occurrences of:
* add_biblio
* create_helper_biblio
* get_biblio


We also will need methods to generate:
* a logged in patron (with correct flag, library, userenv)
* a superlibrarian patron
* a MARC record
* an item that can be checked out (itemtype, onloan, withdrawn, etc.)
* a checkout
* etc.
Comment 6 Marcel de Rooy 2018-11-13 11:51:16 UTC
(In reply to Jonathan Druart from comment #5)
> I am quite ok to create such methods in TestBuilder.pm
> 
> This new method will replace the multiple occurrences of:
> * add_biblio
> * create_helper_biblio
> * get_biblio
> 
> 
> We also will need methods to generate:
> * a logged in patron (with correct flag, library, userenv)
> * a superlibrarian patron
> * a MARC record
> * an item that can be checked out (itemtype, onloan, withdrawn, etc.)
> * a checkout
> * etc.

Or create a TestBuilder dependent module? You want an interface between the test and TestBuilder in order to shorten TestBuilder calls.
Comment 7 Jonathan Druart 2018-11-13 14:38:14 UTC
Created attachment 82276 [details] [review]
Bug 21798: Unify the creation of bibliographic record in tests

Using the newly created subroutine
Comment 8 Jonathan Druart 2018-11-13 14:38:18 UTC
Created attachment 82277 [details] [review]
Bug 21798: Fix a wrong test

Now biblioitem.itemtype has a value, GetItem should fill itype with this
value if item-level_itypes it not set
Comment 9 Jonathan Druart 2018-11-13 14:38:23 UTC
Created attachment 82278 [details] [review]
Bug 21798: Fix another test

We should correctly generate the data we need, here we expect 2 records
with title starting with "Silence in the".
Comment 10 Jonathan Druart 2018-12-06 21:02:20 UTC
Created attachment 82943 [details] [review]
Bug 21798: Replace 2 more occurrences
Comment 11 Marcel de Rooy 2018-12-10 09:46:15 UTC
create_biblio ?
build_biblio ?
Comment 12 Marcel de Rooy 2018-12-12 16:02:08 UTC
(In reply to Marcel de Rooy from comment #11)
> create_biblio ?
> build_biblio ?

Or perhaps is build_sample_biblio even better on second thought. It conveys also that it is a sample.
Comment 13 Jonathan Druart 2018-12-12 16:17:27 UTC
Created attachment 83136 [details] [review]
Bug 21798: Replace 2 more occurrences
Comment 14 Jonathan Druart 2018-12-12 16:17:31 UTC
Created attachment 83137 [details] [review]
Bug 21798: replace gimme_a_biblio with build_sample_biblio
Comment 15 Jonathan Druart 2018-12-12 16:18:20 UTC
I can replace gimme_a_biblio with whatever you want in 5 secondes. However it will take much longer to rebase the patch given its size :)
Comment 16 Marcel de Rooy 2018-12-13 07:33:52 UTC
Applying: Bug 21798: Unify the creation of bibliographic record in tests
fatal: sha1 information is lacking or useless (t/db_dependent/Circulation.t).
Comment 17 Jonathan Druart 2018-12-13 12:42:52 UTC
Created attachment 83149 [details] [review]
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Jonathan Druart 2018-12-13 12:42:57 UTC
Created attachment 83150 [details] [review]
Bug 21798: Unify the creation of bibliographic record in tests

Using the newly created subroutine
Comment 19 Jonathan Druart 2018-12-13 12:43:01 UTC
Created attachment 83151 [details] [review]
Bug 21798: Fix a wrong test

Now biblioitem.itemtype has a value, GetItem should fill itype with this
value if item-level_itypes it not set
Comment 20 Jonathan Druart 2018-12-13 12:43:05 UTC
Created attachment 83152 [details] [review]
Bug 21798: Fix another test

We should correctly generate the data we need, here we expect 2 records
with title starting with "Silence in the".
Comment 21 Jonathan Druart 2018-12-13 12:43:09 UTC
Created attachment 83153 [details] [review]
Bug 21798: Replace 2 more occurrences
Comment 22 Jonathan Druart 2018-12-13 12:43:14 UTC
Created attachment 83154 [details] [review]
Bug 21798: replace gimme_a_biblio with build_sample_biblio
Comment 23 Marcel de Rooy 2018-12-13 12:52:13 UTC
Created attachment 83156 [details] [review]
Bug 21798: (QA follow-up) Fix Biblios.t

Global symbol "$bibnum" requires explicit package name

Test plan:
Run test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2018-12-13 12:59:39 UTC
This fails with me (with and without your patches):

    not ok 4 - Item on hold skipped, count makes sense
    #   Failed test 'Item on hold skipped, count makes sense'
    #   at t/db_dependent/Items/GetItemsForInventory.t line 121.
    #          got: '11'
    #     expected: '12'

Might depend on some settings not made explicit in the script.
Comment 25 Marcel de Rooy 2018-12-13 13:06:08 UTC
Created attachment 83157 [details] [review]
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2018-12-13 13:06:13 UTC
Created attachment 83158 [details] [review]
Bug 21798: Unify the creation of bibliographic record in tests

Using the newly created subroutine

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2018-12-13 13:06:17 UTC
Created attachment 83159 [details] [review]
Bug 21798: Fix a wrong test

Now biblioitem.itemtype has a value, GetItem should fill itype with this
value if item-level_itypes it not set

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2018-12-13 13:06:22 UTC
Created attachment 83160 [details] [review]
Bug 21798: Fix another test

We should correctly generate the data we need, here we expect 2 records
with title starting with "Silence in the".

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2018-12-13 13:06:27 UTC
Created attachment 83161 [details] [review]
Bug 21798: Replace 2 more occurrences

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2018-12-13 13:06:32 UTC
Created attachment 83162 [details] [review]
Bug 21798: replace gimme_a_biblio with build_sample_biblio

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2018-12-13 13:06:37 UTC
Created attachment 83163 [details] [review]
Bug 21798: (QA follow-up) Fix Biblios.t

Global symbol "$bibnum" requires explicit package name

Test plan:
Run test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2018-12-13 13:13:38 UTC
Taking the liberty of passing QA in one run.
Nick: Only affects test suite. Multiple rebasing avoided?
Comment 33 Tomás Cohen Arazi 2018-12-13 13:21:34 UTC
(In reply to Marcel de Rooy from comment #32)
> Taking the liberty of passing QA in one run.
> Nick: Only affects test suite. Multiple rebasing avoided?

+1
Comment 34 Nick Clemens 2019-01-02 21:07:10 UTC
(In reply to Marcel de Rooy from comment #32)
> Taking the liberty of passing QA in one run.
> Nick: Only affects test suite. Multiple rebasing avoided?

It needs a simple rebase - do we not need rollback since bug 14334 - or did we just forgot to add a rollback for the schema? t/db_dependent/Serials.t
Comment 35 Tomás Cohen Arazi 2019-01-28 16:22:05 UTC
Created attachment 84490 [details] [review]
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Tomás Cohen Arazi 2019-01-28 16:22:09 UTC
Created attachment 84491 [details] [review]
Bug 21798: Unify the creation of bibliographic record in tests

Using the newly created subroutine

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Tomás Cohen Arazi 2019-01-28 16:22:13 UTC
Created attachment 84492 [details] [review]
Bug 21798: Fix a wrong test

Now biblioitem.itemtype has a value, GetItem should fill itype with this
value if item-level_itypes it not set

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Tomás Cohen Arazi 2019-01-28 16:22:17 UTC
Created attachment 84493 [details] [review]
Bug 21798: Fix another test

We should correctly generate the data we need, here we expect 2 records
with title starting with "Silence in the".

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Tomás Cohen Arazi 2019-01-28 16:22:21 UTC
Created attachment 84494 [details] [review]
Bug 21798: Replace 2 more occurrences

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Tomás Cohen Arazi 2019-01-28 16:22:25 UTC
Created attachment 84495 [details] [review]
Bug 21798: replace gimme_a_biblio with build_sample_biblio

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Tomás Cohen Arazi 2019-01-28 16:22:29 UTC
Created attachment 84496 [details] [review]
Bug 21798: (QA follow-up) Fix Biblios.t

Global symbol "$bibnum" requires explicit package name

Test plan:
Run test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Nick Clemens 2019-01-28 21:06:06 UTC
Awesome work all!

Pushed to master for 19.05
Comment 43 Martin Renvoize 2019-02-01 15:31:27 UTC
Enhancement, will not be backported at this time to 18.11.x series.
Comment 44 Martin Renvoize 2019-02-25 15:08:00 UTC
Pushed to 18.11.x for 18.11.04 - This was required for bug 22140
Comment 45 Lucas Gass 2019-03-05 21:23:31 UTC
lots of troubles applying this one to 18.05.x, not backporting to 18.05.x, enhancement