Bug 18732 - Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Summary: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: M. Tompsett
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-05 22:02 UTC by M. Tompsett
Modified: 2017-07-16 12:56 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config (1.82 KB, patch)
2017-06-05 22:23 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config (1.89 KB, patch)
2017-06-06 04:11 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config (1018 bytes, patch)
2017-06-09 04:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config (1.33 KB, patch)
2017-06-09 04:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config (1.47 KB, patch)
2017-06-09 17:42 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config (1.61 KB, patch)
2017-06-14 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2017-06-05 22:02:47 UTC
As a result:
prove t/SMS.t
generates noise:
Use of uninitialized value in subroutine entry at /home/mtompset/kohaclone/C4/SMS.pm line 98.
Comment 1 M. Tompsett 2017-06-05 22:23:00 UTC
Created attachment 64015 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This tweaks the tests to compensate and change for missing case.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone, test added
run koha qa test tools.
Comment 2 Josef Moravec 2017-06-06 04:11:29 UTC
Created attachment 64016 [details] [review]
[SIGNED-OFF] Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This tweaks the tests to compensate and change for missing case.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone, test added
run koha qa test tools.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Marcel de Rooy 2017-06-08 15:50:58 UTC
What about this line in C4/SMS:
if ( -f $conf_file ) {

If the file does not exist, it does not seem to hurt. You just do not have additional arguments.
But this patch makes it exit.

Please explain.
Comment 4 M. Tompsett 2017-06-09 04:35:21 UTC
(In reply to Marcel de Rooy from comment #3)
> What about this line in C4/SMS:
> if ( -f $conf_file ) {
> 
> If the file does not exist, it does not seem to hurt. You just do not have
> additional arguments.
> But this patch makes it exit.
> 
> Please explain.

Oh... right, probably not correct action.
Comment 5 M. Tompsett 2017-06-09 04:46:29 UTC
Created attachment 64120 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This quietly bypasses that.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.
Comment 6 M. Tompsett 2017-06-09 04:48:08 UTC
Created attachment 64121 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This silently bypasses the problem.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.
Comment 7 Lee Jamison 2017-06-09 17:42:16 UTC
Created attachment 64167 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This silently bypasses the problem.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Works correctly as indicated by the testing plan.
No "uninitialized" noise after patch is applied.
Comment 8 Jonathan Druart 2017-06-13 15:02:28 UTC
I do not think the behaviour is correct, C4::SMS::send_sms should just do nothing if the config is missing.
Comment 9 Marcel de Rooy 2017-06-14 14:30:56 UTC
Created attachment 64285 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This silently bypasses the problem.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Works correctly as indicated by the testing plan.
No "uninitialized" noise after patch is applied.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Slightly amended: turned the iif around.
Comment 10 Marcel de Rooy 2017-06-14 14:32:35 UTC
(In reply to Jonathan Druart from comment #8)
> I do not think the behaviour is correct, C4::SMS::send_sms should just do
> nothing if the config is missing.

Could be discussed on another report and perhaps solved. At first glance, the config seems to be something additional not required.
This just deals with eliminating a uninit warn.
Comment 11 Jonathan Druart 2017-06-14 14:38:42 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Jonathan Druart from comment #8)
> > I do not think the behaviour is correct, C4::SMS::send_sms should just do
> > nothing if the config is missing.
> 
> Could be discussed on another report and perhaps solved. At first glance,
> the config seems to be something additional not required.
> This just deals with eliminating a uninit warn.

Nope, please fix it here.
Comment 12 Marcel de Rooy 2017-06-14 14:39:43 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > (In reply to Jonathan Druart from comment #8)
> > > I do not think the behaviour is correct, C4::SMS::send_sms should just do
> > > nothing if the config is missing.
> > 
> > Could be discussed on another report and perhaps solved. At first glance,
> > the config seems to be something additional not required.
> > This just deals with eliminating a uninit warn.
> 
> Nope, please fix it here.

I respectfully disagree. This can be pushed imo.
Comment 13 M. Tompsett 2017-06-14 18:09:55 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > (In reply to Jonathan Druart from comment #8)
> > > I do not think the behaviour is correct, C4::SMS::send_sms should just do
> > > nothing if the config is missing.
> > 
> > Could be discussed on another report and perhaps solved. At first glance,
> > the config seems to be something additional not required.
> > This just deals with eliminating a uninit warn.
> 
> Nope, please fix it here.

"Any parameters with a leading underscore are considered private driver-specific options and will be passed through without alteration.

Any other parameters without a leading underscore will be silently stripped out and not passed through to the driver." (SMS::Send - http://search.cpan.org/~adamk/SMS-Send-1.06/lib/SMS/Send.pm)

The configuration file is used to set extraneous parameters based on the driver. It has nothing to do whether sending a message should be attempted or not.

In the case of SMS::Send::Test, it won't matter because it is a test driver. 

In the case of driver where it matters, they may not take extra parameters, so the configuration file is pointless and should be sent.

In the case where it matters, and the extra parameters are needed, the sending will fail and the warn $@; return; will be triggered.

This patch is correct.
Comment 14 Jonathan Druart 2017-06-14 18:59:43 UTC
Indeed, you are right.
Comment 15 Jonathan Druart 2017-06-15 19:09:32 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 16 Fridolin SOMERS 2017-06-20 08:27:55 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 17 Katrin Fischer 2017-06-21 06:27:14 UTC
Patch doesn't apply on 16.11.x - please rebase if you want to have this included.
Comment 18 Katrin Fischer 2017-07-16 12:56:39 UTC
Test is not noisy in 16.11.x. Closing.