Created attachment 82116 [details] [review] Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Had to leave, will provide tests tomorrow
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 ;)
(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.
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.
(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.
Created attachment 82276 [details] [review] Bug 21798: Unify the creation of bibliographic record in tests Using the newly created subroutine
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
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".
Created attachment 82943 [details] [review] Bug 21798: Replace 2 more occurrences
create_biblio ? build_biblio ?
(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.
Created attachment 83136 [details] [review] Bug 21798: Replace 2 more occurrences
Created attachment 83137 [details] [review] Bug 21798: replace gimme_a_biblio with build_sample_biblio
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 :)
Applying: Bug 21798: Unify the creation of bibliographic record in tests fatal: sha1 information is lacking or useless (t/db_dependent/Circulation.t).
Created attachment 83149 [details] [review] Bug 21798: We need t::lib::TestBuilder::gimme_a_biblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 83150 [details] [review] Bug 21798: Unify the creation of bibliographic record in tests Using the newly created subroutine
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
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".
Created attachment 83153 [details] [review] Bug 21798: Replace 2 more occurrences
Created attachment 83154 [details] [review] Bug 21798: replace gimme_a_biblio with build_sample_biblio
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>
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.
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>
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>
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>
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>
Created attachment 83161 [details] [review] Bug 21798: Replace 2 more occurrences Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
Taking the liberty of passing QA in one run. Nick: Only affects test suite. Multiple rebasing avoided?
(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
(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
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>
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>
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>
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>
Created attachment 84494 [details] [review] Bug 21798: Replace 2 more occurrences Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
Awesome work all! Pushed to master for 19.05
Enhancement, will not be backported at this time to 18.11.x series.
Pushed to 18.11.x for 18.11.04 - This was required for bug 22140
lots of troubles applying this one to 18.05.x, not backporting to 18.05.x, enhancement