Bug 14097

Summary: Add unit tests to C4::UsageStats
Product: Koha Reporter: Julian FIOL <julian.fiol>
Component: Test SuiteAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: trivial    
Priority: P5 - low CC: bgkriegel, brendan, indradg, jonathan.druart, julian.maurice, kyle.m.hall, mtompset, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14097 : Testing C4::UsageStats
Bug 14097 : Delete table issues before testing
Bug 14097 : changing verif_systempreferences_values subroutine
[SIGNED-OFF] Bug 14097: Testing C4::UsageStats
[SIGNED-OFF] Bug 14097: Delete table issues before testing
[SIGNED-OFF] Bug 14097: changing verif_systempreferences_values subroutine
Bug 14097 : Rollback issue and warning corrected
[SIGNED-OFF] Bug 14097: Rollback issue and warning corrected
Bug 14097 - Correct noise under Debian
Bug 14097 : Rollback issue and warning corrected
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 : Changing AddReserve prototype call
Bug 14097: Testing C4::UsageStats
Bug 14097: Delete table issues before testing
Bug 14097: changing verif_systempreferences_values subroutine
Bug 14097: Rollback issue and warning corrected
Bug 14097 - Correct noise under Debian
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 : Changing AddReserve prototype call
Bug 14097: Fixed missing test cases
Bug 14097: Testing C4::UsageStats
Bug 14097: Delete table issues before testing
Bug 14097: changing verif_systempreferences_values subroutine
Bug 14097: Rollback issue and warning corrected
Bug 14097 - Correct noise under Debian
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 : Changing AddReserve prototype call
Bug 14097: Fixed missing test cases
Bug 14097: Fixed missing test cases
Bug 14097 - Correct noise under Debian
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 : Changing AddReserve prototype call
Bug 14097 - Correct noise under Debian
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 - Correct noise under Debian
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 : Changing AddReserve prototype call
Bug 14097: Fixed missing test cases
Bug 14097: Testing C4::UsageStats
Bug 14097: Delete table issues before testing
Bug 14097: changing verif_systempreferences_values subroutine
Bug 14097: Rollback issue and warning corrected
Bug 14097 - Correct noise under Debian
Bug 14097 : Avoid issue if the number of syspref shared is modified
Bug 14097 : Changing AddReserve prototype call
Bug 14097: Fixed missing test cases

Description Julian FIOL 2015-04-30 09:19:46 UTC
DESCRIPTION
    -----------
    
    Add unit tests to C4::UsageStats
    Subroutine :
    (V : tested, X : Not tested)
    
        V  NeedUpdate
        V  BuiltReport
        X  ReportToCommunity
        V  _count
    
    TEST PLAN
    ---------
    
    1. Apply patch
    2. prove -v t/db_dependent/UsageStats.t
    -- All tests should run successfully
    
/!\ I have 1 warning but I don't know where it can come /!\
    (in cleanup) DBIx::Class::Storage::txn_rollback(): A txn_rollback
    in nested transaction is ineffective! (depth 1) at
    t/lib/TestBuilder.pm line 308
    
    TEST PLAN OPTIONAL
    ------------------
    
    Go check bug 13899 to see the coverage of this module.
    
    Coverage BEFORE this patch :
    Statement  :  40,5%
    Branch     :   0,0%
    Condition  :   0,0%
    Subroutine :  55,6%
    
    Coverage AFTER this patch :
    Statement  :  81,0%
    Branch     : 100,0%
    Condition  : 100,0%
    Subroutine :  88,8%
Comment 1 Julian FIOL 2015-04-30 09:21:33 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-04-30 12:36:03 UTC
Got following error:

t/db_dependent/UsageStats.t .. 1/552 DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_kohadev`.`issues`, CONSTRAINT `issues_ibfk_2` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON UPDATE CASCADE) at t/db_dependent/UsageStats.t line 45.
DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_kohadev`.`issues`, CONSTRAINT `issues_ibfk_2` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON UPDATE CASCADE) at t/db_dependent/UsageStats.t line 45.
# Looks like you planned 552 tests but ran 9.
# Looks like your test exited with 255 just after 9.
t/db_dependent/UsageStats.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 543/552 subtests 

After that, my DB seems to be broken, can not langer search, get:

No results found
You did not specify any search criteria.
Error: Can't call method "raw" on an undefined value at /usr/share/kohaclone/C4/Search.pm line 799. 

