Do not ignore #@ comments after empty line at start of test file

[zickgraf]

Description

Consider the following test file (the empty line is deliberate):

#@if true
gap> 1;
1
#@fi

When executing this testfile with ignoreComments := false, GAP complains Invalid test file: #@fi without #@if. This is fixed by not skipping lines which start with "#@".

Note regarding the Add(pos, i);: I think this should always have been there. Without it, pos[i] will NOT be the line corresponding to inp[i]. This did not cause any errors since ign is appended to pos in line 147, so pos had the correct length, and if ign is the empty list, this is not a problem. However, when using @if, ign is not the empty list anymore, so the Int call in line 213 fails because it is now applied to a non-empty list.

General note: I did not try to understand all cases in ParseTestInput, so this PR might break other corner cases I am not aware of.

Text for release notes

Do not ignore #@ comments after empty line at start of test file.

Checklist for pull request reviewers

• proper formatting

If your code contains kernel C code, run clang-format on it; the
simplest way is to use git clang-format, e.g. like this (don't
forget to commit the resulting changes):

git clang-format $(git merge-base HEAD master)

• usage of relevant labels

  1. either release notes: not needed or release notes: to be added
  2. at least one of the labels bug or enhancement or new feature
  3. for changes meant to be backported to stable-4.X add the backport-to-4.X label
  4. consider adding any of the labels build system, documentation, kernel, library, tests

• runnable tests

• lines changed in commits are sufficiently covered by the tests

• adequate pull request title

• well formulated text for release notes

• relevant documentation updates

• sensible comments in the code

[ChrisJefferson]

Thanks for this PR. It looks very reasonable. I'm going to wait until the travis-ci tests have all executed, just to check no exists tests are failing because of this.

[coveralls]

Coverage decreased (-0.0003%) to 84.328% when pulling 4598663 on zickgraf:test_framework_fix into abf18c2 on gap-system:master.

[zickgraf]

The CI has timed out multiple times, that's why I have done multiple force pushes to re-trigger it. Now it has finally passed :-)

[ChrisJefferson]

Thanks! If you notice it happening again (it seems be to having lots of bad days recently, we are working to improve it), drop a message, as some of us can selectively restart individual CI tests.

This all looks good, I'll leave it here a short while just in case anyone else has an opinion before merging.

[fingolfin]

Could we please get a test case for this, too? Possibly in tst/testspecial/

[fingolfin]

Also, I am not quite sure I understand your remark about the "missing" Add(pos, i);. Are you saying there are situations where this could have resulted in incorrect behavior? Can you give an example, possibly also in form of a test case?

[zickgraf]

Here is a test case for my remark (of course, the empty line is again deliberate):

gap> Sleep(10);

When running this, GAP currently shows # line [ ] of 2 (0%). The empty list which is displayed here is the last element of pos, which in turn is the empty list ign. With my patch, GAP correctly shows # line 2 of 2 (100%). I think I have seen this problem many times when testing with AutoDOC, since examples written and extracted by AutoDOC from _Chunks.xml always start with a linebreak. But I have never bothered to investigate.

I can add tests for all of this tomorrow.

[zickgraf]

I have now added a testcase, but Coveralls still says the coverage is decreasing. I have checked locally that the new lines are executed by my test. Do you know what could be wrong?

[fingolfin]

LGTM, thanks!!!

And don't worry about the coverage, it's all good.

[zickgraf]

Ah, I just noticed that "testspecial" coverage is not recorded/uploaded to coveralls, that's why it does not show up. Is this deliberate?

I think it would be nice to backport this PR to the stable-4.11 branch so one can use AutoDOC's maketest together with @if. If you agree, should I open a PR with target stable-4.11?

[fingolfin]

I am fine with backporting this.

Regarding coverage tracking for testspecial: well, @ChrisJefferson and me are mainly the only ones we fiddle with that stuff these days, and already spent countless hours to make things work, and some of these are super annoying to debug indirectly on Travis... that said it'd be awesome if that coverage was tracked. PRs welcome (that's not meant cheeky, if you have time to look into, that'd be great, if not, feel free to add an issue to remind about it)