Bug 17950

Summary: Small improvements for Merge.t
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Test SuiteAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, julian.maurice, katrin.fischer, kyle, mtompset
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14026, 17908    
Attachments: Bug 17950: Improve quick UNIMARC fix in Merge.t
Bug 17950: Merge.t: Improve variable handling at top level
Bug 17950: Merge.t: Improve variable handling at top level
Bug 17950: Improve quick UNIMARC fix in Merge.t
Bug 17950: Merge.t: Improve variable handling at top level
Bug 17950: Improve quick UNIMARC fix in Merge.t
Bug 17950: Merge.t: Improve variable handling at top level

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 Mark 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 Mark 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 Mark 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 Mark 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.