Bug 21567

Summary: WebService:ILS related tests fail during package build
Product: Koha Reporter: Mirko Tietgen <mirko>
Component: Test SuiteAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, julian.maurice, lucas, nick, srdjan, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17602
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17602    
Bug Blocks:    
Attachments: Bug 21567: Use like instead of ok in RecordedBooks.t
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t
Bug 21567: Move Koha_ExternalContent_OverDrive.t to db_dependent
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t
Bug 21567: Move Koha_ExternalContent_OverDrive.t to db_dependent
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t

Description Mirko Tietgen 2018-10-14 11:42:23 UTC
Now that libwebservice-ils-perl is available to the nightly server, related tests fail:

1.

#   Failed test 'error_message()'
#   at t/Koha_ExternalContent_OverDrive.t line 52.
# Original:
# Can't connect to oauth.overdrive.com:443
# Request:
# POST https://oauth.overdrive.com/token
# Authorization: Basic RFVNTVk6RFVNTVk=
# User-Agent: Koha/18.06.00.041
# Content-Length: 126
# Content-Type: application/x-www-form-urlencoded
# 
# redirect_uri=http%3A%2F%2Fmykoha.org%2Fcgi-bin%2Fkoha%2Fexternal%2Foverdrive%2Fauth.pl&grant_type=authorization_code&code=blah
# 
# Response:
# 500 Can't connect to oauth.overdrive.com:443
# Content-Type: text/plain
# Client-Date: Sat, 13 Oct 2018 22:30:28 GMT
# Client-Warning: Internal response
# 
# Can't connect to oauth.overdrive.com:443
# 
# Name or service not known at /usr/share/perl5/LWP/Protocol/http.pm line 49.
# 
# Turned into:
# Can't connect to oauth.overdrive.com:443
# Looks like you failed 1 test of 5.
t/Koha_ExternalContent_OverDrive.t ...... 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests 


2.

#   Failed test 'Invalid RecordedBooks partner credentials'
#   at t/Koha_ExternalContent_RecordedBooks.t line 29.
# Looks like you failed 1 test of 3.
t/Koha_ExternalContent_RecordedBooks.t .. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 
	(less 1 skipped subtest: 1 okay)
Comment 1 Mirko Tietgen 2018-10-14 11:46:24 UTC
A different build fails only one of the tests

#   Failed test 'Invalid RecordedBooks partner credentials'
#   at t/Koha_ExternalContent_RecordedBooks.t line 29.
# Looks like you failed 1 test of 3.
t/Koha_ExternalContent_RecordedBooks.t .. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 
	(less 1 skipped subtest: 1 okay)

The other problem may be related to the pbuilder image then.
Comment 2 Mirko Tietgen 2018-10-14 11:59:59 UTC
t/Koha_ExternalContent_OverDrive.t ...... skipped: Need Test::DBIx::Class

