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>
Created attachment 59398 [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
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>
Julian: Would the second patch meet your concerns sufficiently?
(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
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>
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>
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}
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.
(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.
(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.
(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.
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>
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>
Pushed to master for 17.05, thanks Marcel!
These patches have been pushed to 16.11.x and will be in 16.11.04.