Not sure what happend. Will have to restore my VM from yesterday's backup.
Comment 3 Marc Véron 2015-04-30 13:46:58 UTC
Ok, restored VM, tested with same failure as above.

No problem with database after testing. Do not now what happend to break it before.

Question: What happens if the script stops / exits / fails for whatever reason after following lines? 

my $dbh = C4::Context->dbh;
$dbh->{AutoCommit} = 0;
$dbh->{RaiseError} = 1;
$dbh->do('DELETE FROM biblio');
$dbh->do('DELETE FROM items');
$dbh->do('DELETE FROM auth_header');
$dbh->do('DELETE FROM old_issues');
$dbh->do('DELETE FROM old_reserves');
$dbh->do('DELETE FROM borrowers');
$dbh->do('DELETE FROM aqorders');
$dbh->do('DELETE FROM subscription');
Comment 4 Julian FIOL 2015-05-04 09:10:24 UTC Comment hidden (obsolete)
Comment 5 Julian FIOL 2015-05-04 09:27:38 UTC
About Comment #3

After some tests, it seems that if the script stops / exits / fails for whatever reason after following lines, there is no $dbh->rollback, and the changes of the DB remains...


my $dbh = C4::Context->dbh;
$dbh->{AutoCommit} = 0;
$dbh->{RaiseError} = 1;
$dbh->do('DELETE FROM biblio');
$dbh->do('DELETE FROM items');
$dbh->do('DELETE FROM auth_header');
$dbh->do('DELETE FROM old_issues');
$dbh->do('DELETE FROM old_reserves');
$dbh->do('DELETE FROM borrowers');
$dbh->do('DELETE FROM aqorders');
$dbh->do('DELETE FROM subscription');
Comment 6 Julian FIOL 2015-05-04 13:30:29 UTC Comment hidden (obsolete)
Comment 7 Bernardo Gonzalez Kriegel 2015-05-07 13:53:27 UTC Comment hidden (obsolete)
Comment 8 Bernardo Gonzalez Kriegel 2015-05-07 13:53:33 UTC Comment hidden (obsolete)
Comment 9 Bernardo Gonzalez Kriegel 2015-05-07 13:53:40 UTC Comment hidden (obsolete)
Comment 10 Bernardo Gonzalez Kriegel 2015-05-07 14:14:44 UTC
Mmmm, changed my mind.
After running the test, my db was left without items, borrowers, etc.
No rollback it seems
Comment 11 Julian FIOL 2015-05-12 12:44:45 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2015-05-12 13:39:39 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-05-22 15:15:51 UTC
 FAIL	t/db_dependent/UsageStats.t
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		Type of arg 1 to keys must be hash (not hash element)
		t/db_dependent/UsageStats.t had compilation errors.
Comment 14 Bernardo Gonzalez Kriegel 2015-05-22 15:24:10 UTC
(In reply to Kyle M Hall from comment #13)
>  FAIL	t/db_dependent/UsageStats.t
>    OK	  critic
>    OK	  forbidden patterns
>    OK	  pod
>    FAIL	  valid
> 		Type of arg 1 to keys must be hash (not hash element)
> 		t/db_dependent/UsageStats.t had compilation errors.

Strange

qa -c 4
testing 4 commit(s) (applied to 48d793a 'Bug 14106: patch existing zebra confi')

Processing files before patches
|========================>| 2 / 2 (100.00%)
Processing files after patches
|========================>| 2 / 2 (100.00%)

 OK     t/db_dependent/UsageStats.t
 OK     C4/UsageStats.pm

And t & xt tests run ok.
Comment 15 Mark Tompsett 2015-05-22 20:30:44 UTC
Running on Debian Jessie:
[SNIP]
keys on reference is experimental at t/db_dependent/UsageStats.t line 88.
keys on reference is experimental at t/db_dependent/UsageStats.t line 106.
keys on reference is experimental at t/db_dependent/UsageStats.t line 119.
keys on reference is experimental at t/db_dependent/UsageStats.t line 136.
keys on reference is experimental at t/db_dependent/UsageStats.t line 153.
keys on reference is experimental at t/db_dependent/UsageStats.t line 162.
keys on reference is experimental at t/db_dependent/UsageStats.t line 587.
[SNIP]
Please clean up.
Comment 16 Mark Tompsett 2015-05-22 20:32:55 UTC
Similar to comment #13:

mtompset@debian:~/kohaclone$ koha-qa.pl -v 2 -c 3
testing 3 commit(s) (applied to b61e957 'Bug 14097: Testing C4::UsageStats')

Processing files before patches
|========================>| 1 / 1 (100.00%)
Processing files after patches
|========================>| 1 / 1 (100.00%)

 FAIL   t/db_dependent/UsageStats.t
   OK     critic
   OK     forbidden patterns
   OK     pod
   FAIL   valid
                keys on reference is experimental

See line numbers in comment #15.
Comment 17 Mark Tompsett 2015-05-23 04:29:08 UTC Comment hidden (obsolete)
Comment 18 Bernardo Gonzalez Kriegel 2015-05-23 20:19:49 UTC
Thanks Mark, I need some box with Perl 5.20 :)
Comment 19 Jonathan Druart 2015-08-21 10:36:16 UTC
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
Comment 20 Jonathan Druart 2015-08-21 10:40:40 UTC
I meant bug 9809.
Comment 21 Julian FIOL 2015-08-24 11:54:24 UTC Comment hidden (obsolete)
Comment 22 Julian FIOL 2015-08-24 12:08:52 UTC Comment hidden (obsolete)
Comment 23 Julian FIOL 2015-08-24 12:08:57 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2016-01-02 11:19:08 UTC
Does not apply:

CONFLICT (content): Merge conflict in t/db_dependent/UsageStats.t
Failed to merge in the changes.
Comment 25 Mark Tompsett 2016-01-04 06:25:08 UTC Comment hidden (obsolete)
Comment 26 Mark Tompsett 2016-01-04 06:25:19 UTC Comment hidden (obsolete)
Comment 27 Mark Tompsett 2016-01-04 06:25:30 UTC Comment hidden (obsolete)
Comment 28 Mark Tompsett 2016-01-04 06:25:55 UTC Comment hidden (obsolete)
Comment 29 Mark Tompsett 2016-01-04 06:26:05 UTC Comment hidden (obsolete)
Comment 30 Mark Tompsett 2016-01-04 06:26:13 UTC Comment hidden (obsolete)
Comment 31 Mark Tompsett 2016-01-04 06:26:27 UTC Comment hidden (obsolete)
Comment 32 Mark Tompsett 2016-01-04 06:26:36 UTC Comment hidden (obsolete)
Comment 33 Mark Tompsett 2016-01-04 06:30:14 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2016-01-04 06:30:21 UTC Comment hidden (obsolete)
Comment 35 Mark Tompsett 2016-01-04 06:30:30 UTC Comment hidden (obsolete)
Comment 36 Mark Tompsett 2016-01-04 06:30:36 UTC Comment hidden (obsolete)
Comment 37 Mark Tompsett 2016-01-04 06:30:40 UTC Comment hidden (obsolete)
Comment 38 Mark Tompsett 2016-01-04 06:30:45 UTC Comment hidden (obsolete)
Comment 39 Mark Tompsett 2016-01-04 06:30:49 UTC Comment hidden (obsolete)
Comment 40 Mark Tompsett 2016-01-04 06:30:54 UTC Comment hidden (obsolete)
Comment 41 Indranil Das Gupta 2016-02-05 23:14:01 UTC Comment hidden (obsolete)
Comment 42 Indranil Das Gupta 2016-02-06 03:25:48 UTC Comment hidden (obsolete)
Comment 43 Indranil Das Gupta 2016-02-06 03:27:51 UTC Comment hidden (obsolete)
Comment 44 Indranil Das Gupta 2016-02-06 03:29:09 UTC Comment hidden (obsolete)
Comment 45 Indranil Das Gupta 2016-02-06 03:41:43 UTC Comment hidden (obsolete)
Comment 46 Indranil Das Gupta 2016-02-06 03:43:10 UTC Comment hidden (obsolete)
Comment 47 Indranil Das Gupta 2016-02-06 03:54:27 UTC Comment hidden (obsolete)
Comment 48 Indranil Das Gupta 2016-02-06 03:55:51 UTC Comment hidden (obsolete)
Comment 49 Indranil Das Gupta 2016-02-06 03:57:15 UTC Comment hidden (obsolete)
Comment 50 Indranil Das Gupta 2016-02-06 03:58:31 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2016-02-09 12:16:27 UTC
Created attachment 47798 [details] [review]
Bug 14097: Testing C4::UsageStats

DESCRIPTION
-----------