That's why it does not fail on the other image.
Comment 3 Jonathan Druart 2018-10-16 15:49:02 UTC
Created attachment 80664 [details] [review]
Bug 21567: Use like instead of ok in RecordedBooks.t
Comment 4 Jonathan Druart 2018-10-16 15:50:13 UTC
I cannot recreate the failure and the output is not verbose enough. With this patch we could know what is the error.
Comment 5 Mirko Tietgen 2018-10-17 08:40:29 UTC
(In reply to Jonathan Druart from comment #4)
> I cannot recreate the failure and the output is not verbose enough. With
> this patch we could know what is the error.

Here is the more verbose output with the patch applied:

#   Failed test 'Invalid RecordedBooks partner credentials'
#   at t/Koha_ExternalContent_RecordedBooks.t line 29.
#                   'Can't connect to api.rbdigital.com:80
# Request:
# GET http://api.rbdigital.com/v1/libraries/DUMMY/search?all=art
# Authorization: Basic DUMMY
# User-Agent: libwww-perl/6.08
# 
# 
# Response:
# 500 Can't connect to api.rbdigital.com:80
# Content-Type: text/plain
# Client-Date: Wed, 17 Oct 2018 08:16:37 GMT
# Client-Warning: Internal response
# 
# Can't connect to api.rbdigital.com:80
# 
# Name or service not known at /usr/share/perl5/LWP/Protocol/http.pm line 49.
# '
#     doesn't match '(?^u:This endpoint can be called by authorized trusted app or trusted partner only)'
# Looks like you failed 1 test of 3.
t/Koha_ExternalContent_RecordedBooks.t .. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 
	(less 1 skipped subtest: 1 okay)
Comment 6 Jonathan Druart 2018-10-17 13:55:03 UTC
(In reply to Mirko Tietgen from comment #5)
> # Client-Warning: Internal response
> # 
> # Can't connect to api.rbdigital.com:80

So it's a network issue apparently
Comment 7 Mirko Tietgen 2018-10-18 08:39:45 UTC
Trying from the git checkout on the nightly server I get a pass. Reading the skip reason for test 3 I wonder if that is what is supposed to happen during package build, but does not for some reason.

t/Koha_ExternalContent_RecordedBooks.t .. 
1..3
ok 1 - use Koha::ExternalContent::RecordedBooks;
ok 2 - Invalid RecordedBooks partner credentials
ok 3 # skip no RecordedBooks partner credentials
ok
All tests successful.
Files=1, Tests=3,  4 wallclock secs ( 0.04 usr  0.00 sys +  2.50 cusr  0.33 csys =  2.87 CPU)
Result: PASS
Comment 8 Jonathan Druart 2018-10-18 12:37:27 UTC
The test is skipping if ENV RECORDEDBOOKS_TEST_LIBRARY_ID is not set, I have no idea where it comes from and so it will always be skipped as we never set it anywhere...

I let a comment on bug 17602.
Comment 9 Tomás Cohen Arazi 2018-11-14 13:42:29 UTC
This tests should be moved into db_dependent if external world interaction is expected. This tests could remain in t/ if WebService::ILS behaviour was completely mocked.

As-is, they block package building.
Comment 10 Tomás Cohen Arazi 2018-11-14 13:54:27 UTC
There's a similar file in t/db_dependent. This tests shoudl just get removed.
Comment 11 Tomás Cohen Arazi 2018-11-14 18:19:12 UTC
Created attachment 82344 [details] [review]
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t

This patch removes this file. The included tests are similar to those in
the db_dependent section, with the difference that this have some things
mocked. But the tests still require name resolution and connecting to
the RB servers. This situation breaks package building without any
noticeable gain.

The feature could be better tested by mocking WebService::ILS calls (so
no external world contact), but it belongs to a separate bug report.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Marcel de Rooy 2018-11-15 07:39:54 UTC
Created attachment 82352 [details] [review]
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t

This patch removes this file. The included tests are similar to those in
the db_dependent section, with the difference that this have some things
mocked. But the tests still require name resolution and connecting to
the RB servers. This situation breaks package building without any
noticeable gain.

The feature could be better tested by mocking WebService::ILS calls (so
no external world contact), but it belongs to a separate bug report.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Srdjan Jankovic 2018-11-15 12:10:23 UTC
Apologies, at the time I wrote it it was not breaking things, and it was useful for me to make sure it integrates well.

I could try to make it more friendly, just not sure if my test credentials are still valid.
Comment 14 Tomás Cohen Arazi 2018-11-15 12:21:15 UTC
(In reply to Srdjan Jankovic from comment #13)
> Apologies, at the time I wrote it it was not breaking things, and it was
> useful for me to make sure it integrates well.
> 
> I could try to make it more friendly, just not sure if my test credentials
> are still valid.

The tests in t/db_dependent/Koha_ExternalContent_RecordedBooks.t work, I haven't checked how much they cover the functionality, though.
The DB independent tests could also be implemented by mocking all WebService::ILS interactions, thus encapsulating external interactions.
Comment 15 Srdjan Jankovic 2018-11-15 23:53:31 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> The tests in t/db_dependent/Koha_ExternalContent_RecordedBooks.t work, I
> haven't checked how much they cover the functionality, though.

Without credentials - very little if any. With credentials reasonable amount.
Comment 16 Julian Maurice 2018-11-16 08:06:29 UTC
Just a thought: why not just skip the test if network is unavailable ?
Something like this: https://metacpan.org/pod/LWP::Online#Test-Mode
Comment 17 Mirko Tietgen 2018-11-16 08:21:11 UTC
(In reply to Julian Maurice from comment #16)
> Just a thought: why not just skip the test if network is unavailable ?
> Something like this: https://metacpan.org/pod/LWP::Online#Test-Mode

I don't think it is a network issue on my end. 

> # 500 Can't connect to oauth.overdrive.com:443

indicates we get out of the build machine, but not into the server, doesn't it? Other network tasks during build work ok. I'll have another look what is going on.

RecordedBooks is fixed with deleting the file. OverDrive would be skipped during package build if moved to db_dependent. But I don't know if it is the correct solution.
Comment 18 Mirko Tietgen 2018-11-16 10:02:00 UTC
Funny thing I just found while dissecting the build process …

> I: pbuilder: network access will be disabled during build

Which explains why network is fine for other tasks but the test fails. :D
Comment 19 Mirko Tietgen 2018-11-16 10:24:12 UTC
(In reply to Julian Maurice from comment #16)
> Just a thought: why not just skip the test if network is unavailable ?
> Something like this: https://metacpan.org/pod/LWP::Online#Test-Mode


(In reply to Tomás Cohen Arazi from comment #9)
> This tests should be moved into db_dependent if external world interaction
> is expected.



So, which one should it be for t/Koha_ExternalContent_OverDrive.t? I'd be in favour of moving the test, but I'd be ok with adding the dependency too. Any preferences?
Comment 20 Mirko Tietgen 2018-11-16 10:28:59 UTC
Created attachment 82393 [details] [review]
Bug 21567: Move Koha_ExternalContent_OverDrive.t to db_dependent
Comment 21 Mirko Tietgen 2018-11-16 12:17:24 UTC
Created attachment 82398 [details] [review]
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t

This patch removes this file. The included tests are similar to those in
the db_dependent section, with the difference that this have some things
mocked. But the tests still require name resolution and connecting to
the RB servers. This situation breaks package building without any
noticeable gain.

The feature could be better tested by mocking WebService::ILS calls (so
no external world contact), but it belongs to a separate bug report.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 22 Mirko Tietgen 2018-11-16 12:20:09 UTC
I signed off on Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t as a "Passed QA"

Setting to "Needs signoff" for Bug 21567: Move Koha_ExternalContent_OverDrive.t to db_dependent.

Nightly is all green with these to applied, let's get this over with.
Comment 23 Tomás Cohen Arazi 2018-11-16 13:02:53 UTC
Created attachment 82399 [details] [review]
Bug 21567: Move Koha_ExternalContent_OverDrive.t to db_dependent

This tests pass on the build step only because they are skipped, due to
the absense of Test::DBIx::Class library.

It makes sense for them to be in the db_dependent directory, as they are
not completely mocked (re: external world interactions).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2018-11-16 13:02:58 UTC
Created attachment 82400 [details] [review]
Bug 21567: Remove Koha_ExternalContent_RecordedBooks.t

This patch removes this file. The included tests are similar to those in
the db_dependent section, with the difference that this have some things
mocked. But the tests still require name resolution and connecting to
the RB servers. This situation breaks package building without any
noticeable gain.

The feature could be better tested by mocking WebService::ILS calls (so
no external world contact), but it belongs to a separate bug report.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 25 Nick Clemens 2018-11-16 13:09:40 UTC
Awesome work all!

Pushed to master for 18.11
Comment 26 Lucas Gass 2018-12-07 18:56:13 UTC
backported to 18.05 for 18.05.07
Comment 27 Mirko Tietgen 2018-12-11 16:58:51 UTC
Also needed in 17.11, adding Fridolin.
Comment 28 Fridolin Somers 2018-12-19 07:08:05 UTC
Depends on Bug 17602 not in 17.11.x