Bug 21798 - We need t::lib::TestBuilder::build_sample_biblio
Summary: We need t::lib::TestBuilder::build_sample_biblio
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 21816 21971 23821
  Show dependency treegraph
 
Reported: 2018-11-08 19:55 UTC by Tomás Cohen Arazi
Modified: 2020-06-04 20:32 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:


Attachments
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio (1.17 KB, patch)
2018-11-08 20:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: Unify the creation of bibliographic record in tests (68.03 KB, patch)
2018-11-13 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Fix a wrong test (1.09 KB, patch)
2018-11-13 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Fix another test (1.04 KB, patch)
2018-11-13 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Replace 2 more occurrences (12.79 KB, patch)
2018-12-06 21:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Replace 2 more occurrences (12.80 KB, patch)
2018-12-12 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: replace gimme_a_biblio with build_sample_biblio (21.07 KB, patch)
2018-12-12 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio (1.17 KB, patch)
2018-12-13 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Unify the creation of bibliographic record in tests (67.99 KB, patch)
2018-12-13 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Fix a wrong test (1.09 KB, patch)
2018-12-13 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Fix another test (1.04 KB, patch)
2018-12-13 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: Replace 2 more occurrences (12.80 KB, patch)
2018-12-13 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: replace gimme_a_biblio with build_sample_biblio (21.07 KB, patch)
2018-12-13 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21798: (QA follow-up) Fix Biblios.t (1.33 KB, patch)
2018-12-13 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio (1.26 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: Unify the creation of bibliographic record in tests (68.01 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: Fix a wrong test (1.18 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: Fix another test (1.13 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: Replace 2 more occurrences (12.89 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: replace gimme_a_biblio with build_sample_biblio (21.08 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: (QA follow-up) Fix Biblios.t (1.33 KB, patch)
2018-12-13 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio (1.23 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: Unify the creation of bibliographic record in tests (66.99 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: Fix a wrong test (1.15 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: Fix another test (1.10 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: Replace 2 more occurrences (12.83 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: replace gimme_a_biblio with build_sample_biblio (20.93 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21798: (QA follow-up) Fix Biblios.t (1.30 KB, patch)
2019-01-28 16:22 UTC, Tomás Cohen Arazi
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 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