Bug 18762

Summary: Some tests are noisy
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, katrin.fischer, ldjamison, mtompset, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18633
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 18762: Remove warnings from xt/author/valid-templates.t
Bug 18762: Remove a warning from History.t
Bug 18762: Remove some warnings from Search.t
Bug 18762: Remove some warnings from Search.t
Bug 18762: Catch noisy conditions in t/db_dependent/Search.t
[SIGNED OFF] Bug 18762: Catch noisy Search.t
Bug 18656 - Require confirmation of deletion of files from patron record
[SIGNED OFF] Bug 18762: Warnings valid-templates.t
Bug 18762: Remove warnings from xt/author/valid-templates.t
Bug 18762: Remove a warning from History.t
Bug 18762: Remove some warnings from Search.t

Description Jonathan Druart 2017-06-08 19:15:10 UTC
xt/author/valid-templates.t .. 32/? Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt line 785.
Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt line 785.
xt/author/valid-templates.t .. 47/? Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-detail.tt line 49.
xt/author/valid-templates.t .. 59/? # excluding template /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc because it cannot stand on its own
# excluding template /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-close.inc because it cannot stand on its own
xt/author/valid-templates.t .. 150/? Argument "" isn't numeric in multiplication (*) at /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/supplier.tt line 356.
[SKIP CGI warnings]
Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt line 134.
Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt line 134.


t/db_dependent/Search/History.t .. 1/27 DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1560.


t/db_dependent/Search.t .. The syspref casAuthentication was requested but I don't know what to say; this indicates that the test requires updating at t/db_dependent/Search.t line 140.
The syspref casLogout was requested but I don't know what to say; this indicates that the test requires updating at t/db_dependent/Search.t line 140.
The syspref casServerUrl was requested but I don't know what to say; this indicates that the test requires updating at t/db_dependent/Search.t line 140.
The syspref template was requested but I don't know what to say; this indicates that the test requires updating at t/db_dependent/Search.t line 140.
Comment 1 Jonathan Druart 2017-06-08 19:19:21 UTC
Created attachment 64114 [details] [review]
Bug 18762: Remove warnings from xt/author/valid-templates.t

Test plan:
Read the changes and make sure they make sense
Comment 2 Jonathan Druart 2017-06-08 19:19:25 UTC
Created attachment 64115 [details] [review]
Bug 18762: Remove a warning from History.t

Test plan:
  prove t/db_dependent/Search/History.t
should not display any warnings
Comment 3 Jonathan Druart 2017-06-08 19:19:29 UTC
Created attachment 64116 [details] [review]
Bug 18762: Remove some warnings from Search.t

Note that some are caused by CGI, see bug 18632

Test plan:
Confirm there are less warnings with this patch applied
Comment 4 Jonathan Druart 2017-06-08 19:20:11 UTC
Created attachment 64117 [details] [review]
Bug 18762: Remove some warnings from Search.t

Note that some are caused by CGI, see bug 18633

Test plan:
Confirm there are less warnings with this patch applied
Comment 5 Mark Tompsett 2017-06-10 03:09:51 UTC
Created attachment 64170 [details] [review]
Bug 18762: Catch noisy conditions in t/db_dependent/Search.t

Seems that the grs1 case for ti:punctuation generates a warning,
while the dom case does not. This tweak catches both.

