|
Description
Mirko Tietgen
2018-10-14 11:42:23 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. t/Koha_ExternalContent_OverDrive.t ...... skipped: Need Test::DBIx::Class That's why it does not fail on the other image. Created attachment 80664 [details] [review] Bug 21567: Use like instead of ok in RecordedBooks.t I cannot recreate the failure and the output is not verbose enough. With this patch we could know what is the error. (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) (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 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 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. 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. There's a similar file in t/db_dependent. This tests shoudl just get removed. 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> 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> 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. (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. (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. 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 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. 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
(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? Created attachment 82393 [details] [review] Bug 21567: Move Koha_ExternalContent_OverDrive.t to db_dependent 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> 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. 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> 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> Awesome work all! Pushed to master for 18.11 backported to 18.05 for 18.05.07 Also needed in 17.11, adding Fridolin. Depends on Bug 17602 not in 17.11.x |