From c58e4b37e3d7eee000698cc21ac81d770e8d8ba5 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 6 Nov 2015 15:12:25 +0000 Subject: [PATCH] Bug 15152: Try and make Guided Reports related tests pass Jenkins is currently failing on t_db_dependent_Reports_Guided_t.filter takes report id (393) Failing for the past 1 build (Since Unstable#190 ) Took 2 ms. Error Message not ok 8 - filter takes report id Standard Output 1..19 ok 1 - use C4::Reports::Guided; ok 2 - Not breaking subqueries ok 3 - Returns correct default offset ok 4 - Returns correct default LIMIT ok 5 - Correctly removes only final LIMIT ok 6 - Returns correct default offset ok 7 - Returns correct extracted LIMIT ok 8 - Correctly removes only final LIMIT (with offset) ok 9 - Returns correct extracted offset ok 10 - Returns correct extracted LIMIT ok 11 - Not breaking simple queries ok 12 - Returns correct default offset ok 13 - Returns correct default LIMIT ok 14 - Correctly removes LIMIT in simple case ok 15 - Returns correct default offset ok 16 - Returns correct extracted LIMIT ok 17 - Correctly removes LIMIT in simple case (with offset) ok 18 - Returns correct extracted offset ok 19 - Returns correct extracted LIMIT Which does not make sense. I suspect that there is a mismatch with the different filenames. This patch groups the different tests for Guided reports into the same file. The goal is to make Jenkins happy going all-in with this assumption. --- t/db_dependent/Reports/Guided.t | 318 ++++++++++++++++++++++++++++++++-------- t/db_dependent/ReportsGuided.t | 102 ------------- t/db_dependent/Reports_Guided.t | 121 --------------- 3 files changed, 255 insertions(+), 286 deletions(-) delete mode 100755 t/db_dependent/ReportsGuided.t delete mode 100755 t/db_dependent/Reports_Guided.t diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t index 506acee..f369d77 100644 --- a/t/db_dependent/Reports/Guided.t +++ b/t/db_dependent/Reports/Guided.t @@ -1,4 +1,5 @@ # Copyright 2012 Catalyst IT Ltd. +# Copyright Koha Development Team # # This file is part of Koha. # @@ -15,75 +16,266 @@ # You should have received a copy of the GNU General Public License # along with Koha; if not, see . -use strict; -use warnings; +use Modern::Perl; -use Test::More tests => 19; # last test to print +use Test::More tests => 4; +use Test::MockModule; +use Test::Warn; use_ok('C4::Reports::Guided'); -# This is the query I found that triggered bug 8594. -my $sql = "SELECT aqorders.ordernumber, biblio.title, biblio.biblionumber, items.homebranch, - aqorders.entrydate, aqorders.datereceived, - (SELECT DATE(datetime) FROM statistics - WHERE itemnumber=items.itemnumber AND - (type='return' OR type='issue') LIMIT 1) - AS shelvedate, - DATEDIFF(COALESCE( +subtest 'prevent from breaking some subqueries' => sub { + plan tests => 18; + # This is the query I found that triggered bug 8594. + my $sql = "SELECT aqorders.ordernumber, biblio.title, biblio.biblionumber, items.homebranch, + aqorders.entrydate, aqorders.datereceived, (SELECT DATE(datetime) FROM statistics WHERE itemnumber=items.itemnumber AND - (type='return' OR type='issue') LIMIT 1), - aqorders.datereceived), aqorders.entrydate) AS totaldays -FROM aqorders -LEFT JOIN biblio USING (biblionumber) -LEFT JOIN items ON (items.biblionumber = biblio.biblionumber - AND dateaccessioned=aqorders.datereceived) -WHERE (entrydate >= '2011-01-01' AND (datereceived < '2011-02-01' OR datereceived IS NULL)) - AND items.homebranch LIKE 'INFO' -ORDER BY title"; - -my ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($sql); -is($res_sql, $sql, "Not breaking subqueries"); -is($res_lim1, 0, "Returns correct default offset"); -is($res_lim2, undef, "Returns correct default LIMIT"); - -# Now the same thing, but we want it to remove the LIMIT from the end - -my $test_sql = $res_sql . " LIMIT 242"; -($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); -# The replacement drops a ' ' where the limit was -is(trim($res_sql), $sql, "Correctly removes only final LIMIT"); -is($res_lim1, 0, "Returns correct default offset"); -is($res_lim2, 242, "Returns correct extracted LIMIT"); - -$test_sql = $res_sql . " LIMIT 13,242"; -($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); -# The replacement drops a ' ' where the limit was -is(trim($res_sql), $sql, "Correctly removes only final LIMIT (with offset)"); -is($res_lim1, 13, "Returns correct extracted offset"); -is($res_lim2, 242, "Returns correct extracted LIMIT"); - -# After here is the simpler case, where there isn't a WHERE clause to worry -# about. - -# First case with nothing to change -$sql = "SELECT * FROM items"; -($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($sql); -is($res_sql, $sql, "Not breaking simple queries"); -is($res_lim1, 0, "Returns correct default offset"); -is($res_lim2, undef, "Returns correct default LIMIT"); - -$test_sql = $sql . " LIMIT 242"; -($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); -is(trim($res_sql), $sql, "Correctly removes LIMIT in simple case"); -is($res_lim1, 0, "Returns correct default offset"); -is($res_lim2, 242, "Returns correct extracted LIMIT"); - -$test_sql = $sql . " LIMIT 13,242"; -($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); -is(trim($res_sql), $sql, "Correctly removes LIMIT in simple case (with offset)"); -is($res_lim1, 13, "Returns correct extracted offset"); -is($res_lim2, 242, "Returns correct extracted LIMIT"); + (type='return' OR type='issue') LIMIT 1) + AS shelvedate, + DATEDIFF(COALESCE( + (SELECT DATE(datetime) FROM statistics + WHERE itemnumber=items.itemnumber AND + (type='return' OR type='issue') LIMIT 1), + aqorders.datereceived), aqorders.entrydate) AS totaldays + FROM aqorders + LEFT JOIN biblio USING (biblionumber) + LEFT JOIN items ON (items.biblionumber = biblio.biblionumber + AND dateaccessioned=aqorders.datereceived) + WHERE (entrydate >= '2011-01-01' AND (datereceived < '2011-02-01' OR datereceived IS NULL)) + AND items.homebranch LIKE 'INFO' + ORDER BY title"; + + my ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($sql); + is($res_sql, $sql, "Not breaking subqueries"); + is($res_lim1, 0, "Returns correct default offset"); + is($res_lim2, undef, "Returns correct default LIMIT"); + + # Now the same thing, but we want it to remove the LIMIT from the end + + my $test_sql = $res_sql . " LIMIT 242"; + ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); + # The replacement drops a ' ' where the limit was + is(trim($res_sql), $sql, "Correctly removes only final LIMIT"); + is($res_lim1, 0, "Returns correct default offset"); + is($res_lim2, 242, "Returns correct extracted LIMIT"); + + $test_sql = $res_sql . " LIMIT 13,242"; + ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); + # The replacement drops a ' ' where the limit was + is(trim($res_sql), $sql, "Correctly removes only final LIMIT (with offset)"); + is($res_lim1, 13, "Returns correct extracted offset"); + is($res_lim2, 242, "Returns correct extracted LIMIT"); + + # After here is the simpler case, where there isn't a WHERE clause to worry + # about. + + # First case with nothing to change + $sql = "SELECT * FROM items"; + ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($sql); + is($res_sql, $sql, "Not breaking simple queries"); + is($res_lim1, 0, "Returns correct default offset"); + is($res_lim2, undef, "Returns correct default LIMIT"); + + $test_sql = $sql . " LIMIT 242"; + ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); + is(trim($res_sql), $sql, "Correctly removes LIMIT in simple case"); + is($res_lim1, 0, "Returns correct default offset"); + is($res_lim2, 242, "Returns correct extracted LIMIT"); + + $test_sql = $sql . " LIMIT 13,242"; + ($res_sql, $res_lim1, $res_lim2) = C4::Reports::Guided::strip_limit($test_sql); + is(trim($res_sql), $sql, "Correctly removes LIMIT in simple case (with offset)"); + is($res_lim1, 13, "Returns correct extracted offset"); + is($res_lim2, 242, "Returns correct extracted LIMIT"); +}; + +subtest "Test save_report and delete_report" => sub { + plan tests => 12; + my $context = new Test::MockModule('C4::Context'); + my $koha = new Test::MockModule('C4::Koha'); + + sub MockedIsAuthorisedValueCategory { + my $authorised_value = shift; + + if ( $authorised_value eq 'LOC' ) { + return 1; + } else { + return 0; + } + } + + $koha->mock( + 'IsAuthorisedValueCategory', + \&MockedIsAuthorisedValueCategory + ); + + { # GetReservedAuthorisedValues tests + # This one will catch new reserved words not added + # to GetReservedAuthorisedValues + my %test_authval = ( + 'date' => 1, + 'branches' => 1, + 'itemtypes' => 1, + 'cn_source' => 1, + 'categorycode' => 1, + 'biblio_framework' => 1, + ); + + my $reserved_authorised_values = GetReservedAuthorisedValues(); + is_deeply(\%test_authval, $reserved_authorised_values, + 'GetReservedAuthorisedValues returns a fixed list'); + } + + { + ok( IsAuthorisedValueValid('LOC'), + 'User defined authorised value category is valid'); + + ok( ! IsAuthorisedValueValid('XXX'), + 'Not defined authorised value category is invalid'); + + # Loop through the reserved authorised values + foreach my $authorised_value ( keys %{GetReservedAuthorisedValues()} ) { + ok( IsAuthorisedValueValid($authorised_value), + '\''.$authorised_value.'\' is a reserved word, and thus a valid authorised value'); + } + } + + { # GetParametersFromSQL tests + + my $test_query_1 = " + SELECT date_due + FROM old_issues + WHERE YEAR(timestamp) = <> AND + branchcode = <> AND + borrowernumber = <> + "; + + my @test_parameters_with_custom_list = ( + { 'name' => 'Year', 'authval' => 'custom_list' }, + { 'name' => 'Branch', 'authval' => 'branches' }, + { 'name' => 'Borrower', 'authval' => undef } + ); + + is_deeply( GetParametersFromSQL($test_query_1), \@test_parameters_with_custom_list, + 'SQL params are correctly parsed'); + + # ValidateSQLParameters tests + my @problematic_parameters = (); + push @problematic_parameters, { 'name' => 'Year', 'authval' => 'custom_list' }; + is_deeply( ValidateSQLParameters( $test_query_1 ), + \@problematic_parameters, + '\'custom_list\' not a valid category' ); + + my $test_query_2 = " + SELECT date_due + FROM old_issues + WHERE YEAR(timestamp) = <> AND + branchcode = <> AND + borrowernumber = <> + "; + + is_deeply( ValidateSQLParameters( $test_query_2 ), + [], + 'All parameters valid, empty problematic authvals list'); + } +}; + +subtest "Test IsAuthorisedValueValid" => sub { + plan tests => 17; + can_ok( + 'C4::Reports::Guided', + qw(save_report delete_report execute_query) + ); + + my $schema = Koha::Database->new->schema; + $schema->storage->txn_begin; + + my $dbh = C4::Context->dbh; + $dbh->do(q|DELETE FROM saved_sql|); + $dbh->do(q|DELETE FROM saved_reports|); + + #Start tests + + #Test save_report + my $count = scalar( @{ get_saved_reports() } ); + is( $count, 0, "There is no report" ); + + my @report_ids; + for my $id ( 1 .. 3 ) { + push @report_ids, save_report({ + borrowernumber => $id, + sql => "SQL$id", + name => "Name$id", + area => "area$id", + group => "group$id", + subgroup => "subgroup$id", + type => "type$id", + notes => "note$id", + cache_expiry => "null", + public => "null" + }); + $count++; + } + like( $report_ids[0], '/^\d+$/', "Save_report returns an id for first" ); + like( $report_ids[1], '/^\d+$/', "Save_report returns an id for second" ); + like( $report_ids[2], '/^\d+$/', "Save_report returns an id for third" ); + + is( scalar( @{ get_saved_reports() } ), + $count, "$count reports have been added" ); + + is( scalar( @{ get_saved_reports( $report_ids[0] ) } ), + 1, "filter takes report id" ); + + #Test delete_report + is (delete_report(),undef, "Without id delete_report returns undef"); + + is( delete_report( $report_ids[0] ), 1, "report 1 is deleted" ); + $count--; + + is( scalar( @{ get_saved_reports() } ), $count, "Report1 has been deleted" ); + + is( delete_report( $report_ids[1], $report_ids[2] ), 2, "report 2 and 3 are deleted" ); + $count -= 2; + + is( scalar( @{ get_saved_reports() } ), + $count, "Report2 and report3 have been deleted" ); + + my $sth = execute_query('SELECT COUNT(*) FROM systempreferences', 0, 10); + my $results = $sth->fetchall_arrayref; + is(scalar(@$results), 1, 'running a query returned a result'); + + my $version = C4::Context->preference('Version'); + $sth = execute_query( + 'SELECT value FROM systempreferences WHERE variable = ?', + 0, + 10, + [ 'Version' ], + ); + $results = $sth->fetchall_arrayref; + is_deeply( + $results, + [ [ $version ] ], + 'running a query with a parameter returned the expected result' + ); + + # for next test, we want to let execute_query capture any SQL errors + $dbh->{RaiseError} = 0; + my $errors; + warning_like { ($sth, $errors) = execute_query( + 'SELECT surname FRM borrowers', # error in the query is intentional + 0, 10 ) } + qr/^DBD::mysql::st execute failed: You have an error in your SQL syntax;/, + "Wrong SQL syntax raises warning"; + ok( + defined($errors) && exists($errors->{queryerr}), + 'attempting to run a report with an SQL syntax error returns error message (Bug 12214)' + ); + + is_deeply( get_report_areas(), [ 'CIRC', 'CAT', 'PAT', 'ACQ', 'ACC', 'SER' ], + "get_report_areas returns the correct array of report areas"); + +}; sub trim { my ($s) = @_; diff --git a/t/db_dependent/ReportsGuided.t b/t/db_dependent/ReportsGuided.t deleted file mode 100755 index 097722a..0000000 --- a/t/db_dependent/ReportsGuided.t +++ /dev/null @@ -1,102 +0,0 @@ -#!/usr/bin/perl - -use Modern::Perl; - -use Test::More tests => 13; -use Test::MockModule; - -use t::lib::Mocks; - -use_ok('C4::Reports::Guided'); - -my $context = new Test::MockModule('C4::Context'); -my $koha = new Test::MockModule('C4::Koha'); - -BEGIN { - t::lib::Mocks::mock_dbh; -} - -sub MockedIsAuthorisedValueCategory { - my $authorised_value = shift; - - if ( $authorised_value eq 'LOC' ) { - return 1; - } else { - return 0; - } -} - -$koha->mock( - 'IsAuthorisedValueCategory', - \&MockedIsAuthorisedValueCategory -); - -{ # GetReservedAuthorisedValues tests - # This one will catch new reserved words not added - # to GetReservedAuthorisedValues - my %test_authval = ( - 'date' => 1, - 'branches' => 1, - 'itemtypes' => 1, - 'cn_source' => 1, - 'categorycode' => 1, - 'biblio_framework' => 1, - ); - - my $reserved_authorised_values = GetReservedAuthorisedValues(); - is_deeply(\%test_authval, $reserved_authorised_values, - 'GetReservedAuthorisedValues returns a fixed list'); -} - -{ - ok( IsAuthorisedValueValid('LOC'), - 'User defined authorised value category is valid'); - - ok( ! IsAuthorisedValueValid('XXX'), - 'Not defined authorised value category is invalid'); - - # Loop through the reserved authorised values - foreach my $authorised_value ( keys %{GetReservedAuthorisedValues()} ) { - ok( IsAuthorisedValueValid($authorised_value), - '\''.$authorised_value.'\' is a reserved word, and thus a valid authorised value'); - } -} - -{ # GetParametersFromSQL tests - - my $test_query_1 = " - SELECT date_due - FROM old_issues - WHERE YEAR(timestamp) = <> AND - branchcode = <> AND - borrowernumber = <> - "; - - my @test_parameters_with_custom_list = ( - { 'name' => 'Year', 'authval' => 'custom_list' }, - { 'name' => 'Branch', 'authval' => 'branches' }, - { 'name' => 'Borrower', 'authval' => undef } - ); - - is_deeply( GetParametersFromSQL($test_query_1), \@test_parameters_with_custom_list, - 'SQL params are correctly parsed'); - - # ValidateSQLParameters tests - my @problematic_parameters = (); - push @problematic_parameters, { 'name' => 'Year', 'authval' => 'custom_list' }; - is_deeply( ValidateSQLParameters( $test_query_1 ), - \@problematic_parameters, - '\'custom_list\' not a valid category' ); - - my $test_query_2 = " - SELECT date_due - FROM old_issues - WHERE YEAR(timestamp) = <> AND - branchcode = <> AND - borrowernumber = <> - "; - - is_deeply( ValidateSQLParameters( $test_query_2 ), - [], - 'All parameters valid, empty problematic authvals list'); -} diff --git a/t/db_dependent/Reports_Guided.t b/t/db_dependent/Reports_Guided.t deleted file mode 100755 index 649e33a..0000000 --- a/t/db_dependent/Reports_Guided.t +++ /dev/null @@ -1,121 +0,0 @@ -#!/usr/bin/perl - -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use Modern::Perl; - -use Test::More tests => 18; -use Test::Warn; - -use C4::Context; -use Koha::Database; - -BEGIN { - use_ok('C4::Reports::Guided'); -} -can_ok( - 'C4::Reports::Guided', - qw(save_report delete_report execute_query) -); - -my $schema = Koha::Database->new->schema; -$schema->storage->txn_begin; - -my $dbh = C4::Context->dbh; -$dbh->do(q|DELETE FROM saved_sql|); -$dbh->do(q|DELETE FROM saved_reports|); - -#Start tests - -#Test save_report -my $count = scalar( @{ get_saved_reports() } ); -is( $count, 0, "There is no report" ); - -my @report_ids; -for my $id ( 1 .. 3 ) { - push @report_ids, save_report({ - borrowernumber => $id, - sql => "SQL$id", - name => "Name$id", - area => "area$id", - group => "group$id", - subgroup => "subgroup$id", - type => "type$id", - notes => "note$id", - cache_expiry => "null", - public => "null" - }); - $count++; -} -like( $report_ids[0], '/^\d+$/', "Save_report returns an id for first" ); -like( $report_ids[1], '/^\d+$/', "Save_report returns an id for second" ); -like( $report_ids[2], '/^\d+$/', "Save_report returns an id for third" ); - -is( scalar( @{ get_saved_reports() } ), - $count, "$count reports have been added" ); - -is( scalar( @{ get_saved_reports( $report_ids[0] ) } ), - 1, "filter takes report id" ); - -#Test delete_report -is (delete_report(),undef, "Without id delete_report returns undef"); - -is( delete_report( $report_ids[0] ), 1, "report 1 is deleted" ); -$count--; - -is( scalar( @{ get_saved_reports() } ), $count, "Report1 has been deleted" ); - -is( delete_report( $report_ids[1], $report_ids[2] ), 2, "report 2 and 3 are deleted" ); -$count -= 2; - -is( scalar( @{ get_saved_reports() } ), - $count, "Report2 and report3 have been deleted" ); - -my $sth = execute_query('SELECT COUNT(*) FROM systempreferences', 0, 10); -my $results = $sth->fetchall_arrayref; -is(scalar(@$results), 1, 'running a query returned a result'); - -my $version = C4::Context->preference('Version'); -$sth = execute_query( - 'SELECT value FROM systempreferences WHERE variable = ?', - 0, - 10, - [ 'Version' ], -); -$results = $sth->fetchall_arrayref; -is_deeply( - $results, - [ [ $version ] ], - 'running a query with a parameter returned the expected result' -); - -# for next test, we want to let execute_query capture any SQL errors -$dbh->{RaiseError} = 0; -my $errors; -warning_like { ($sth, $errors) = execute_query( - 'SELECT surname FRM borrowers', # error in the query is intentional - 0, 10 ) } - qr/^DBD::mysql::st execute failed: You have an error in your SQL syntax;/, - "Wrong SQL syntax raises warning"; -ok( - defined($errors) && exists($errors->{queryerr}), - 'attempting to run a report with an SQL syntax error returns error message (Bug 12214)' -); - -is_deeply( get_report_areas(), [ 'CIRC', 'CAT', 'PAT', 'ACQ', 'ACC', 'SER' ], - "get_report_areas returns the correct array of report areas"); - -$schema->storage->txn_rollback; -- 2.1.0