Add unit tests to C4::UsageStats
Subroutine :
(V : tested, X : Not tested)

	V  NeedUpdate
	V  BuiltReport
	X  ReportToCommunity
	V  _count

TEST PLAN
---------

1. Apply patch
2. prove -v t/db_dependent/UsageStats.t
-- All tests should run successfully

/!\ I have 1 warning but I don't know where it can come /!\
(in cleanup) DBIx::Class::Storage::txn_rollback(): A txn_rollback
in nested transaction is ineffective! (depth 1) at
t/lib/TestBuilder.pm line 308

TEST PLAN OPTIONAL
------------------

Go check bug 13899 to see the coverage of this module.

Coverage BEFORE this patch :
Statement  :  40,5%
Branch     :   0,0%
Condition  :   0,0%
Subroutine :  55,6%

Coverage AFTER this patch :
Statement  :  81,0%
Branch     : 100,0%
Condition  : 100,0%
Subroutine :  88,8%

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 52 Jonathan Druart 2016-02-09 12:16:31 UTC
Created attachment 47799 [details] [review]
Bug 14097: Delete table issues before testing

If we delete all biblio DB, we could have some troubles with foreign keys

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 53 Jonathan Druart 2016-02-09 12:16:37 UTC
Created attachment 47800 [details] [review]
Bug 14097: changing verif_systempreferences_values subroutine

It's saving a large number of lines

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
All tests run, no koha-qa errors

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 54 Jonathan Druart 2016-02-09 12:16:41 UTC
Created attachment 47801 [details] [review]
Bug 14097: Rollback issue and warning corrected

The rollback issue came from '_count' testing.
It used a 'DROP TABLE' and 'CREATE TABLE' which implicitly do autocommit.
That's why the final rollback could not be.

This patch no longer uses t::lib::TestBuilder either because it was the cause
of the warning on Comment #1 :
"DBIx::Class::Storage::txn_rollback(): A txn_rollback
in nested transaction is ineffective! (depth 1) at
t/lib/TestBuilder.pm line 308"

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Now data is preserved, no errors.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 55 Jonathan Druart 2016-02-09 12:16:45 UTC
Created attachment 47802 [details] [review]
Bug 14097 - Correct noise under Debian

The first four patches do not create any noise under Ubuntu 14.04 LTS.

Running on Debian Jessie:
[SNIP]
keys on reference is experimental at t/db_dependent/UsageStats.t line 88.
keys on reference is experimental at t/db_dependent/UsageStats.t line 106.
keys on reference is experimental at t/db_dependent/UsageStats.t line 119.
keys on reference is experimental at t/db_dependent/UsageStats.t line 136.
keys on reference is experimental at t/db_dependent/UsageStats.t line 153.
keys on reference is experimental at t/db_dependent/UsageStats.t line 162.
keys on reference is experimental at t/db_dependent/UsageStats.t line 587.
[SNIP]

Instead of having 'keys $foo->{bar}', 'keys %{$foo->{bar}}' was used.

TEST PLAN
---------
1) On a DEBIAN system (Jessie gives above messages)
   apply the first 4 patches interactively.
2) $ prove t/db_dependent/UsageStats.t
   -- trigger noise like above.
3) apply this patch
4) $ prove t/db_dependent/UsageStats.t
   -- no noise triggered.
5) koha qa test tools

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 56 Jonathan Druart 2016-02-09 12:16:50 UTC
Created attachment 47803 [details] [review]
Bug 14097 : Avoid issue if the number of syspref shared is modified

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 57 Jonathan Druart 2016-02-09 12:16:54 UTC
Created attachment 47804 [details] [review]
Bug 14097 : Changing AddReserve prototype call

In response of Joubu comment #19

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 58 Jonathan Druart 2016-02-09 12:16:59 UTC
Created attachment 47805 [details] [review]
Bug 14097: Fixed missing test cases

Fixed AllowPkiAuth issue caused by 15101.
Added missing AudioAlerts

TEST PLAN
---------
1) prove -v t/db_dependent/UsageStats.t
   -- should pass after applying patch
2) Run koha qa test tools

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 59 Brendan Gallagher 2016-02-24 03:19:18 UTC
Pushed to Master - Thanks!  Should be in the May 2016 Release.  Thanks!
Comment 60 Julian Maurice 2016-03-07 13:22:52 UTC
Patch pushed to 3.22.x, will be in 3.22.5