Diffs in AtlasRep standard tests

[olexandr-konovalov]

The log below comes from Jenkins CI. It happens that AtlasRep's tst/testall.g file contains some tests that does not work when TERM = "dumb", the setting used in Jenkins CI.

Those tests pass fine when tst/testall.g is read into a usual GAP session, and do not take much time. They automatically open and close some Browse package windows, and I understand that the developers of AtlasRep would like to keep them in tst/testall.g to run regularly. Therefore, it seems for me that the best solution would be to omit them if TERM = "dumb" and run otherwise.

Additionally, tst/testall.g calls Test, but does not analyse its returned value. This does not permit automated detection of test failures. As said in https://github.com/gap-system/gap/wiki/Status-of-standard-tests-in-GAP-packages, the automated detection of the result of the test is relying on lines containing the substrings

#I  Errors detected while testing

or

#I  No errors detected while testing

(with exactly two spaces after #I), dependently on the test result. All that is needs is to calculate testresult and add the following lines

if testresult then
  Print("#I  No errors detected while testing\n");
else
  Print("#I  Errors detected while testing\n");
fi;

to the end of the test.

The mentioned log file is below.

============================OUTPUT START==============================

#I  RunPackageTests("atlasrep", "1.5.1", "tst/testall.g", auto);
 ���������������������������   GAP 4.8.8, 20-Aug-2017, build of 2017-08-20 19:40:04 (BST)
 ���  GAP  ���   https://www.gap-system.org
 ���������������������������   Architecture: x86_64-pc-linux-gnu-gcc-default64
 Libs used:  gmp, readline
 Loaded workspace: wsp.g
 Components: trans 1.0, prim 2.1, small* 1.0, id* 1.0
 Packages:   AClib 1.2, Alnuth 3.0.0, AtlasRep 1.5.1, AutPGrp 1.8, 
             Browse 1.8.7, CRISP 1.4.4, Cryst 4.1.12, CrystCat 1.1.6, 
             CTblLib 1.2.2, FactInt 1.5.4, FGA 1.3.1, GAPDoc 1.6, IO 4.4.6, 
             IRREDSOL 1.4, LAGUNA 3.7.0, Polenta 1.3.7, Polycyclic 2.11, 
             RadiRoot 2.7, ResClasses 4.6.0, Sophus 1.23, SpinSym 1.5, 
             TomLib 1.2.6, Utils 0.46
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
########> Diff in /data/gap-jenkins/workspace/GAP-minor-release-test/GAPCOPTS/\
64build/GAPGMP/gmp/GAPTARGET/packages/label/kovacs/gap4r8/pkg/atlasrep/tst/doc\
xpl.tst, line 871:
# Input is:
if IsBound( BrowseMinimalDegrees ) then
  down:= NCurses.keys.DOWN;;  DOWN:= NCurses.keys.NPAGE;;
  right:= NCurses.keys.RIGHT;;  END:= NCurses.keys.END;;
  enter:= NCurses.keys.ENTER;;  nop:= [ 14, 14, 14 ];;
  # just scroll in the table
  BrowseData.SetReplay( Concatenation( [ DOWN, DOWN, DOWN,
         right, right, right ], "sedddrrrddd", nop, nop, "Q" ) );
  BrowseMinimalDegrees();;
  # restrict the table to the groups with minimal ordinary degree 6
  BrowseData.SetReplay( Concatenation( "scf6",
       [ down, down, right, enter, enter ] , nop, nop, "Q" ) );
  BrowseMinimalDegrees();;
  BrowseData.SetReplay( false );
fi;
# Expected output:
# But found:
#I  cannot switch to visual mode because of TERM = "dumb"
#I  cannot switch to visual mode because of TERM = "dumb"
########
########> Diff in /data/gap-jenkins/workspace/GAP-minor-release-test/GAPCOPTS/\
64build/GAPGMP/gmp/GAPTARGET/packages/label/kovacs/gap4r8/pkg/atlasrep/tst/doc\
xpl.tst, line 887:
# Input is:
if IsBound( BrowseMinimalDegrees ) then
  # just scroll in the table
  BrowseData.SetReplay( Concatenation( [ DOWN, DOWN, DOWN, END ],
         "rrrrrrrrrrrrrr", nop, nop, "Q" ) );
  BrowseMinimalDegrees( BibliographySporadicSimple.groupNamesJan05 );;
fi;
# Expected output:
# But found:
#I  cannot switch to visual mode because of TERM = "dumb"
########
########> Diff in /data/gap-jenkins/workspace/GAP-minor-release-test/GAPCOPTS/\
64build/GAPGMP/gmp/GAPTARGET/packages/label/kovacs/gap4r8/pkg/atlasrep/tst/doc\
xpl.tst, line 895:
# Input is:
if IsBound( BrowseBibliographySporadicSimple ) then
  enter:= NCurses.keys.ENTER;;  nop:= [ 14, 14, 14 ];;
  BrowseData.SetReplay( Concatenation(
    # choose the application
    "/Bibliography of Sporadic Simple Groups", [ enter, enter ],
    # search in the title column for the Atlas of Finite Groups
    "scr/Atlas of finite groups", [ enter,
    # and quit
    nop, nop, nop, nop ], "Q" ) );
  BrowseGapData();;
  BrowseData.SetReplay( false );
fi;
# Expected output:
# But found:
#I  cannot switch to visual mode because of TERM = "dumb"
########
Input file: docxpl.tst
GAP4stones: 316
Input file: atlasrep.tst
GAP4stones: 1005
########> Diff in /data/gap-jenkins/workspace/GAP-minor-release-test/GAPCOPTS/\
64build/GAPGMP/gmp/GAPTARGET/packages/label/kovacs/gap4r8/pkg/atlasrep/tst/doc\
xpl.tst, line 871:
# Input is:
if IsBound( BrowseMinimalDegrees ) then
  down:= NCurses.keys.DOWN;;  DOWN:= NCurses.keys.NPAGE;;
  right:= NCurses.keys.RIGHT;;  END:= NCurses.keys.END;;
  enter:= NCurses.keys.ENTER;;  nop:= [ 14, 14, 14 ];;
  # just scroll in the table
  BrowseData.SetReplay( Concatenation( [ DOWN, DOWN, DOWN,
         right, right, right ], "sedddrrrddd", nop, nop, "Q" ) );
  BrowseMinimalDegrees();;
  # restrict the table to the groups with minimal ordinary degree 6
  BrowseData.SetReplay( Concatenation( "scf6",
       [ down, down, right, enter, enter ] , nop, nop, "Q" ) );
  BrowseMinimalDegrees();;
  BrowseData.SetReplay( false );
fi;
# Expected output:
# But found:
#I  cannot switch to visual mode because of TERM = "dumb"
#I  cannot switch to visual mode because of TERM = "dumb"
########
########> Diff in /data/gap-jenkins/workspace/GAP-minor-release-test/GAPCOPTS/\
64build/GAPGMP/gmp/GAPTARGET/packages/label/kovacs/gap4r8/pkg/atlasrep/tst/doc\
xpl.tst, line 887:
# Input is:
if IsBound( BrowseMinimalDegrees ) then
  # just scroll in the table
  BrowseData.SetReplay( Concatenation( [ DOWN, DOWN, DOWN, END ],
         "rrrrrrrrrrrrrr", nop, nop, "Q" ) );
  BrowseMinimalDegrees( BibliographySporadicSimple.groupNamesJan05 );;
fi;
# Expected output:
# But found:
#I  cannot switch to visual mode because of TERM = "dumb"
########
########> Diff in /data/gap-jenkins/workspace/GAP-minor-release-test/GAPCOPTS/\
64build/GAPGMP/gmp/GAPTARGET/packages/label/kovacs/gap4r8/pkg/atlasrep/tst/doc\
xpl.tst, line 895:
# Input is:
if IsBound( BrowseBibliographySporadicSimple ) then
  enter:= NCurses.keys.ENTER;;  nop:= [ 14, 14, 14 ];;
  BrowseData.SetReplay( Concatenation(
    # choose the application
    "/Bibliography of Sporadic Simple Groups", [ enter, enter ],
    # search in the title column for the Atlas of Finite Groups
    "scr/Atlas of finite groups", [ enter,
    # and quit
    nop, nop, nop, nop ], "Q" ) );
  BrowseGapData();;
  BrowseData.SetReplay( false );
fi;
# Expected output:
# But found:
#I  cannot switch to visual mode because of TERM = "dumb"
########
Input file: docxpl.tst
GAP4stones: 322
Input file: atlasrep.tst
GAP4stones: 1107
#I  RunPackageTests("atlasrep", "1.5.1", "tst/testall.g", auto): runtime 81958
============================OUTPUT END================================

[olexandr-konovalov]

Also notified @ThomasBreuer by email.

[olexandr-konovalov]

I have found a setting (calling the test with TERM=xterm) which removes all #I cannot switch to visual mode because of TERM = "dumb" messages.

Now in the stable-4.8 branch AtlasRep tests pass (https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-staging/jobs/331189400) but that can not be detected automatically because of the reasons explained above.

In the stable-4.9 branch tests status is as follows:

With no packages loaded (GAP started with -r option) :  4 DIFFS
With all packages loaded with LoadAllPackages()      :  2 DIFFS
With default packages loaded at GAP startup          :  2 DIFFS

with diffs being

########> Diff in /home/gap/inst/gap-stable-4.9/pkg/atlasrep/tst/docxpl.tst:61
# Input is:
NrMovedPoints( Image( hom ) );
# Expected output:
1540
# But found:
266
########
########> Diff in /home/gap/inst/gap-stable-4.9/pkg/atlasrep/tst/docxpl.tst:
413
# Input is:
StructureDescription( stab );
# Expected output:
"(A4 x A4) : C4"
# But found:
"(C2 x C2 x C2 x C2) : ((C3 x C3) : C4)"
########

Full logs can be viewed at https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.9-staging/jobs/327912054

P.S. @ThomasBreuer reported the regression with StructureDescription in #2109.

[olexandr-konovalov]

The StructureDescription diff is triggered by not loading Crisp package (see #2109). In this case, if the point of the example is to check that stab has correct isomorphism type, one could replace it by

gap> IdGroup( stab );
[ 576, 8652 ] 

which should not depend on loaded packages.

[ThomasBreuer]

Of course not, we all should know that StructureDescription cannot be used to check isomorphism types.

The example is part of the tutorial, and the point is to express that the group has the claimed structure.
In this situation, a group ID is as useless as the new default StructureDescription output.

[olexandr-konovalov]

I see... Then perhaps the nice option, as @fingolfin suggested elsewhere, is suitable? The other diff in the example about NrMovedPoints seems to be a smaller degree permutation representation that GAP 4.9 will produce, so it just needs to be updated to the new output when you will be making a new release.

[olexandr-konovalov]

@ThomasBreuer AtlasReps tests still do not pass in the stable-4.10 branch, see for example the test log at https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10-staging/jobs/438054207. For packages whose tests do not pass cleanly it's very easy to overlook when they become broken because of changes in GAP and/or other packages. Therefore, it will be very useful if AtlasRep tests can be fixed.

First, even the following small changes in testall.g will allow automated detection of test failures in CI scripts:

LoadPackage( "atlasrep" );

dirs:= DirectoriesPackageLibrary( "atlasrep", "tst" );

# Make sure that the component is bound to either `true' or `false'.
if not IsBound( CMeatAxe.FastRead ) or CMeatAxe.FastRead <> true then
  CMeatAxe.FastRead:= false;
fi;

testresult:=true;

# Run the standard tests with this value.
testresult := testresult and Test( Filename( dirs, "docxpl.tst" ) );
testresult := testresult and Test( Filename( dirs, "atlasrep.tst" ) );

# Now run the tests with the other value.
CMeatAxe.FastRead:= not CMeatAxe.FastRead;
testresult := testresult and Test( Filename( dirs, "docxpl.tst" ) );
testresult := testresult and Test( Filename( dirs, "atlasrep.tst" ) );

# Reset the value.
CMeatAxe.FastRead:= not CMeatAxe.FastRead;

if testresult then
  Print("#I  No errors detected while testing\n");
  QUIT_GAP(0);
else
  Print("#I  Errors detected while testing\n");
  QUIT_GAP(1);
fi;

FORCE_QUIT_GAP(1); # if we ever get here, there was an error

Second, there is currently a diff when GAP is started with -A option:

########> Diff in /home/gap/inst/gap-stable-4.10/pkg/atlasrep/tst/docxpl.tst:
413
# Input is:
StructureDescription( stab );
# Expected output:
"(A4 x A4) : C4"
# But found:
"(C2 x C2 x C2 x C2) : ((C3 x C3) : C4)"
########

It was discussed in #2109, and a "nice" option has been suggested there - is it suitable?

Note that now they are supposed to pass for 87 packages at https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10 and fail for 12 at https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10-staging, so the coming GAP 4.10 will have package tests in the state like never before. But there is still a space to reduce the latter number even further down to 10 or less in GAP 4.10.

[olexandr-konovalov]

@ThomasBreuer we have published GAP 4.10.0.

Unfortunately, no new AtlasRep release appeared, so as you can see at https://travis-ci.org/gap-system/gap-docker-pkg-tests, it is one of the 6 packages (out of 103) for which we can not report that their tests pass.

Furthermore, among those 6 packages there are two which are loaded by default (AtlasRep and CTbliLib) and it's quite unlucky that we do not check that they work as expected as thoroughly as for many other packages.

Therefore, are you happy with the testall.g file proposed above? Will you be able to publish a new release, say, this year, please? It could also address suggestions from #2237 and #2791. Do you have a repository for AtlasRep already? We will be happy to create one for you, and populate it with past releases (with one-to-one correspondence between releases and commits) - please let us know.

[olexandr-konovalov]

@ThomasBreuer the problem with StructureDescription fixed in the latest AtlasRep release, thanks.

Unfortunately, there is still no way now to detect whether tests pass or fail automatically for the package tests on Travis, so I am keeping AtlasRep in the "staging" tests:

• https://travis-ci.org/gap-infra/gap-docker-pkg-tests-master-staging
• https://travis-ci.org/gap-infra/gap-docker-pkg-tests-stable-4.10-staging

Hence I am asking you to keep an eye on them and check that changes in GAP master and stable-4.10 branches do not break the test.

If you would like to be notified when tests fail, rather than watch those tests yourself, there is a suggestion which I am reiterating: use tst/testall.g with the content below and specify it as a test file in the PackageInfo.g:

LoadPackage( "atlasrep" );

dirs:= DirectoriesPackageLibrary( "atlasrep", "tst" );

# Make sure that the component is bound to either `true' or `false'.
if not IsBound( CMeatAxe.FastRead ) or CMeatAxe.FastRead <> true then
  CMeatAxe.FastRead:= false;
fi;

testresult:=true;

# Run the standard tests with this value.
testresult := testresult and Test( Filename( dirs, "docxpl.tst" ) );
testresult := testresult and Test( Filename( dirs, "atlasrep.tst" ) );

# Now run the tests with the other value.
CMeatAxe.FastRead:= not CMeatAxe.FastRead;
testresult := testresult and Test( Filename( dirs, "docxpl.tst" ) );
testresult := testresult and Test( Filename( dirs, "atlasrep.tst" ) );

# Reset the value.
CMeatAxe.FastRead:= not CMeatAxe.FastRead;

if testresult then
  Print("#I  No errors detected while testing\n");
  QUIT_GAP(0);
else
  Print("#I  Errors detected while testing\n");
  QUIT_GAP(1);
fi;

FORCE_QUIT_GAP(1); # if we ever get here, there was an error

[ThomasBreuer]

@alex-konovalov
Apparently you expect a particular file format for the package tests.
I did not find the definition of this format in the documentation.
Natural places for this definition or for links to it would be the sections TestPackage in the Reference Manual and Tests for packages in the Development Manual, perhaps there are more such places.
Pointing to the documentation would be both easier for you and more appropriate for me.

Concerning AtlasRep, I had already discussed with @fingolfin that I will provide a file in the abovementioned format (or a newer one if that is outdated by then) with the next released version of the package.

(Besides that, testall.g is not suitable for your kinds of tests, I had created testauto.g for that purpose.)

[olexandr-konovalov]

@ThomasBreuer thanks - when you will be releasing AtlasRep update, please also note that it has the following warnings when it is loaded with OnlyNeeded option:

%%% Loading atlasrep 2.1.0 , only needed
Syntax warning: Unbound global variable in /circa/scratch/gap-jenkins/workspac\
e/GAP-pkg-update-stable-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/packag\
esload/label/kovacs/GAP-pkg-update-stable-snapshot/pkg/atlasrep/gap/test.g:661
        if HasStandardGeneratorsInfo( tom ) then
           ^^^^^^^^^^^^^^^^^^^^^^^^^
Syntax warning: Unbound global variable in /circa/scratch/gap-jenkins/workspac\
e/GAP-pkg-update-stable-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/packag\
esload/label/kovacs/GAP-pkg-update-stable-snapshot/pkg/atlasrep/gap/test.g:662
          info:= StandardGeneratorsInfo( tom );
                 ^^^^^^^^^^^^^^^^^^^^^^
Syntax warning: Unbound global variable in /circa/scratch/gap-jenkins/workspac\
e/GAP-pkg-update-stable-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/packag\
esload/label/kovacs/GAP-pkg-update-stable-snapshot/pkg/atlasrep/gap/test.g:360\
5
                result.subgroup:= StructureDescriptionCharacterTableName(
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Perhaps TomLib and CTblLib should be made needed packages.

[olexandr-konovalov]

@ThomasBreuer also AtlasRep uses obsolete USER_HOME_EXPAND:

Syntax warning: Unbound global variable in /circa/scratch/gap-jenkins/workspac\
e/GAP-pkg-update-stable-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/obsole\
tes/label/kovacs/GAP-pkg-update-stable-snapshot/pkg/atlasrep/gap/access.gi:256\
8
        firstarg:= USER_HOME_EXPAND( firstarg );
                   ^^^^^^^^^^^^^^^^

[fingolfin]

I wonder what the status of this is now...

@ThomasBreuer perhaps we can briefly talk about this tomorrow? or next week, it's not urgent

[ThomasBreuer]

@fingolfin Yes, we can.

Concerning the status with AtlasRep, the latest released version is 2.1.2, and the abovementioned problems should have disappeared in this version.

(For the release of GAP 4.12.0 or 4.12.1, it will make sense to release AtlasRep 2.1.3, due to the changed address of the data server, and due to the new HexSHA256 function of GAP 4.12.)

[fingolfin]

AtlasRep 2.1.1 is passing all test in the PackageDistro, see https://github.com/gap-system/PackageDistro/blob/data/reports/master/2022-07-01-03:21:56-6b7f6a77/report.md -- so this can be closed. Thanks @ThomasBreuer !

As to making 2.1.3 which requires GAP 4.12.0: that's no problem; I'd recommend to release it in early August, then we can merge it together with matrgrp and the new semigroups version around that time and include it with 4.12.0