Bug 17441 - t/db_dependent/Letters.t fails on Jenkins
Summary: t/db_dependent/Letters.t fails on Jenkins
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Chris Cormack
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 5260 17446
  Show dependency treegraph
 
Reported: 2016-10-13 17:07 UTC by Katrin Fischer
Modified: 2017-06-14 22:13 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins (720 bytes, patch)
2016-10-13 17:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins (780 bytes, patch)
2016-10-14 08:06 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 17441 - t/db_dependent/Letters.t fails on Jenkins (928 bytes, patch)
2016-10-14 09:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17441: [QA Follow-up] Return value of SendAlerts (3.22 KB, patch)
2016-10-14 09:21 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 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