From 29260299c314bee90980f105baa345072fc35f84 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 2 Aug 2017 13:01:47 +0200 Subject: [PATCH] Bug 18996: [16.11.x] [QA Follow-up] Remove global variables from Message.t 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 Signed-off-by: Colin Campbell Signed-off-by: Kyle M Hall Conflicts: t/db_dependent/SIP/Message.t --- t/db_dependent/SIP/Message.t | 79 ++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/t/db_dependent/SIP/Message.t b/t/db_dependent/SIP/Message.t index d37f5d3c55..6c44db29a7 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,33 +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; } ); -$branch = $builder->build({ - source => 'Branch', -}); -$branchcode = $branch->{branchcode}; - # 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; @@ -79,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; @@ -87,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; @@ -99,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 => { @@ -115,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 ); @@ -154,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. '|'. @@ -170,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 => { @@ -185,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.' ); @@ -223,7 +212,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; @@ -235,6 +224,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', @@ -250,6 +244,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; } ); @@ -337,12 +332,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; @@ -367,3 +378,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.39.5