Bug 24756

Created attachment 99746 [details] [review]
Bug 24756: Fix D8 and U18 failures for Koha/XSLT/Security.t

Instead of warning_like, we eventually catch multiple warnings and look
if we catch one specific warn and not catch another specific one.

Test plan:
Run t/db_dependent/Koha/XSLT/Security.t on D8, D9, D10 or U18.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Passes on D8 and D9 for me.

Another option is to just remove this test. In the script we test the read_net case already. This is just something additional that we could miss too..

Skipping QA so we can see how Jenkins responds. Certainly, no harm will be done by this change :)

Nice work everyone!

Pushed to master for 20.05

U18 on master:

not ok 7 - Expected import error
ok 8 - No read_net warn for remote import

Yeah, skip that skip. D9 failed for MDB?

Created attachment 99754 [details] [review]
Bug 24756: Fix failing tests by removing temporary filenames

In order to resolve the random failure, that might be caused by reusing
filenames in File::Temp and caching xslt code, this patch replaces the
temporary file by using the code parameter.

Note: Since XSLT uses a digest based on the passed code fragment, we will
exclude collisions here.

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

Martin,
Assuming that the failure might be in a combination of not unique file names and caching somehow, I added a second patch and tested it on D8, D9 and D10.
Hopefully catching it now..

(In reply to Marcel de Rooy from comment #10)
> Martin,
> Assuming that the failure might be in a combination of not unique file names
> and caching somehow, I added a second patch and tested it on D8, D9 and D10.
> Hopefully catching it now..

Maybe I added the literal files once just for that reason. Who knows?

Master D10 #100 29-02-2020 19:45 
not ok 7 - Expected import error
ok 8 - No read_net warn for remote import
Master D9 My8 #179 28-02-2020 15:57
idem not ok 7, ok 8

Created attachment 99858 [details] [review]
Bug 24756: Show the warnings on Jenkins

Temporary measure to see which warnings Jenkins got while we cannot
reproduce them ourselves on D8, D9 and D10.

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

This produces output (in my D9 testing environment) like:

ok 7 - Expected import error. Additional info: $VAR1 = 'XSLT::Base: No such file or directoryI/O warning : failed to load external entity "http://notexpected.koha-community.org/noxsl/nothing.xsl"
# compilation error: file unknown-558d811413c0 element import
# xsl:import : unable to load http://notexpected.koha-community.org/noxsl/nothing.xsl
#  at /usr/share/koha/Koha/XSLT/Base.pm line 343.
# ';
#
ok 8 - No read_net warn for remote import

Hopefully we'll know more when we run this on Jenkins?

(In reply to Marcel de Rooy from comment #14)
> Hopefully we'll know more when we run this on Jenkins?

=== https://jenkins.koha-community.org/job/Koha_Master_D8/725/
not ok 7 - Expected import error. Additional info: $VAR1 = 'XSLT::Base: I/O warning : failed to load HTTP resource
# compilation error: file unknown-3d22ae0 element import
# xsl:import : unable to load http://notexpected.koha-community.org/noxsl/nothing.xsl
#  at /kohadevbox/koha/Koha/XSLT/Base.pm line 343.
# ';
# 
ok 8 - No read_net warn for remote import

Here the wording "failed to load HTTP resource" is used instead of "failed to load external entity". It appears that both can come up.
libxml2 contains both strings
  failed to load external entity "%s"
  failed to load HTTP resource "%s"
and even:
  Failed to load externalRef %s
  Failed to load include %s
(But we did not see the last two.)

So we probably just need:
    is( ( grep { /failed to load (external entity|HTTP resource)/ } @warn ), 1, 'Expected import error' );

I will wait a few days now before replacing the additional Dumper info.

Created attachment 99892 [details] [review]
Bug 24756: Catch two failed to load-variations in Security.t

Test 7 in Security.t failed when 'failed to load HTTP resource' was
returned by libxml2 instead of 'failed to load external entity'.

We now remove the debugging with Dumper again and adjust the regex to
include both variations.

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

Nice work everyone!

Pushed to master for 20.05

It seems that we need it for 19.11 as well

19.11_D9 (79)

koha_1       | #   Failed test 'Remote import does not fail on read_net'
koha_1       | #   at t/db_dependent/Koha/XSLT/Security.t line 124.
koha_1       | # found warning: XSLT_Handler: I/O warning : failed to load HTTP resource
koha_1       | # expected to find warning: (?^u:I/O warning : failed to load external entity)
koha_1       | # Looks like you failed 1 test of 7.

(In reply to Jonathan Druart from comment #18)
> It seems that we need it for 19.11 as well
> 
> 19.11_D9 (79)
> 
> koha_1       | #   Failed test 'Remote import does not fail on read_net'
> koha_1       | #   at t/db_dependent/Koha/XSLT/Security.t line 124.
> koha_1       | # found warning: XSLT_Handler: I/O warning : failed to load
> HTTP resource
> koha_1       | # expected to find warning: (?^u:I/O warning : failed to load
> external entity)
> koha_1       | # Looks like you failed 1 test of 7.

This should be backported, yes. The RMaints look in this queue, right?

this does not apply cleanly to 19.11.x branch.  Please rebase. 
Thanks!

(In reply to Joy Nelson from comment #20)
> this does not apply cleanly to 19.11.x branch.  Please rebase. 
> Thanks!

Hi Joy.
Actually you only need the last patch marked 19.11.X.

Created attachment 102271 [details] [review]
Bug 24756: [19.11.X] Catch two failed to load-variations in Security.t

Test 7 in Security.t failed when 'failed to load HTTP resource' was
returned by libxml2 instead of 'failed to load external entity'.

We now remove the debugging with Dumper again and adjust the regex to
include both variations.

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

backported 19.11 patch to 19.11.x branch for 19.11.05

backported to 19.05.x for 19.05.10