From 4735f890212ea0426180a61d0a613e536b0a36bd Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 2 Aug 2017 13:01:47 +0200 Subject: [PATCH] Bug 18996: [QA Follow-up] Remove global variables from Message.t Content-Type: text/plain; charset=utf-8 As per request of Colin in comment18, this patch makes the use of global variables in Message.t no longer needed. The three subtests are now completely independent and could well be moved to separate test scripts. Note: Strictly speaking, the use of global (package) variables could potentially introduce new bugs (e.g. if the value is modified outside the script). This seems not to be the case here, but we are safe now. Test plan: Run the test again. Signed-off-by: Marcel de Rooy --- t/db_dependent/SIP/Message.t | 75 +++++++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/t/db_dependent/SIP/Message.t b/t/db_dependent/SIP/Message.t index 50b2ef2..12019c2 100755 --- a/t/db_dependent/SIP/Message.t +++ b/t/db_dependent/SIP/Message.t @@ -35,6 +35,7 @@ use Koha::DateUtils; use Koha::Items; use Koha::Checkouts; use Koha::Old::Checkouts; +use Koha::Patrons; use C4::SIP::ILS; use C4::SIP::ILS::Patron; @@ -44,29 +45,9 @@ use C4::SIP::Sip::MsgType; use constant PATRON_PW => 'do_not_ever_use_this_one'; -our $fixed_length = { #length of fixed fields including response code - ( PATRON_STATUS_RESP ) => 37, - ( PATRON_INFO_RESP ) => 61, - ( CHECKIN_RESP ) => 24, -}; - -our $schema = Koha::Database->new->schema; -our $builder = t::lib::TestBuilder->new(); - -# COMMON: Some common stuff for all/most subtests -our ( $response, $findpatron, $branchcode ); -$branchcode = $builder->build({ source => 'Branch' })->{branchcode}; -# mock write_msg (imported from Sip.pm into Message.pm) -my $mockMsg = Test::MockModule->new( 'C4::SIP::Sip::MsgType' ); -$mockMsg->mock( 'write_msg', sub { $response = $_[1]; } ); # save response -# mock ils object -our $mockILS = Test::MockObject->new; -$mockILS->mock( 'check_inst_id', sub {} ); -$mockILS->mock( 'institution_id', sub { $branchcode; } ); -$mockILS->mock( 'find_patron', sub { $findpatron; } ); - # START testing subtest 'Testing Patron Status Request V2' => sub { + my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; plan tests => 13; $C4::SIP::Sip::protocol_version = 2; @@ -75,6 +56,7 @@ subtest 'Testing Patron Status Request V2' => sub { }; subtest 'Testing Patron Info Request V2' => sub { + my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; plan tests => 18; $C4::SIP::Sip::protocol_version = 2; @@ -83,6 +65,7 @@ subtest 'Testing Patron Info Request V2' => sub { }; subtest 'Checkin V2' => sub { + my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; plan tests => 21; $C4::SIP::Sip::protocol_version = 2; @@ -95,6 +78,11 @@ subtest 'Checkin V2' => sub { # END of main code sub test_request_patron_status_v2 { + my $builder = t::lib::TestBuilder->new(); + my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; + my ( $response, $findpatron ); + my $mocks = create_mocks( \$response, \$findpatron, \$branchcode ); + my $patron1 = $builder->build({ source => 'Borrower', value => { @@ -111,7 +99,7 @@ sub test_request_patron_status_v2 { FID_PATRON_PWD. PATRON_PW. '|'; my $msg = C4::SIP::Sip::MsgType->new( $siprequest, 0 ); - my $server = { ils => $mockILS }; + my $server = { ils => $mocks->{ils} }; undef $response; $msg->handle_patron_status( $server ); @@ -150,7 +138,7 @@ sub test_request_patron_status_v2 { # Finally, we send a wrong card number and check AE, BL # This is done by removing the new patron first - $schema->resultset('Borrower')->search({ cardnumber => $card1 })->delete; + Koha::Patrons->search({ cardnumber => $card1 })->delete; undef $findpatron; $siprequest = PATRON_STATUS_REQ. 'engYYYYMMDDZZZZHHMMSS'. FID_INST_ID. $branchcode. '|'. @@ -166,6 +154,11 @@ sub test_request_patron_status_v2 { } sub test_request_patron_info_v2 { + my $builder = t::lib::TestBuilder->new(); + my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; + my ( $response, $findpatron ); + my $mocks = create_mocks( \$response, \$findpatron, \$branchcode ); + my $patron2 = $builder->build({ source => 'Borrower', value => { @@ -181,7 +174,7 @@ sub test_request_patron_info_v2 { FID_PATRON_PWD. PATRON_PW. '|'; my $msg = C4::SIP::Sip::MsgType->new( $siprequest, 0 ); - my $server = { ils => $mockILS }; + my $server = { ils => $mocks->{ils} }; undef $response; $msg->handle_patron_info( $server ); isnt( $response, undef, 'At least we got a response.' ); @@ -228,7 +221,7 @@ sub test_request_patron_info_v2 { check_field( $respcode, $response, FID_VALID_PATRON_PWD, 'Y', 'code CQ should be Y if empty AD allowed' ); # Finally, we send a wrong card number - $schema->resultset('Borrower')->search({ cardnumber => $card })->delete; + Koha::Patrons->search({ cardnumber => $card })->delete; undef $findpatron; $msg = C4::SIP::Sip::MsgType->new( $siprequest, 0 ); undef $response; @@ -240,6 +233,11 @@ sub test_request_patron_info_v2 { } sub test_checkin_v2 { + my $builder = t::lib::TestBuilder->new(); + my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; + my ( $response, $findpatron ); + my $mocks = create_mocks( \$response, \$findpatron, \$branchcode ); + # create some data my $patron1 = $builder->build({ source => 'Borrower', @@ -255,6 +253,7 @@ sub test_checkin_v2 { value => { damaged => 0, withdrawn => 0, itemlost => 0, restricted => 0, homebranch => $branchcode, holdingbranch => $branchcode }, }); + my $mockILS = $mocks->{ils}; my $server = { ils => $mockILS, account => {} }; $mockILS->mock( 'institution', sub { $branchcode; } ); $mockILS->mock( 'supports', sub { return; } ); @@ -342,12 +341,28 @@ sub test_checkin_v2 { # Helper routines +sub create_mocks { + my ( $response, $findpatron, $branchcode ) = @_; # referenced variables ! + + # mock write_msg (imported from Sip.pm into Message.pm) + my $mockMsg = Test::MockModule->new( 'C4::SIP::Sip::MsgType' ); + $mockMsg->mock( 'write_msg', sub { $$response = $_[1]; } ); # save response + + # mock ils object + my $mockILS = Test::MockObject->new; + $mockILS->mock( 'check_inst_id', sub {} ); + $mockILS->mock( 'institution_id', sub { $$branchcode; } ); + $mockILS->mock( 'find_patron', sub { $$findpatron; } ); + + return { ils => $mockILS, message => $mockMsg }; +} + sub check_field { my ( $code, $resp, $fld, $expr, $msg, $mode ) = @_; # mode: contains || equals || regex (by default: equals) # strip fixed part; prefix to simplify next regex - $resp = '|'. substr( $resp, $fixed_length->{$code} ); + $resp = '|'. substr( $resp, fixed_length( $code ) ); my $fldval; if( $resp =~ /\|$fld([^\|]*)\|/ ) { $fldval = $1; @@ -372,3 +387,11 @@ sub siprequestdate { my ( $dt ) = @_; return $dt->ymd('').(' 'x4).$dt->hms(''); } + +sub fixed_length { #length of fixed fields including response code + return { + ( PATRON_STATUS_RESP ) => 37, + ( PATRON_INFO_RESP ) => 61, + ( CHECKIN_RESP ) => 24, + }->{$_[0]}; +} -- 2.1.4