Test manualexamples #1082

markuspf · 2017-01-17T10:09:18Z

This is not yet finished, and rather hacky. Feel free to comment though.

At the moment a lot of the tests fail because of screen width problems etc.

markuspf · 2017-01-17T10:09:32Z

First stab at addressing #1076

olexandr-konovalov · 2017-01-17T10:20:51Z

@markuspf thanks. Have you seen my last comment in #1076 btw?

Immediate remark: this

########> Diff in /home/travis/build/gap-system/gap/tst/testmanuals/chapter64.\
tst:594
# Input is:
Basis(L)[17]^vv[100];
# Expected output:
-1*y5*y7*y8*v0-1*y5*y9*v0
 ]
# But found:
-1*y5*y7*y8*v0-1*y5*y9*v0
########

refers to lines in the test file. One should now go there to figure out from which source file this example is taken (if this is recorded at all). To the contrary, the current setup refers to the actual location of the manual example, avoiding such indirection. IMHO this is an important feature.

olexandr-konovalov · 2017-01-17T10:33:47Z

@markuspf cf. pkg/wedderga/makedocrel.g - I am extracting examples from he package manual there. Produced test files are not kept under version control but are included in the package release. This involves redirection, but at least the test files records the location of the original example in the comment.

codecov-io · 2017-01-17T10:55:43Z

Current coverage is 56.88% (diff: 100%)

Merging #1082 into master will increase coverage by 0.05%

@@             master      #1082   diff @@
==========================================
  Files           433        434     +1   
  Lines        224901     224996    +95   
  Methods        3447       3447          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         127806     127989   +183   
+ Misses        97095      97007    -88   
  Partials          0          0

Powered by Codecov. Last update 34f3178...421cf25

markuspf · 2017-01-17T12:46:48Z

This is starting to look better after I hit it repeatedly with a hammer.

markuspf · 2017-01-17T13:00:26Z

This fragility is making me really sad.

markuspf · 2017-01-17T15:16:04Z

There are still a few tests that fail, maybe we should ignore these failures in travis and just add the improved coverage to codecov though. Feel free to chase up the remaining diffs...

fingolfin · 2017-01-17T16:24:21Z

First, off: Thanks for working on this.

As @alex-konovalov already pointed out, some of those test failures are spurious, either due to randomness, irrelevant side effects, etc. etc. I think we really should try to fix these properly. Becaue what Alex has been doing manually (i.e. curating the test results with a lot of manual work) simply does not scale.

So, it would indeed be good if people could help to improve this. Some things that will help:

Merge PR Fix regression in group constructors for matrix groups #1077 (which so far has not been reviewed hint hint ;-).
For some tests, we may wish to suppress some output
For the AssignGeneratorVariables we might want to modify the InfoWarning and/or InfoGlobal level (just for the manual tests, perhaps?)
Some tests should simply be changed from <Example> to <Log> (both look the same to users, but only the former gets tested, not the latter)
Improve our test system (we need to do that anyway), e.g. to allow us to insert meta data which indicates splits in the tests inside of chapters. As a minimal version, that might be some kind of marker which tells us "test everything up to this point; then reset everything, e.g. by restarting GAP" -- right now, we only do that at chapter borders, which may be too broad.
In some cases, the outputs in the manual simply may be out of date and should be updated...

