Bug 17950 - Small improvements for Merge.t
Summary: Small improvements for Merge.t
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Galen Charlton
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17908 14026
  Show dependency treegraph
 
Reported: 2017-01-20 13:10 UTC by Marcel de Rooy
Modified: 2017-06-14 22:18 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17950: Improve quick UNIMARC fix in Merge.t (7.81 KB, patch)
2017-01-23 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17950: Merge.t: Improve variable handling at top level (3.39 KB, patch)
2017-01-23 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17950: Merge.t: Improve variable handling at top level (3.44 KB, patch)
2017-01-23 10:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17950: Improve quick UNIMARC fix in Merge.t (7.82 KB, patch)
2017-01-31 17:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17950: Merge.t: Improve variable handling at top level (3.46 KB, patch)
2017-01-31 17:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17950: Improve quick UNIMARC fix in Merge.t (7.89 KB, patch)
2017-02-06 08:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17950: Merge.t: Improve variable handling at top level (3.52 KB, patch)
2017-02-06 08:37 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-20 13:10:08 UTC

    
Comment 1 Marcel de Rooy 2017-01-23 10:48:46 UTC
Created attachment 59397 [details] [review]
Bug 17950: Improve quick UNIMARC fix in Merge.t

Bug 17909 and 17913 added a quick fix for Merge.t on UNIMARC records.
This patch improves that fix with the sub compare_fields, a merge from
compare_field_count and compare_field_order.
Also it adds the option to test MARC21/UNIMARC by adding a command line
switch that triggers mocking the marcflavour preference.
The test on a cleared field 609 in strict mode has been broken up in two
tests: first a count without 609 and then counting 609s only.

Note: Could have mocked GetMarcBiblio too, but decided to go this way.

Test plan:
[1] Run perl t/db_dependent/Authorities/Merge.t
[2] (For UNIMARC users:) Run perl Merge.t -flavour MARC21
[3] (For others:) Run perl Merge.t -flavour UNIMARC

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2017-01-23 10:48:49 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-01-23 10:50:49 UTC
Created attachment 59399 [details] [review]
Bug 17950: Merge.t: Improve variable handling at top level

This patch does:
[1] Remove unused $dbh.
[2] Move all mocking statements to one sub.
[3] Promote a few lexicals to globals when used in the subtests.

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-23 10:52:29 UTC
Julian: Would the second patch meet your concerns sufficiently?
Comment 5 Julian Maurice 2017-01-23 11:29:36 UTC
(In reply to Marcel de Rooy from comment #4)
> Julian: Would the second patch meet your concerns sufficiently?

Absolutely :)
Having the mock objects initialized in the sub where they are used looks better IMO
Comment 6 M. Tompsett 2017-01-31 17:36:00 UTC
Created attachment 59689 [details] [review]
Bug 17950: Improve quick UNIMARC fix in Merge.t

Bug 17909 and 17913 added a quick fix for Merge.t on UNIMARC records.
This patch improves that fix with the sub compare_fields, a merge from
compare_field_count and compare_field_order.
Also it adds the option to test MARC21/UNIMARC by adding a command line
switch that triggers mocking the marcflavour preference.
The test on a cleared field 609 in strict mode has been broken up in two
tests: first a count without 609 and then counting 609s only.

Note: Could have mocked GetMarcBiblio too, but decided to go this way.

Test plan:
[1] Run perl t/db_dependent/Authorities/Merge.t
[2] (For UNIMARC users:) Run perl Merge.t -flavour MARC21
[3] (For others:) Run perl Merge.t -flavour UNIMARC

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

NOTE: prove -v t/db_dependent/Authorities/Merge.t :: -flavor {flavour}

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 M. Tompsett 2017-01-31 17:36:09 UTC
Created attachment 59690 [details] [review]
Bug 17950: Merge.t: Improve variable handling at top level

This patch does:
[1] Remove unused $dbh.
[2] Move all mocking statements to one sub.
[3] Promote a few lexicals to globals when used in the subtests.

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 M. Tompsett 2017-01-31 17:37:25 UTC
No need to run perl {testfile} you can use :: to pass parameters to the test file.
$ prove -v t/db_dependent/Authorities/Merge.t :: -flavor {flavour}
Comment 9 Julian Maurice 2017-02-03 16:08:44 UTC
I wonder if making a test script parameterizable is the way to go here.
Wouldn't it be cool to run the tests for each MARC flavour when you run prove ? :)

Not blocking for QA, just a thought.
Comment 10 Marcel de Rooy 2017-02-03 16:20:26 UTC
(In reply to Julian Maurice from comment #9)
> I wonder if making a test script parameterizable is the way to go here.
> Wouldn't it be cool to run the tests for each MARC flavour when you run
> prove ? :)
> 
> Not blocking for QA, just a thought.

Yes, we could to that. But I prefer to postpone it to another report.
Comment 11 M. Tompsett 2017-02-03 16:23:56 UTC
(In reply to Julian Maurice from comment #9)
> I wonder if making a test script parameterizable is the way to go here.

It is the most expandable, in terms of we don't know what marcflavors will be valid in the future, and writing code such that you have to change it all the time is kind of annoying.

Plus, I agree with Marcel in comment #10.
Comment 12 Julian Maurice 2017-02-06 08:29:32 UTC
(In reply to M. Tompsett from comment #11)
> It is the most expandable, in terms of we don't know what marcflavors will
> be valid in the future, and writing code such that you have to change it all
> the time is kind of annoying.

The list of MARC flavours doesn't change all the time. Plus we have the full list available from systempreferences.options.

Anyway, as I said it's not blocking so I'll do QA now.
Comment 13 Julian Maurice 2017-02-06 08:37:01 UTC
Created attachment 59917 [details] [review]
Bug 17950: Improve quick UNIMARC fix in Merge.t

Bug 17909 and 17913 added a quick fix for Merge.t on UNIMARC records.
This patch improves that fix with the sub compare_fields, a merge from
compare_field_count and compare_field_order.
Also it adds the option to test MARC21/UNIMARC by adding a command line
switch that triggers mocking the marcflavour preference.
The test on a cleared field 609 in strict mode has been broken up in two
tests: first a count without 609 and then counting 609s only.

Note: Could have mocked GetMarcBiblio too, but decided to go this way.

Test plan:
[1] Run perl t/db_dependent/Authorities/Merge.t
[2] (For UNIMARC users:) Run perl Merge.t -flavour MARC21
[3] (For others:) Run perl Merge.t -flavour UNIMARC

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 14 Julian Maurice 2017-02-06 08:37:05 UTC
Created attachment 59918 [details] [review]
Bug 17950: Merge.t: Improve variable handling at top level

This patch does:
[1] Remove unused $dbh.
[2] Move all mocking statements to one sub.
[3] Promote a few lexicals to globals when used in the subtests.

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 15 Kyle M Hall 2017-02-17 11:42:48 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 16 Katrin Fischer 2017-02-19 20:19:22 UTC
These patches have been pushed to 16.11.x and will be in 16.11.04.