Created attachment 59005 [details] [review] Bug 17909: Add unit tests for C4::AuthoritiesMarc::merge Original patch from Julian Maurice on bug 11700. With sign offs by: Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Amended by Marcel de Rooy on report 17909. EDIT (January 2017): Removed some tests not related to merge. Put remaining tests in a subtest, made them working on current merge. Slightly revised the mocking. Note: I plan to move the zebra retrieval stuff outside merge in one of the next stages, and replace it by calling Koha::SearchEngine. This will reduce mocking complexity here. Test plan: Just run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59006 [details] [review] Bug 17909: Adding tests from bug 11315 Based on original patch from Maxime Beaulieu on bug 11315. Amended by Marcel de Rooy on report 17909. EDIT: Original tests have been adjusted in view of: [1] Test on bug 11315 heavily leaned on DBD::Mock. Since we are using Test::DBIx::Class on such tests now, this would need attention. Moreover, the advantage of mocking the database in this case is at least arguable. [2] Matching the first (somewhat older) subtest of 11700. [3] Simplification and readability. Look e.g. at the use of $MARCto and $MARCfrom on 11315. This made me merge them in the db_dependent counterpart. Also note that this subtest adds another needed test case: the merge from auth1 to modified auth1, while 11700 tested auth1 to auth2. Test plan: Just run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59007 [details] [review] Bug 17909: Add tests for merging with another authtype Originally aimed for bug 9988. Adjusted in line with other subtests. Will polish the three subtests a little more on a follow-up. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59008 [details] [review] Bug 17909: Additional polishing No spectacular things: [1] Move the framework modifications to a sub. Use same style to create auth types and linked fields. [2] Change some new Object occurrences to Object->new. [3] Add tests for field count and field order in the first two subsets. [4] Few whitespace changes (sorry) and comment lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Might be interesting for backporting if we manage to backport the fixes for merge too..
Created attachment 59011 [details] [review] [SIGNED-OFF] Bug 17909: Add unit tests for C4::AuthoritiesMarc::merge Original patch from Julian Maurice on bug 11700. With sign offs by: Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Amended by Marcel de Rooy on report 17909. EDIT (January 2017): Removed some tests not related to merge. Put remaining tests in a subtest, made them working on current merge. Slightly revised the mocking. Note: I plan to move the zebra retrieval stuff outside merge in one of the next stages, and replace it by calling Koha::SearchEngine. This will reduce mocking complexity here. Test plan: Just run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59012 [details] [review] [SIGNED-OFF] Bug 17909: Adding tests from bug 11315 Based on original patch from Maxime Beaulieu on bug 11315. Amended by Marcel de Rooy on report 17909. EDIT: Original tests have been adjusted in view of: [1] Test on bug 11315 heavily leaned on DBD::Mock. Since we are using Test::DBIx::Class on such tests now, this would need attention. Moreover, the advantage of mocking the database in this case is at least arguable. [2] Matching the first (somewhat older) subtest of 11700. [3] Simplification and readability. Look e.g. at the use of $MARCto and $MARCfrom on 11315. This made me merge them in the db_dependent counterpart. Also note that this subtest adds another needed test case: the merge from auth1 to modified auth1, while 11700 tested auth1 to auth2. Test plan: Just run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59013 [details] [review] [SIGNED-OFF] Bug 17909: Add tests for merging with another authtype Originally aimed for bug 9988. Adjusted in line with other subtests. Will polish the three subtests a little more on a follow-up. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59014 [details] [review] [SIGNED-OFF] Bug 17909: Additional polishing No spectacular things: [1] Move the framework modifications to a sub. Use same style to create auth types and linked fields. [2] Change some new Object occurrences to Object->new. [3] Add tests for field count and field order in the first two subsets. [4] Few whitespace changes (sorry) and comment lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59015 [details] [review] [SIGNED-OFF] Bug 17909: Followup - fix typo Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Thanks Josef for this very fast signoff
Note for QA: I am adding a linked field 612 as well as 712 on bug 17913 when refining the test on changed order.
Sorry, Josef. But while working on bug 17913 I came across two annoying typos that I want to correct here after all.
Created attachment 59077 [details] [review] Bug 17909: Followup - fix typos Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> EDIT: Adjusted three small typos that did not disturb the test in its current form, but do when we are fixing bugs on bug 17913.
(In reply to Marcel de Rooy from comment #13) > Sorry, Josef. But while working on bug 17913 I came across two annoying > typos that I want to correct here after all. Even three. Could you repeat your signoff?
(In reply to Marcel de Rooy from comment #15) > (In reply to Marcel de Rooy from comment #13) > > Sorry, Josef. But while working on bug 17913 I came across two annoying > > typos that I want to correct here after all. > > Even three. Could you repeat your signoff? No problem, working on it
Created attachment 59088 [details] [review] [SIGNED-OFF] Bug 17909: Followup - fix typos Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> EDIT: Adjusted three small typos that did not disturb the test in its current form, but do when we are fixing bugs on bug 17913. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59323 [details] [review] Bug 17909: [Follow-up] Quick fix for UNIMARC UNIMARC inserts field 100. This interferes the field count and order in the test. Note: This is a quick fix. Will polish it after bug 17913. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59324 [details] [review] Bug 17909: [Follow-up] Quick fix for UNIMARC UNIMARC inserts field 100. This interferes the field count and order in the test. Note: This is a quick fix. Will polish it after bug 17913. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59334 [details] [review] Bug 17909: Add unit tests for C4::AuthoritiesMarc::merge Original patch from Julian Maurice on bug 11700. With sign offs by: Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Amended by Marcel de Rooy on report 17909. EDIT (January 2017): Removed some tests not related to merge. Put remaining tests in a subtest, made them working on current merge. Slightly revised the mocking. Note: I plan to move the zebra retrieval stuff outside merge in one of the next stages, and replace it by calling Koha::SearchEngine. This will reduce mocking complexity here. Test plan: Just run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59335 [details] [review] Bug 17909: Adding tests from bug 11315 Based on original patch from Maxime Beaulieu on bug 11315. Amended by Marcel de Rooy on report 17909. EDIT: Original tests have been adjusted in view of: [1] Test on bug 11315 heavily leaned on DBD::Mock. Since we are using Test::DBIx::Class on such tests now, this would need attention. Moreover, the advantage of mocking the database in this case is at least arguable. [2] Matching the first (somewhat older) subtest of 11700. [3] Simplification and readability. Look e.g. at the use of $MARCto and $MARCfrom on 11315. This made me merge them in the db_dependent counterpart. Also note that this subtest adds another needed test case: the merge from auth1 to modified auth1, while 11700 tested auth1 to auth2. Test plan: Just run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59336 [details] [review] Bug 17909: Add tests for merging with another authtype Originally aimed for bug 9988. Adjusted in line with other subtests. Will polish the three subtests a little more on a follow-up. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59337 [details] [review] Bug 17909: Additional polishing No spectacular things: [1] Move the framework modifications to a sub. Use same style to create auth types and linked fields. [2] Change some new Object occurrences to Object->new. [3] Add tests for field count and field order in the first two subsets. [4] Few whitespace changes (sorry) and comment lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59338 [details] [review] Bug 17909: Followup - fix typos Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> EDIT: Adjusted three small typos that did not disturb the test in its current form, but do when we are fixing bugs on bug 17913. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59339 [details] [review] Bug 17909: [Follow-up] Quick fix for UNIMARC UNIMARC inserts field 100. This interferes the field count and order in the test. Note: This is a quick fix. Will polish it after bug 17913. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59340 [details] [review] Bug 17909: QA followup: remove unused var and move global var Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Thanks, Julian
Pushed to master for 17.05, thanks Julian, Maxime, Marcel, Josef!
These patches have been pushed to 16.11.x and will be in 16.11.02. .
Pushed to 16.05.x, for 16.05.08 release
Pushed to 3.22.x for 3.22.17