Bug 17441

Summary: t/db_dependent/Letters.t fails on Jenkins
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Test SuiteAssignee: Chris Cormack <chris>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: brendan, frederic, julian.maurice, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5260, 17446    
Attachments: Bug 17441 - t/db_dependent/Letters.t fails on Jenkins
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins
Bug 17441: [QA Follow-up] Return value of SendAlerts

Description Katrin Fischer 2016-10-13 17:07:55 UTC
Attached patch will fix t/db_dependent/Letters.t from failing on Jenkins. 
The code is from Jonathan, debugging the problem on my laptop.
Comment 1 Katrin Fischer 2016-10-13 17:21:57 UTC
Created attachment 56438 [details] [review]
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins

Run prove t/db_dependent/Letters.t
Comment 2 Claire Gravely 2016-10-14 08:06:29 UTC
Created attachment 56462 [details] [review]
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins

Run prove t/db_dependent/Letters.t

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Comment 3 Marcel de Rooy 2016-10-14 09:21:24 UTC
Created attachment 56491 [details] [review]
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins

Run prove t/db_dependent/Letters.t

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
To improve consistency, adding a follow-up.
Comment 4 Marcel de Rooy 2016-10-14 09:21:29 UTC
Created attachment 56492 [details] [review]
Bug 17441: [QA Follow-up] Return value of SendAlerts

This patch makes the return value of SendAlerts more consistent.
It returns 1 on success, or undef || { error => 'msg' } on failure.
Needed to adjust one test in Letters.t too.
Adjusted one typo along the way (seleted).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by running Letters.t.
Also tested SendAlerts from the interface with AutoEmailOpacUser and
memberentry (adding new patron).
Comment 5 Marcel de Rooy 2016-10-14 09:22:03 UTC
From Jenkins:
#   Failed test 'Successfully sent claim'
#   at t/db_dependent/Letters.t line 431.
#          got: undef
#     expected: '1'
# Looks like you failed 1 test of 74.
[15:18:31] t/db_dependent/Letters.t .................................. 

By returning 1, you will resolve this issue on Jenkins.
But the change itself is somewhat obscure now.

Looking further, we never really defined what SendAlerts should really return.
In most cases we call like: $err=SendAlerts(...)
In some cases SendAlerts warns and returns undef on an error.
Or carps and returns { error => ... }

Another line in Letters.t tests the results of SendAlerts with 
is($err2, "", "Successfully sent serial notification");

Added a follow-up to return 1 on success and add some consistency on the sendmail calls in SendAlerts. Adjusted the one line in Letters.t.

Passed QA
Comment 6 Brendan Gallagher 2016-10-14 09:45:38 UTC
Pushed to Master - Should be in the November 16.11 Release.   Thanks!
Comment 7 Frédéric Demians 2016-10-22 12:47:52 UTC
Pushed in 16.05. Will be in 16.05.05.
Comment 8 Julian Maurice 2016-10-24 10:28:50 UTC
Pushed in 3.22.x, will be in 3.22.12