gap-packages / cryst

Computing with crystallographic groups
https://www.math.uni-bielefeld.de/~gaehler/gap/packages.php#Cryst
Other
3 stars 6 forks source link

Set up Travis & CodeCov #5

Closed olexandr-konovalov closed 6 years ago

olexandr-konovalov commented 6 years ago

This is our standard setup already used by many packages in this organisation. With its help, you will be able to monitor the status of the tests of your package and code coverage.

olexandr-konovalov commented 6 years ago

@gaehler Test failed in one of the three configurations, namely in 32-bit build. So Travis and CI setup works, and this may be merged. I've also reported https://github.com/gap-packages/carat/issues/2.

The test failure could be caused by Carat failed to build in 32-bit case. I've found diffs in the test. Are they caused by missing Carat, or anything else?

########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
cryst.tst:122
# Input is:
c := ConjugatorSpaceGroupsStdSamePG( S2, S );;
# Expected output:
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 3rd choice method found for `NormalizerInGLnZ' on 1 arguments
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
cryst.tst:123
# Input is:
S2^c=S;
# Expected output:
true

# But found:
false
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
cryst.tst:140
# Input is:
C  := ConjugatorSpaceGroups( S1, S2 );
# Expected output:
[ [ -4/13, -2/13, 0 ], [ -1/13, -20/13, 0 ], [ -3343/4095, -6221/455, 1 ] ]

# But found:
[ [ 4/13, 2/13, 0 ], [ 1/13, 20/13, 0 ], [ 4253/4095, 9021/455, 1 ] ]
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
manual.tst:40
# Input is:
norm := GeneratorsOfGroup( NormalizerInGLnZ( P ) );
# Expected output:
[ [ [ -1, 0 ], [ 0, -1 ] ], [ [ -1, 0 ], [ 0, 1 ] ], [ [ -1, 0 ], [ 0, -1 ] ],
  [ [ 1, 0 ], [ 0, -1 ] ], [ [ 0, 1 ], [ 1, 0 ] ] ]

# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 2nd choice method found for `NormalizerInGLnZ' on 1 arguments
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
manual.tst:48
# Input is:
SpaceGroupsByPointGroupOnRight( P, norm );
# Expected output:
[ <matrix group with 4 generators>, <matrix group with 4 generators>, 
  <matrix group with 4 generators> ]

# But found:
Error, Variable: 'norm' must have a value
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
manual.tst:52
# Input is:
SpaceGroupsByPointGroupOnRight( P, norm, true );
# Expected output:
[ [ <matrix group with 4 generators> ], 
  [ <matrix group with 4 generators>, <matrix group with 4 generators> ], 
  [ <matrix group with 4 generators> ] ]

# But found:
Error, Variable: 'norm' must have a value
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
manual.tst:57
# Input is:
SpaceGroupTypesByPointGroupOnRight( P );
# Expected output:
[ <matrix group with 4 generators>, <matrix group with 4 generators>, 
  <matrix group with 4 generators> ]

# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 2nd choice method found for `NormalizerInGLnZ' on 1 arguments
########
########> Diff in /home/travis/build/gap-packages/cryst/gaproot/pkg/cryst/tst/\
manual.tst:61
# Input is:
SpaceGroupTypesByPointGroupOnRight( P, true );
# Expected output:
[ [ <matrix group with 4 generators> ], 
  [ <matrix group with 4 generators>, <matrix group with 4 generators> ], 
  [ <matrix group with 4 generators> ] ]

# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 2nd choice method found for `NormalizerInGLnZ' on 1 arguments
########
gaehler commented 6 years ago

Do I have to make a new release including the added files? Or include them in future releases? Are the two extra lines at the beginning of the README mandatory, or just informational? It seems to me that they do at least not belong to the top of the REAMDE, even more as they are meaningful only in an .md file.

Regarding the failed tests: yes, they depend on Carat being available. I could possibly move them to a separate file, which is tested only if Carat is loaded. I am a bit puzzled though that Carat does not build/work in 32bit mode. But I'm working on the build system of Carat anyway, and will check it.

olexandr-konovalov commented 6 years ago

These new files need not to be in the released version, neither they do require a new release.

When you will be making a new release using ReleaseTools, they will however be included in the package archive. This is discussed by @fingolfin in https://github.com/gap-system/ReleaseTools/issues/49. Anyhow, they add a tiny overhead to the archive, and it may be neglected considering the value and convenience of using ReleaseTools.

The two extra lines are very convenient for a quick navigation from the repository to Travis and CodeCov pages for your package - see e.g. https://github.com/gap-packages/example. It is also good to show the status of your code to users. But they are not mandatory, so if you don't want them, I can edit my PR and remove them.

gaehler commented 6 years ago

In other words, the tests are done on the version of (the master branch of) the GitHub repository? This reminds me of not pushing unfinished stuff to the master branch. Also, it requires all relevant stuff to be in the repository. That is not a problem for Cryst and CrystCat, but has to be kept in mind for Carat.

As for tests run only only conditionally on Carat being available: this is actually more difficult than I anticipated. Any idea how this can be done? A simple if GAPInfo.PackagesLoaded.carat around START_TEST and STOP_TEST doesn't seem to do the job.

olexandr-konovalov commented 6 years ago

@gaehler

1) the tests will be done not only on any push to the master branch, but also much better - on each pull request! Therefore the strategy is not to avoid "pushing unfinished stuff to the master branch" but to "push untested stuff to a branch and submit pull request in order to test it before merging into master".

2) There is no need to keep ALL stuff in the repository - it can be downloaded during the build. For Carat, see https://github.com/gap-packages/carat/issues/3.

3) An example of a test dependable on whether some package is loaded: https://github.com/gap-system/gap/blob/master/tst/testextra/ctblpope.tst - will it be possible to use that approach?