Bug 17909 - Add unit tests for authority merge
Summary: Add unit tests for authority merge
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Julian Maurice
URL:
Keywords:
Depends on:
Blocks: 17908 17913
  Show dependency treegraph
 
Reported: 2017-01-14 10:59 UTC by Marcel de Rooy
Modified: 2018-06-04 20:10 UTC (History)
6 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 17909: Add unit tests for C4::AuthoritiesMarc::merge (5.74 KB, patch)
2017-01-16 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17909: Adding tests from bug 11315 (4.00 KB, patch)
2017-01-16 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17909: Add tests for merging with another authtype (6.09 KB, patch)
2017-01-16 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17909: Additional polishing (12.24 KB, patch)
2017-01-16 12:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17909: Add unit tests for C4::AuthoritiesMarc::merge (5.77 KB, patch)
2017-01-16 13:05 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17909: Adding tests from bug 11315 (4.03 KB, patch)
2017-01-16 13:05 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17909: Add tests for merging with another authtype (6.12 KB, patch)
2017-01-16 13:05 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17909: Additional polishing (12.27 KB, patch)
2017-01-16 13:05 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17909: Followup - fix typo (868 bytes, patch)
2017-01-16 13:05 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17909: Followup - fix typos (2.21 KB, patch)
2017-01-17 09:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17909: Followup - fix typos (2.24 KB, patch)
2017-01-17 12:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17909: [Follow-up] Quick fix for UNIMARC (2.77 KB, patch)
2017-01-20 10:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17909: [Follow-up] Quick fix for UNIMARC (3.11 KB, patch)
2017-01-20 10:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17909: Add unit tests for C4::AuthoritiesMarc::merge (5.82 KB, patch)
2017-01-20 11:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17909: Adding tests from bug 11315 (4.08 KB, patch)
2017-01-20 11:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17909: Add tests for merging with another authtype (6.17 KB, patch)
2017-01-20 11:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17909: Additional polishing (12.32 KB, patch)
2017-01-20 11:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17909: Followup - fix typos (2.29 KB, patch)
2017-01-20 11:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17909: [Follow-up] Quick fix for UNIMARC (3.13 KB, patch)
2017-01-20 11:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17909: QA followup: remove unused var and move global var (1.28 KB, patch)
2017-01-20 11:25 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-14 10:59:51 UTC

    
Comment 1 Marcel de Rooy 2017-01-16 12:04:12 UTC
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>
Comment 2 Marcel de Rooy 2017-01-16 12:04:16 UTC
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>
Comment 3 Marcel de Rooy 2017-01-16 12:04:19 UTC
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>
Comment 4 Marcel de Rooy 2017-01-16 12:04:23 UTC
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>
Comment 5 Marcel de Rooy 2017-01-16 12:05:25 UTC
Might be interesting for backporting if we manage to backport the fixes for merge too..
Comment 6 Josef Moravec 2017-01-16 13:05:06 UTC
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>
Comment 7 Josef Moravec 2017-01-16 13:05:16 UTC
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>
Comment 8 Josef Moravec 2017-01-16 13:05:25 UTC
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>
Comment 9 Josef Moravec 2017-01-16 13:05:33 UTC
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>
Comment 10 Josef Moravec 2017-01-16 13:05:37 UTC
Created attachment 59015 [details] [review]
[SIGNED-OFF] Bug 17909: Followup - fix typo

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Marcel de Rooy 2017-01-16 13:10:28 UTC
Thanks Josef for this very fast signoff
Comment 12 Marcel de Rooy 2017-01-16 14:02:05 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-01-17 09:31:12 UTC
Sorry, Josef. But while working on bug 17913 I came across two annoying typos that I want to correct here after all.
Comment 14 Marcel de Rooy 2017-01-17 09:38:50 UTC
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.
Comment 15 Marcel de Rooy 2017-01-17 09:39:20 UTC
(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?
Comment 16 Josef Moravec 2017-01-17 12:12:11 UTC
(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
Comment 17 Josef Moravec 2017-01-17 12:14:35 UTC
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>
Comment 18 Marcel de Rooy 2017-01-20 10:31:07 UTC
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>
Comment 19 Marcel de Rooy 2017-01-20 10:38:54 UTC
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>
Comment 20 Julian Maurice 2017-01-20 11:24:36 UTC
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>
Comment 21 Julian Maurice 2017-01-20 11:24:46 UTC
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>
Comment 22 Julian Maurice 2017-01-20 11:24:55 UTC
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>
Comment 23 Julian Maurice 2017-01-20 11:25:05 UTC
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>
Comment 24 Julian Maurice 2017-01-20 11:25:13 UTC
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>
Comment 25 Julian Maurice 2017-01-20 11:25:19 UTC
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>
Comment 26 Julian Maurice 2017-01-20 11:25:25 UTC
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>
Comment 27 Marcel de Rooy 2017-01-20 11:49:14 UTC
Thanks, Julian
Comment 28 Kyle M Hall 2017-01-20 13:53:38 UTC
Pushed to master for 17.05, thanks Julian, Maxime, Marcel, Josef!
Comment 29 Katrin Fischer 2017-01-22 19:59:35 UTC
These patches have been pushed to 16.11.x and will be in 16.11.02.
.
Comment 30 Mason James 2017-01-27 03:47:13 UTC
Pushed to 16.05.x, for 16.05.08 release
Comment 31 Julian Maurice 2017-02-20 10:50:22 UTC
Pushed to 3.22.x for 3.22.17