search

gap-packages / Thelma

GAP Package on threshold logic.
https://gap-packages.github.io/Thelma
Other
1 stars 2 forks source link

thelma breaks testinstall.g in GAP #4

Closed olexandr-konovalov closed 5 years ago

olexandr-konovalov commented 5 years ago

One of the requirements for GAP packages is that they should not break tests from the GAP standard testsuite when they are loaded (see "76.18-4 Testing a GAP package with the GAP standard test suite" in https://www.gap-system.org/Manuals/doc/ref/chap76.html#X7C90C8B87BF6EF0B). This is not the case for Thelma: if it is loaded, then running tst/testinstall.g from GAP shows a number of diffs (27 in total), for example

########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pkg-update-stable-q\
uicktest-drop-in/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/install/label/kovacs/GA\
P-pkg-update-stable-snapshot/tst/testinstall/ffe.tst:344
# Input is:
Norm( AsField( GF(8), GF(8) ), GF(2), Z(8) );
# Expected output:
Z(2)^0
# But found:
1
########
########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pkg-update-stable-q\
uicktest-drop-in/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/install/label/kovacs/GA\
P-pkg-update-stable-snapshot/tst/testinstall/ffe.tst:350
# Input is:
Trace( GF(16), Z(4) );
# Expected output:
0*Z(2)
# But found:
0
########

They all seem to come from Thelma having a method that shows zero and one of a field differently. This has to be fixed - it is not recommended for a package to change the output of built-in objects defined in the core GAP system. So far it's not obvious to me from the overview that I've posted in #5.

olexandr-konovalov commented 5 years ago

P.S. most likely this will break many other tests, e.g. tst/teststandard.g, test of the GAP manual examples, and also tests that are contained in other GAP packages.

olexandr-konovalov commented 5 years ago

Apparently this will be fixed by https://github.com/vlaver/Thelma/commit/219b38ffb179b6d9a9aa1e05432b876c53eb3a79#diff-6dc370f80e66d2c325e10934df42f2b0 removing these two lines

SetNameObject(0*Z(2),"0");  
SetNameObject(Z(2)^0,"1");

I will now re-run GAP testsuite with Telma to check that nothing else is broken, then confirm if this issue may be closed.

vlaver commented 5 years ago

Apparently this will by 219b38f#diff-6dc370f80e66d2c325e10934df42f2b0 removing these two lines

SetNameObject(0*Z(2),"0");    
SetNameObject(Z(2)^0,"1");

I will now re-run GAP testsuite with Telma to check that nothing else is broken, then confirm if this issue may be closed.

Please note that I already deleted this lines and made the corresponding changes in "tst" files. So this release of Thelma (which is currently uploaded on github) should not break the tests that you have mentioned.

olexandr-konovalov commented 5 years ago

I've seen it - that's why I put a reference to the commit which fixed it:) Although it's not a good practice to make such huge commits which mix a number of unrelated updates - it will take much longer to see what has been done in the future if you will need to look at revisions history.

olexandr-konovalov commented 5 years ago

Thanks @vlaver for the fix, it worked!