before patch: noisy.
after patch: no unexpected output.
run koha qa test tools
Comment 6 Mark Tompsett 2017-06-10 03:10:51 UTC
And with this patch, I believe the only noise generated are the CGI related issues.
Comment 7 Lee Jamison 2017-06-10 12:48:30 UTC
Running 'prove -v xt/author/valid-templates.t' outputs the following warnings (I included surrounding passed tests to indicate where in the test the warnings appear:



ok 228 - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/issuehistory.tt
CGI::param called in list context from package Template::Document line 41, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 43, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 41, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 43, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 41, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 43, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 41, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 43, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 121, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 122, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 123, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 124, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 466, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
CGI::param called in list context from package Template::Document line 471, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
k 229 - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt
ok 230 - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch_csv.tt
CGI::param called in list context from package Template::Document line 3, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
ok 231 - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch_json.tt
ok 599 - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/image-manage.tt
CGI::param called in list context from package Template::Document line 47, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
ok 600 - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/manage.tt



Everything passes so I COULD sign off on it but before I do I wanted to make sure the CGI::Param warning wouldn't cause a security/architectural concern.
Comment 8 Mark Tompsett 2017-06-10 20:12:34 UTC
(In reply to Lee Jamison from comment #7)
[snip] 
> Everything passes so I COULD sign off on it but before I do I wanted to make
> sure the CGI::Param warning wouldn't cause a security/architectural concern.

The CGI::Param is beyond the scope of this bug, and is dealt with on other bugs such as bug 18633. The CGI::Param issues are more difficult to narrow down as well. You will note also on comment #0 that JD expressly said, "[SKIP CGI warnings]". If all the other bugs, except the CGI::Param ones are eliminated, then I would recommend a sign off.
Comment 9 Lee Jamison 2017-06-11 05:03:27 UTC
Created attachment 64175 [details] [review]
[SIGNED OFF] Bug 18762: Catch noisy Search.t

Seems that the grs1 case for ti:punctuation generates a warning,
while the dom case does not. This tweak catches both.

before patch: noisy.
after patch: no unexpected output.
run koha qa test tools

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
I ran reset_all in kohadevbox. This patch clears
up the extra noise correctly now and passes QA tools.
Comment 10 Mark Tompsett 2017-06-12 02:23:08 UTC
Comment on attachment 64170 [details] [review]
Bug 18762: Catch noisy conditions in t/db_dependent/Search.t

Forgot to obsolete this patch that was signed off. This is why the kohadevbox [SIGNED OFF] logic may get in the way.
Comment 11 Mark Tompsett 2017-06-12 02:26:15 UTC
It's late here, and I noticed that Lee only signed off the last one. I've tested this as well, and would sign everything off, if a QA'r just wants to go ahead and fake my sign off in the midst of QA'ing. If I remember to check later, I'll sign off.
Comment 12 Lee Jamison 2017-06-12 17:42:41 UTC
Created attachment 64232 [details] [review]
Bug 18656 - Require confirmation of deletion of files from patron record

This patch adds a JavaScript confirm to the deletion of files. It also
styles the file deletion link as a Bootstrap button.

To test, apply the patch and open the "Files" page for a patron who has
files attached.

- Verify that clicking any "delete" link prompts for a confirmation.
- Verify that confirming the dialog triggers the deletion of the correct
  file.
- Verify that canceling the confirmation dialog aborts the deletion.
- Confirm that the "Delete" button looks correct.

Signed-off-by: David Roberts <david.roberts@ptfs-europe.com>

Revision for QA: Replaced '.click(function' with '.on("click",function'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Lee Jamison 2017-06-12 18:44:56 UTC
Created attachment 64234 [details] [review]
[SIGNED OFF] Bug 18762: Warnings valid-templates.t

Test plan:
Read the changes and make sure they make sense

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Changes make sense.
Comment 14 Jonathan Druart 2017-06-12 22:01:37 UTC
(In reply to M. Tompsett from comment #5)
> Created attachment 64170 [details] [review] [review]
> Bug 18762: Catch noisy conditions in t/db_dependent/Search.t
> 
> Seems that the grs1 case for ti:punctuation generates a warning,
> while the dom case does not. This tweak catches both.
> 
> before patch: noisy.
> after patch: no unexpected output.
> run koha qa test tools

Tomas, could you take a look at this last patch please? No idea why it is warning now.
Comment 15 Jonathan Druart 2017-06-14 17:08:50 UTC
Created attachment 64294 [details] [review]
Bug 18762: Remove warnings from xt/author/valid-templates.t

Test plan:
Read the changes and make sure they make sense

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Jonathan Druart 2017-06-14 17:08:55 UTC
Created attachment 64295 [details] [review]
Bug 18762: Remove a warning from History.t

Test plan:
  prove t/db_dependent/Search/History.t
should not display any warnings

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 17 Jonathan Druart 2017-06-14 17:08:59 UTC
Created attachment 64296 [details] [review]
Bug 18762: Remove some warnings from Search.t

Note that some are caused by CGI, see bug 18632

Test plan:
Confirm there are less warnings with this patch applied

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 18 Jonathan Druart 2017-06-14 17:10:30 UTC
Added the signed-off-by lines. Mark I removed your patch, I am not sure if the warning should be displayed. I do not have time to dig into the problem right now and would like to see the other patches pushed.
Will see that later.

Skipping QA.
Comment 19 Jonathan Druart 2017-06-14 20:39:30 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 20 Fridolin Somers 2017-06-15 12:42:29 UTC
Just to be clear on TT syntax.

There is a change :
-[% IF fines > 0 %]
+[% IF fines and fines > 0 %]

But also :
-    [% IF count > 0 %]
+    [% IF count %]

Why not using the same compact [% IF var %] ?
Is it false with undef and "0" right ?
Comment 21 Jonathan Druart 2017-06-15 16:12:23 UTC
Yes it is the same.
Comment 22 Fridolin Somers 2017-06-20 07:57:54 UTC
(In reply to Jonathan Druart from comment #21)
> Yes it is the same.

Ok thanks a lot
Comment 23 Fridolin Somers 2017-06-20 08:13:49 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 24 Katrin Fischer 2017-06-21 06:18:01 UTC
These patches have been pushed to 16.11.x and will be in 16.11.09.