Bugzilla – Attachment 48631 Details for
Bug 15871
Improve perl critic of t/RecordProcessor.t
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t
Bug-15871-Improve-PerlCritic-level-for-tRecordProc.patch (text/plain), 12.33 KB, created by
Tomás Cohen Arazi (tcohen)
on 2016-03-03 15:48:59 UTC
(
hide
)
Description:
Bug 15871: Improve PerlCritic level for t/RecordProcessor.t
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2016-03-03 15:48:59 UTC
Size:
12.33 KB
patch
obsolete
>From 101d878b3bfb97107dd452a954de7f0d4588ee78 Mon Sep 17 00:00:00 2001 >From: Mark Tompsett <mtompset@hotmail.com> >Date: Fri, 19 Feb 2016 15:49:02 -0500 >Subject: [PATCH] Bug 15871: Improve PerlCritic level for t/RecordProcessor.t > >perlcritic -5 failed. >Attempt to clean up to a higher level: >-- use English to address use of $@ variable >-- perltidy on the code >-- substitute q{} for '' >-- expand out single line hacky goodness (... s/\.pm$//) to more code >-- remove parenthesis on functions that don't need it >-- add x, s, and m as needed to regexps >-- change double quotes to single quotes where no variable involved >-- tweaked eval destroy test to check return value and use $EVAL_ERROR >-- renamed $processor to $record_processor in the subtest to avoid > lexical warnings > >TEST PLAN >--------- > >$ perlcritic -5 t/RecordProcessor.t >Don't modify $_ in list functions at line 43, column 25. See page 114 of PBP. (Severity: 5) > >$ perlcritic -2 t/RecordProcessor.t >No package-scoped "$VERSION" variable found at line 1, column 1. See page 404 of PBP. (Severity: 2) >Quotes used with a string containing no non-whitespace characters at line 34, column 36. See page 53 of PBP. (Severity: 2) >Quotes used with a string containing no non-whitespace characters at line 34, column 39. See page 53 of PBP. (Severity: 2) >Quotes used with a string containing no non-whitespace characters at line 36, column 33. See page 53 of PBP. (Severity: 2) >Quotes used with a string containing no non-whitespace characters at line 36, column 36. See page 53 of PBP. (Severity: 2) >Don't modify $_ in list functions at line 43, column 25. See page 114 of PBP. (Severity: 5) >Regular expression without "/s" flag at line 43, column 33. See pages 240,241 of PBP. (Severity: 2) >Regular expression without "/x" flag at line 43, column 33. See page 236 of PBP. (Severity: 3) >Regular expression without "/m" flag at line 43, column 33. See page 237 of PBP. (Severity: 2) >Regular expression without "/s" flag at line 43, column 66. See pages 240,241 of PBP. (Severity: 2) >Regular expression without "/x" flag at line 43, column 66. See page 236 of PBP. (Severity: 3) >Regular expression without "/m" flag at line 43, column 66. See page 237 of PBP. (Severity: 2) >Expression form of "grep" at line 47, column 8. See page 169 of PBP. (Severity: 4) >Expression form of "grep" at line 50, column 20. See page 169 of PBP. (Severity: 4) >Regular expression without "/s" flag at line 50, column 26. See pages 240,241 of PBP. (Severity: 2) >Regular expression without "/m" flag at line 50, column 26. See page 237 of PBP. (Severity: 2) >Return value of eval not tested at line 73, column 1. You can't depend upon the value of $@/$EVAL_ERROR to tell whether an eval failed. (Severity: 3) >Magic punctuation variable $@ used at line 78, column 5. See page 79 of PBP. (Severity: 2) >Reused variable name in lexical scope: $processor at line 84, column 5. Invent unique variable names. (Severity: 3) >Subroutine "new" called using indirect syntax at line 87, column 18. See page 349 of PBP. (Severity: 4) >Subroutine "new" called using indirect syntax at line 93, column 18. See page 349 of PBP. (Severity: 4) >Quotes used with a string containing no non-whitespace characters at line 96, column 40. See page 53 of PBP. (Severity: 2) >Subroutine "new" called using indirect syntax at line 99, column 18. See page 349 of PBP. (Severity: 4) >Subroutine "new" called using indirect syntax at line 106, column 18. See page 349 of PBP. (Severity: 4) >$ prove -v t/RecordProcessor.t >t/RecordProcessor.t .. ok >All tests successful. >Files=1, Tests=13, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.22 cusr 0.02 csys = 0.25 CPU) >Result: PASS > >$ prove -v t/RecordProcessor.t >... >$ git bz apply 15871 > >Repeat perlcritic level 2, and only $VERSION warning should exist. >Retest with the prove. >Run koha qa test tools. > >Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> >Works as advertised > >Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> >Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> >I don't really care about perlcritic as long as it involves changing '' into qw{} (WTF?) >Anyway, I'd do this kind of things as we go, for example, if we were adding more tests. In that >case it would just be a followup for this, after you provided a patch for an enh/bugfix. >--- > t/RecordProcessor.t | 124 +++++++++++++++++++++++++++++++++------------------- > 1 file changed, 78 insertions(+), 46 deletions(-) > >diff --git a/t/RecordProcessor.t b/t/RecordProcessor.t >index ad475d4..63683c1 100755 >--- a/t/RecordProcessor.t >+++ b/t/RecordProcessor.t >@@ -21,86 +21,117 @@ use Modern::Perl; > > use File::Spec; > use MARC::Record; >- >+use English qw( -no_match_vars ); > use Test::More; > > BEGIN { >- use_ok('Koha::RecordProcessor'); >+ use_ok('Koha::RecordProcessor'); > } > >-my $isbn = '0590353403'; >-my $title = 'Foundation'; >-my $marc_record=MARC::Record->new; >-my $field = MARC::Field->new('020','','','a' => $isbn); >+my $isbn = '0590353403'; >+my $title = 'Foundation'; >+my $marc_record = MARC::Record->new; >+my $field = MARC::Field->new( '020', q{}, q{}, 'a' => $isbn ); > $marc_record->append_fields($field); >-$field = MARC::Field->new('245','','','a' => $title); >+$field = MARC::Field->new( '245', q{}, q{}, 'a' => $title ); > $marc_record->append_fields($field); > >- > my $filterdir = File::Spec->rel2abs('Koha/Filter') . '/MARC'; > >-opendir(my $dh, $filterdir); >-my @installed_filters = map { ( /\.pm$/ && -f "$filterdir/$_" && s/\.pm$// ) ? "Koha::Filters::MARC::$_" : () } readdir($dh); >+my $dh; >+opendir $dh, $filterdir; >+my @installed_filters; >+my @directory_entries = readdir $dh; >+foreach my $entry (@directory_entries) { >+ if ( $entry =~ /[.]pm$/xsm && -f "$filterdir/$entry" ) { >+ my $filter_name = $entry; >+ $filter_name =~ s/[.]pm$//xsm; >+ push @installed_filters, $filter_name; >+ } >+} >+closedir $dh; > my @available_filters = Koha::RecordProcessor::AvailableFilters(); > > foreach my $filter (@installed_filters) { >- ok(grep($filter, @available_filters), "Found filter $filter"); >+ ok( grep { /${filter}/xsm } @available_filters, "Found filter $filter" ); > } > >-my $marc_filters = grep (/MARC/, @available_filters); >-is(scalar Koha::RecordProcessor::AvailableFilters('MARC'), $marc_filters, 'Retrieved list of MARC filters'); >+my $marc_filters = grep { /MARC/sm } @available_filters; >+is( scalar Koha::RecordProcessor::AvailableFilters('MARC'), >+ $marc_filters, 'Retrieved list of MARC filters' ); > >-my $processor = Koha::RecordProcessor->new( { filters => ( 'ABCD::EFGH::IJKL' ) } ); >+my $processor = >+ Koha::RecordProcessor->new( { filters => ('ABCD::EFGH::IJKL') } ); > >-is(ref($processor), 'Koha::RecordProcessor', 'Created record processor with invalid filter'); >+is( ref($processor), 'Koha::RecordProcessor', >+ 'Created record processor with invalid filter' ); > >-is($processor->process($marc_record), $marc_record, 'Process record with empty processor'); >+is( $processor->process($marc_record), >+ $marc_record, 'Process record with empty processor' ); > >-$processor = Koha::RecordProcessor->new( { filters => ( 'Null' ) } ); >-is(ref($processor->filters->[0]), 'Koha::Filter::MARC::Null', 'Created record processor with implicitly scoped Null filter'); >+$processor = Koha::RecordProcessor->new( { filters => ('Null') } ); >+is( ref( $processor->filters->[0] ), >+ 'Koha::Filter::MARC::Null', >+ 'Created record processor with implicitly scoped Null filter' ); > >-$processor = Koha::RecordProcessor->new( { filters => ( 'Koha::Filter::MARC::Null' ) } ); >-is(ref($processor->filters->[0]), 'Koha::Filter::MARC::Null', 'Created record processor with explicitly scoped Null filter'); >+$processor = >+ Koha::RecordProcessor->new( { filters => ('Koha::Filter::MARC::Null') } ); >+is( ref( $processor->filters->[0] ), >+ 'Koha::Filter::MARC::Null', >+ 'Created record processor with explicitly scoped Null filter' ); > >-is($processor->process($marc_record), $marc_record, 'Process record'); >+is( $processor->process($marc_record), $marc_record, 'Process record' ); > > $processor->bind($marc_record); > >-is($processor->record, $marc_record, 'Bound record to processor'); >+is( $processor->record, $marc_record, 'Bound record to processor' ); > >-is($processor->process(), $marc_record, 'Filter bound record'); >+is( $processor->process(), $marc_record, 'Filter bound record' ); > >-eval { >- $processor = Koha::RecordProcessor->new( { filters => ( 'Koha::Filter::MARC::Null' ) } ); >+my $destroy_test = eval { >+ $processor = >+ Koha::RecordProcessor->new( { filters => ('Koha::Filter::MARC::Null') } ); > undef $processor; >+ return 1; > }; > >-ok(!$@, 'Destroyed processor successfully'); >+ok( !$EVAL_ERROR && $destroy_test == 1, 'Destroyed processor successfully' ); > >-subtest "new() tests" => sub { >+subtest 'new() tests' => sub { > > plan tests => 14; > >- my $processor; >+ my $record_processor; > > # Create a processor with a valid filter >- $processor = new Koha::RecordProcessor({ filters => 'Null' }); >- is( ref($processor), 'Koha::RecordProcessor', 'Processor created' ); >- is( scalar @{ $processor->filters }, 1, 'One filter initialized' ); >- is( ref($processor->filters->[0]), 'Koha::Filter::MARC::Null', 'Correct filter initialized' ); >+ $record_processor = Koha::RecordProcessor->new( { filters => 'Null' } ); >+ is( ref($record_processor), 'Koha::RecordProcessor', 'Processor created' ); >+ is( scalar @{ $record_processor->filters }, 1, 'One filter initialized' ); >+ is( ref( $record_processor->filters->[0] ), >+ 'Koha::Filter::MARC::Null', 'Correct filter initialized' ); > > # Create a processor with an invalid filter >- $processor = new Koha::RecordProcessor({ filters => 'Dummy' }); >- is( ref($processor), 'Koha::RecordProcessor', 'Processor created' ); >- is( scalar @{ $processor->filters }, 0, 'No filter initialized' ); >- is( ref($processor->filters->[0]), '', 'Make sure no filter initialized' ); >+ $record_processor = Koha::RecordProcessor->new( { filters => 'Dummy' } ); >+ is( ref($record_processor), 'Koha::RecordProcessor', 'Processor created' ); >+ is( scalar @{ $record_processor->filters }, 0, 'No filter initialized' ); >+ is( ref( $record_processor->filters->[0] ), >+ q{}, 'Make sure no filter initialized' ); > > # Create a processor with two valid filters >- $processor = new Koha::RecordProcessor({ filters => [ 'Null', 'EmbedSeeFromHeadings' ] }); >- is( ref($processor), 'Koha::RecordProcessor', 'Processor created' ); >- is( scalar @{ $processor->filters }, 2, 'Two filters initialized' ); >- is( ref($processor->filters->[0]), 'Koha::Filter::MARC::Null', 'Correct first filter initialized' ); >- is( ref($processor->filters->[1]), 'Koha::Filter::MARC::EmbedSeeFromHeadings', 'Correct second filter initialized' ); >+ $record_processor = Koha::RecordProcessor->new( >+ { filters => [ 'Null', 'EmbedSeeFromHeadings' ] } ); >+ is( ref($record_processor), 'Koha::RecordProcessor', 'Processor created' ); >+ is( scalar @{ $record_processor->filters }, 2, 'Two filters initialized' ); >+ is( >+ ref( $record_processor->filters->[0] ), >+ 'Koha::Filter::MARC::Null', >+ 'Correct first filter initialized' >+ ); >+ is( >+ ref( $record_processor->filters->[1] ), >+ 'Koha::Filter::MARC::EmbedSeeFromHeadings', >+ 'Correct second filter initialized' >+ ); > > # Create a processor with both valid and invalid filters. > # use hash reference for regression testing >@@ -108,12 +139,13 @@ subtest "new() tests" => sub { > filters => [ 'Null', 'Dummy' ], > options => { 'test' => 'true' } > }; >- $processor = new Koha::RecordProcessor($parameters); >- is( ref($processor), 'Koha::RecordProcessor', 'Processor created' ); >- is( scalar @{ $processor->filters }, 1, 'Invalid filter skipped' ); >- is( ref($processor->filters->[0]), 'Koha::Filter::MARC::Null', 'Correct filter initialized' ); >+ $record_processor = Koha::RecordProcessor->new($parameters); >+ is( ref($record_processor), 'Koha::RecordProcessor', 'Processor created' ); >+ is( scalar @{ $record_processor->filters }, 1, 'Invalid filter skipped' ); >+ is( ref( $record_processor->filters->[0] ), >+ 'Koha::Filter::MARC::Null', 'Correct filter initialized' ); > >- my $filter_params = $processor->filters->[0]->params; >+ my $filter_params = $record_processor->filters->[0]->params; > is_deeply( $filter_params, $parameters, 'Initialization parameters' ); > }; > >-- >2.7.0
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=48631">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 15871
:
48255
|
48257
|
48325
|
48326
|
48336
|
48349
| 48631