-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove GAP stones #1072
Remove GAP stones #1072
Conversation
Current coverage is 56.85% (diff: 90.18%)@@ master #1072 diff @@
==========================================
Files 432 433 +1
Lines 224765 225073 +308
Methods 3431 3431
Messages 0 0
Branches 0 0
==========================================
+ Hits 122572 127955 +5383
+ Misses 102193 97118 -5075
Partials 0 0
|
327f9ed
to
e6c0fa3
Compare
OK, I now added a bunch of tweaks that go beyond removing stones, but which try to get a better working "testravis" target. Let's see how that works out. For this, I moved some of the slowest .tst files to a new directory BTW, I think the reason Travis kills our slow tests is not that they take long, but rather that there is no output for more than 10 minutes. So if we split them into multiple quicker files, or enabled the |
I am strongly in favour of splitting up test files into smaller pieces. Just an advanced warning. I tried this with bugfix.tst and found it broke other tests, as pieces were run earlier, and messed up bits of gap state. Could move bugfix.tst also to its own directory so it can be split up of course. |
Awesome, those test failures are due to a genuine bug :-) |
9239a32
to
6d3dc90
Compare
In particular, don't rely on printing polynomials, as the output of that can change when the indeterminates get a new name.
This way, the order the tests run is stable across different platforms and file systems, making it easier to predict their behavior and interdependencies, and also to compare results. Note: The old stones values also gave us a sort order, so this is a replacement for something we lost by removing them.
... since we already run those in a separate Travis job. This saves us some time which we can use to run more teststandard tests. Also, run the 32/64 bit specific tests.
* on OS X, run testinstall instead of testtravis * remove the separate testbugfix target (it is now part of testtravis anyway) * add a second 32bit build which runs testinstall (as that is no longer part of testtravis)
Rebased version, now builds on PR #1100. Also removed the "do not merge" label -- from my point of view, this is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is thorough walk through everything related to GAP4Stones. It also contains other changes, most notably it deletes some old and not used any more .g
files from tst
, and moves some text files to the new testextra directory - one should hook them to a test or create a new issue to remember. As an intermediate step, TestDirectory
accepts a list of directories too, so one could add testextra
to teststandard
to have previous behaviour.
Anticipating various surprises coming from scripts relying on old format of output (e.g. to extract runtime of some individual tests) and now having to deal with
testing: /Users/travis/build/gap-system/gap/tst/testinstall/alghom.tst
238 msec for alghom.tst
testing: /Users/travis/build/gap-system/gap/tst/testinstall/algmat.tst
3706 msec for algmat.tst
...
testing: /Users/travis/build/gap-system/gap/tst/testinstall/zmodnze.tst
226 msec for zmodnze.tst
-----------------------------------
total 128973 msec
instead of
testing: /home/gap-jenkins/workspace/GAP-master-test/GAPCOPTS/32build/GAPTARGE\
T/install/label/graupius/GAP-master-snapshot/tst/testinstall/grppcnrm.tst
grppcnrm.tst 10853 12367
-------------------------------------------
total 0 766649
but that's inevitable and should be done at some point. I should be able to deal with that after the merge.
It may be worth mentioning briefly in the changes overview (https://github.com/gap-system/gap/wiki/Changes-between-GAP-4.8-and-GAP-4.9) that we had revised tests and reorganised test directories.
gap> f := FreeGroup(2);; | ||
gap> g:=f/[f.1^2,f.2^3];; | ||
gap> g.1^5=g.1; | ||
true | ||
gap> STOP_TEST( "xgap.tst", 62690000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any special reasons to remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took several seconds (5-10?) to complete, so does not belong into testinstall
.
I contemplated moving it to a new test file in teststandard
, but ultimately decided that this is a weird random test, for whch it is unclear what exactly it is meant to exercise. Actually, the whole xgap.tst
somewhat falls into that category. From the original commit by Max N., who added it 1999-05-30: "Added tests for fp groups for problems found with XGAP. Max."
So ultimately, this file should go, its contents (or equivalent content) distributed over more appropriate files (e.g. fpgrps.tst
or files in opers
).
@alex-konovalov As to adapting scripts which parse the output of |
@alex-konovalov Also not PR #1100, which is a subset of this PR. We could get that in first (with suitable changes), as that should not break any of your scripts. |
This implements issue #1060.
However, it is not quite ready yet, because I did nothing to adapt
testtravis
, which previously run a subset of testinstall and teststandard, selecting only those .tst files whoe GAPstones were below a certain threshold.I think we should change testtravis to not run testinstall (as we run that in a separate travis task already anyway). Next, we should identify which teststandard tasks take particularly long, and then either make them faster, or else consider moving them into a yet another category, say "testslow" or "testextensive" or so.