Hmm... Markus, could you perhaps hack this some more, and add two more travis tests, with gap -A and with LoadAllPackages? It would be really interesting to know how bad those fail. (To do this, I would retain a single testmanuals.g, but add environment variables which specifies the extra modes... perhaps GAPFLAGS=-A and then change etc/ci.sh to pass those to gap.sh; and a second env which testmanuals.g checks for and if set, it executes LoadAllPackages()

(Note: I would try it on my local machine, but I am afraid that my set of packages is vastly different from what is bundled with GAP; in fact, for me LoadAllPackages() runs into an error; I can reenter it several times to get more and more packages, but usually it eventually causes GAP to crash...)

fingolfin · 2017-01-17T16:28:56Z

It seems that a ton of diffs are also caused by X(Rationals,1) having a non-default name (x instead of x_1). But that's very strange: If I manually run the tests (e.g. Test("tst/testmanuals/chapter58.tst"); then this diff does not crop up....

Oh wait: You are not restarting GAP after every chapter, are you? That would explain a big bunch of these diffs...

markuspf · 2017-01-17T18:36:34Z

I am indeed not restarting GAP for every test.

I have a hacked up version of TestDirectory that can use IO_fork to run the tests, not sure yet whether this is a good idea though.

ChrisJefferson · 2017-01-17T19:03:27Z

Just as a point, I've had to make a separate hack based on IO_fork for the profiling package, as I also need to run each test in a separate GAP (my hack was far too hacky to be in GAP). I think it's a sensible thing to do.

fingolfin · 2017-01-17T19:07:13Z

Well, if you have working code, fine.. though what I would do is create a shell script which loops over the test files and starts GAP with each of them. This could then also use a workspace to speed up things a bit. Of course a little bit extra work is needed if we don't want to abort the script after the first chapter with failures.

This could then also be used on Windows (if we wanted to), while IO_fork might be problematic there...

Anyway, whatever works is fine by me.

fingolfin · 2017-01-18T19:08:13Z

tst/testmanuals.g

+
+# RS := rec(changeSources := true);
+# WS := rec(compareFunction := "uptowhitespace");
+# WSRS := rec(changeSources := true, compareFunction := "uptowhitespace");


What are these comments about?

fingolfin · 2017-01-18T19:09:03Z

tst/testmanuals.g

+##
+
+# This code extracts the examples from manuals chapter-wise and
+# stores this in a workspace.


Where does it store anything in a workspace?

fingolfin · 2017-01-18T19:09:59Z

tst/testmanuals.g

+Print("Extracting manual examples...\n");
+Read(Filename(DirectoriesLibrary("doc/ref"), "makedocreldata.g"));
+
+GAPInfo.ManualDataRef.pathtodoc := DirectoriesLibrary("doc/ref");


Minor nitpick: DirectoriesLibrary("doc/ref"); occurs twice, perhaps reorder and use GAPInfo.ManualDataRef.pathtodoc in the Read ? Not that it matters much...

Of course keeping the Read as it is can be convenient when debugging this file, as it allows one to copy&paste that line into a GAP session.

So an alternative would be to get rid of GAPInfo.ManualDataRef.pathtodoc (or is it used outside of this script). There is only one place here using it...

fingolfin · 2017-01-18T19:12:02Z

tst/testmanuals.g

+        PrintTo(output, "####  Reference manual, Chapter ",i,"  ####\n",
+                "gap> START_TEST(\"", chname, "\");\n");
+        for a in ch do
+            AppendTo(output, "\n#LOC# ", a[2], a[1]);


Another minor nitpick: You switch between PrintTo and AppendTo. I think they do the same for streams anyway, but using both might confuse somebody who doesn't know that...

fingolfin · 2017-01-18T19:15:22Z

tst/testmanuals.g

+Read(Filename(DirectoriesLibrary("doc/ref"), "makedocreldata.g"));
+
+GAPInfo.ManualDataRef.pathtodoc := DirectoriesLibrary("doc/ref");
+GAPInfo.ManualDataRef.pathtoroot := DirectoriesLibrary("");


This variable is not used at all, why set it at all?

markuspf · 2017-01-19T10:05:17Z

Mostly these things are leftovers from banging my head against how to build manuals and examples.

I am also not quite sure whether there is currently consensus on #1076 at all...

fingolfin · 2017-01-19T11:48:11Z

@markuspf I see nothing controversial on #1076, what are you refering to?

fingolfin · 2017-01-19T12:02:21Z

tst/testmanuals.g

@@ -33,6 +30,8 @@ WriteExamplesTst := function(directory)
        output := OutputTextFile( Concatenation(directory, "/", chname), false );
        SetPrintFormattingStatus( output, false );

+        # PrintTo will *overwrite* the file if it exists, making sure
+        # we don't concatenate the same test into a file multiple times


This comment is incorrect. It is it the false in the call to OutputTextFile which takes care of that. Other than that, PRINT_TO_STREAM and APPEND_TO_STREAM are doing exactly the same thing (see PR #1093). Note that the GAP manual even states that PrintTo and AppendTo for streams both append -- of course, if you think about it, they have no other choice, since that's how streams work -- there is no way to "delete" the content of a stream.

Orgh. Sorry, shows how much attention I paid reading the documentation (which I did), and remembering my code (which I obviously didn't).

fingolfin · 2017-01-19T13:07:31Z

.travis.yml

@@ -10,7 +10,7 @@ matrix:
      after_success:
          - bash <(curl -s https://codecov.io/bash)
    - os: linux
-      env: TEST_SUITE=testtravis CFLAGS="-fprofile-arcs -ftest-coverage" LDFLAGS="-fprofile-arcs"
+      env: TEST_SUITE=testmanuals CFLAGS="-fprofile-arcs -ftest-coverage" LDFLAGS="-fprofile-arcs"


Shouldn't this be an additional travis run? I.e. not replace the testtravis run on Linux.

fingolfin · 2017-01-22T00:21:01Z

One of the test failures is fixed in master, another will be fixed by PR #1100. Might want to rebase once the latter is merged.

And of course @alex-konovalov mentioned an upcoming alternate implementation, looking forward to that, too :-).

fingolfin · 2017-01-25T10:23:23Z

@markuspf BTW, how about replacing that tst/testmanuals/KEEP by a call to CreateDir (which is idempotent) in tst/testmanuals.g ?

fingolfin · 2017-01-25T10:25:33Z

Also, the last test run for this died with this error on Travis:

Extracting manual examples...
Error, Record: '<rec>.ManualDataRef' must have an assigned value
not in any function at tst/testmanuals.g:14
you can 'return;' after assigning a value
brk>

fingolfin · 2017-01-25T16:19:56Z

etc/ci.sh

-    else
+case $TEST_SUITE in
+    makemanuals)
+        if [[ $TRAVIS_OS_NAME = 'linux' ]]


Any particular reason for this check? We only run makemanuals on linux anyway, right?

fingolfin · 2017-01-25T16:21:07Z

etc/ci.sh

+        then
+            make manuals
+            cat doc/*/make_manuals.out
+            if [ `cat doc/*/make_manuals.out | grep -c "manual.lab written"` != '3' ]


Why the use of cat, and not just grep rep -c "manual.lab written doc/*/make_manuals.out ?

(Oh wait, that's old code, OK.)

I don't know, but this must've been there before, copied and pasted through the centuries. I can change it though while I am at it.

fingolfin · 2017-01-25T16:25:36Z

etc/ci.sh

+        for ch in tst/testmanuals/*.tst
+        do
+            COVNAME="coverage.`basename $ch .tst`"
+            sh bin/gap.sh -q --cover $COVNAME <<GAPInput


On my system with a new SSD, it takes 1.5 seconds to start GAP. Even if it's fully cached in RAM. I do not know how long it takes on Travis, but I wouldn't be surprised if it was slower there. We do this 80 times or so, hence just starting GAP that often costs us 2 minutes or more.

So I would really consider using a workspace here, like we do in the make targets.

Even better: You could move this call after the esac, and loop over all coverage* files in GAP. I.e. it would be the same code for the testmanuals code as for all the other testFOO targets.

fingolfin · 2017-01-25T16:27:17Z

etc/ci.sh

+        done
+        cd bin/x86* ; gcov -o . ../../src/*
+        cd ../..
+        ;;


I would suggest moving these two commands after the esac. It's always the same -- and it doesn't hurt if we run it for the makemanuals target, too does it?

fingolfin · 2017-01-25T16:29:44Z

etc/ci.sh

-    cd ../..
-fi;
+
+            sh bin/gap.sh -q <<GAPInput


Oh wait, we call gap twice per iteration? Then it is 4 minutes :-).

Though perhaps we could move this second invocation out of the loop, turning it into a single invocation, where GAP loops over the coverage.*.tst files? This way, we turn ~80 GAP starts into a single one.

fingolfin · 2017-01-25T16:31:17Z

etc/ci.sh

-            OutputJsonCoverage("coverage", "coverage.json");
+        for ch in tst/testmanuals/*.tst
+        do
+            COVNAME="coverage.`basename $ch .tst`"


wait, why the tst suffix? That would suggest the files are .tst files, which they are not, no?

Just to clarify this is to remove the tst suffix from the chapterX.tst filename (which are .tst files) to make a filename for a coverage file which then is not a .tst file.

You are of course right, I misparsed this, sorry

markuspf · 2017-01-25T19:56:07Z

This should be ready now. Test fails until #1106 is merged.

olexandr-konovalov · 2017-01-25T20:49:29Z

.gitignore

@@ -63,3 +63,7 @@

 /tags
 /src/TAGS
+src/TAGS


I just find it strange that all paths above start with / and paths below without /, and we have both tags and tags.

* Extracts the examples from the reference manual chapter by chapter into separate `.tst` files * Runs each of the created `.tst` files in a separate GAP process * Creates coverage reports, which are uploaded to codecov.io.

markuspf added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jan 17, 2017

markuspf self-assigned this Jan 17, 2017

markuspf requested review from fingolfin, ChrisJefferson and olexandr-konovalov January 17, 2017 10:09

markuspf force-pushed the test-manualexamples branch from c86fcba to ef42f42 Compare January 17, 2017 10:55

markuspf force-pushed the test-manualexamples branch from ef42f42 to c9c7948 Compare January 17, 2017 12:13

markuspf force-pushed the test-manualexamples branch from 239fddb to 10f9aa5 Compare January 17, 2017 14:09

fingolfin reviewed Jan 18, 2017

View reviewed changes

fingolfin mentioned this pull request Jan 19, 2017

Can we run (some) manual tests on travis? #1076

Closed

fingolfin reviewed Jan 19, 2017

View reviewed changes

markuspf force-pushed the test-manualexamples branch from fa4d7f5 to bec76dd Compare January 19, 2017 12:07

fingolfin reviewed Jan 19, 2017

View reviewed changes

markuspf force-pushed the test-manualexamples branch from bec76dd to 3e19ab0 Compare January 25, 2017 11:20

fingolfin reviewed Jan 25, 2017

View reviewed changes

markuspf force-pushed the test-manualexamples branch from 315fe8d to 4727bf2 Compare January 25, 2017 19:54

markuspf removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jan 25, 2017

markuspf added this to the GAP 4.9.0 milestone Jan 25, 2017

markuspf force-pushed the test-manualexamples branch from 4727bf2 to 526885e Compare January 25, 2017 20:02

olexandr-konovalov approved these changes Jan 25, 2017

View reviewed changes

markuspf force-pushed the test-manualexamples branch from 526885e to 6177b24 Compare January 25, 2017 21:03

Make manual examples a testsuite

421cf25

* Extracts the examples from the reference manual chapter by chapter into separate `.tst` files * Runs each of the created `.tst` files in a separate GAP process * Creates coverage reports, which are uploaded to codecov.io.

markuspf force-pushed the test-manualexamples branch from 6177b24 to 421cf25 Compare January 26, 2017 21:16

fingolfin merged commit f51a365 into gap-system:master Feb 6, 2017

markuspf deleted the test-manualexamples branch February 6, 2017 12:41

olexandr-konovalov mentioned this pull request Feb 11, 2017

New test of manual examples fails too fast #1138

Closed

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 20, 2018

Test manualexamples #1082

Test manualexamples #1082

Conversation

markuspf commented Jan 17, 2017

markuspf commented Jan 17, 2017

olexandr-konovalov commented Jan 17, 2017

olexandr-konovalov commented Jan 17, 2017

codecov-io commented Jan 17, 2017 • edited Loading

Current coverage is 56.88% (diff: 100%)

markuspf commented Jan 17, 2017

markuspf commented Jan 17, 2017

markuspf commented Jan 17, 2017

fingolfin commented Jan 17, 2017

fingolfin commented Jan 17, 2017

markuspf commented Jan 17, 2017

ChrisJefferson commented Jan 17, 2017

fingolfin commented Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Jan 19, 2017

fingolfin commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 22, 2017

fingolfin commented Jan 25, 2017

fingolfin commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Jan 25, 2017

Choose a reason for hiding this comment

codecov-io commented Jan 17, 2017 •

edited

Loading

fingolfin commented Jan 17, 2017 •

edited

Loading