Add calls to TestPackageAvailability in bugfix tests that load packages

[wilfwilson]

Background: the purpose of this PR is to resolve the initial problem that I encountered in #2809. Basically: I want to run tst/testbugfix.g in the Semigroups CI setup, as an extra check that Semigroups doesn't break things in GAP. But I can't be bothered installing the necessary packages that some of the tests use.

However, if I don't install these packages, then I get diffs because of the way that LoadPackage produces Info statements when it returns fail. This causes the tests to fail. See #2809 for more discussion about this topic. I think that the behaviour of LoadPackage with respect to Info statements is actually a bug, as I discuss on issue #2809. This PR doesn't resolve that 'bug', but I will attempt to fix that bug in a separate PR if someone agrees with me that there is a bug.

This is an example of a diff that arises when you don't have a package installed:

testing: /home/travis/gap/tst/testbugfix/2012-06-18-t00328.tst
########> Diff in /home/travis/gap/tst/testbugfix/2012-06-18-t00328.tst:2
# Input is:
if LoadPackage("anupq",false) <> fail then
for i in [1..192] do Q:=Pq( FreeGroup(2) : Prime:=3, ClassBound:=1 ); od; fi;
# Expected output:
# But found:
#I  anupq package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
########

This PR avoids these problems by replacing all occurrences if LoadPackage("blah") <> fail then in testbugfix test files with if TestPackageAvailability("blah") <> fail and LoadPackage("blah") <> fail then, because TestPackageAvailability prints no unexpected output when it returns fail. Therefore the test files 'pass' cleanly when the relevant package is not able to be loaded.

There is one exception: tst/testbugfix/2005-12-08-t00323.tst. Please look at the relevant commit. This is not formulated with an if statement, and just blindly loads the package. The test involves printing a table to the screen. Therefore, for the test file to pass without the package loaded, I add the if statement as before, and when the condition is not met, I manually print the expected output to screen. It feels weird to do so, but it seems to be the only way to ensure that testbugfix.g still passes when you don't have additional packages installed.

The obvious problem with my changes is that, if one of these packages stops loading for some reason, then this won't be captured by the testbugfix test files. If this happens and we don't notice that packages are failing to load, then we won't notice a potential regression of the relevant tests.

I'd like people's opinions on whether it is okay to do what I have done. I won't be offended by your comments: I made this PR at the suggestion of @alex-konovalov so it's not my idea anyway; I don't mind finding a different solution to my problem if others think that's best. Perhaps it would be better to move the bugfix files that require a package into a new collection? Perhaps we should ensure that such tests are also present in the relevant packages? Perhaps I should just get over it and install the relevant packages in my Travis setup...?

[fingolfin]

See also PR #1633 and the #@skipif extension to the .tst format it implements -- that could be applied to these tests, too, I think (but that doesn't mean we shouldn't merge this PR here as-is, just thought I'd bring it up for completeness.)

[fingolfin]

If we are using this a lot, perhaps it'd be better to add an option to LoadPackage to suppress warnings, and/or have a LoadPackageIfAvailable function? (That's not a change request for this PR, just thinking about how we could possibly refactor things in the future to make it easier to write this kind of logic)

[fingolfin]

This test is about verifying that launch a couple hundred subprocesses via InputOutputLocalProcess works.

[fingolfin]

This goes back to this commit (only available in my "secret" git history):

commit 614132d823d43e6f036e6352a54015b0889e77a7
Author: Frank Lübeck <frank.luebeck@math.rwth-aachen.de>
Date:   2012-06-18 12:43:23 +0000

    Avoid ConvertToMatrixRep and use Matrix. Now POW_MAT_INT also works
    for matrices in IsCMatRep.
    FL

diff --git a/lib/matrix.gi b/lib/matrix.gi
index 7ebde84e4..3db395796 100644
--- a/lib/matrix.gi
+++ b/lib/matrix.gi
@@ -4168,7 +4168,7 @@ BindGlobal("POW_MAT_INT", function(mat, n)
         until r <> true;
       fi;
     od;
-    ConvertToMatrixRep(t);
+    t := Matrix(t, m);
     return t;
   end;
   # compared to standard method, we avoid some zero or identity matrices

[fingolfin]

Oops, I just noticed that this PR is targeting stable-4.10. That means if merged, it would not go into master. Is that really what you want? But then the problem would persist in master resp. 4.11, would it not

So I think this should be rebased on master, and the backport-to-4.10 be added.

And of course it'd be good if other GAP team members had a look at this, too...

[wilfwilson]

Well-spotted @fingolfin. I will sort this out tomorrow at GAP Days

[wilfwilson]

@fingolfin How does the backporting work? I've rebased on master and added the label backport-to-4.10; if/when this PR is merged into master, will this then be cherry-picked into stable-4.10? Or do I then open another PR against stable-4.10?

[olexandr-konovalov]

@wilfwilson

when this PR is merged into master, will this then be cherry-picked into stable-4.10

yes, exactly - by one of the people who have appropriate access and who will then change the label from backport-to-4.10 to backport-to-4.10-DONE. No need for you to open a second PR.

[wilfwilson]

Thanks @alex-konovalov

[fingolfin]

Backported to stable-4.10 in da66cbc