Remove locale specific tests

[fingolfin]

These caused issues in various places, but test nothing useful.

Resolves #2194
Resolves #3979

[dimpase]

I am not 100% sure it is wise to remove these tests - I would have kept them, but only tested that running them doesn't lead to a crash.

…

On Mon, 11 May 2020, 00:25 Max Horn, ***@***.***> wrote: @fingolfin <https://github.com/fingolfin> requested your review on: #4022 <#4022> Remove locale specific tests. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#4022 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHHLKJPGCYJEC5AGIZDRQ4ZX3ANCNFSM4M5OJEWQ> .

[fingolfin]

@dimpase and why is that? What do you believe is tested by the removed parts of the tests which is not tested by the remaining tests?

[coveralls]

Coverage remained the same at 84.372% when pulling 0a39897 on fingolfin:mh/locale into 4c03509 on gap-system:master.

[dimpase]

I meant to say that other tests don't test printing "unprintable"/locale-dependent characters. I do not know the underlying code, but it is easy to imagine an implementation that does not do a correct thing.

Testing platforms such as Python's doctests allow to test for wildcards in the output (this is ... in doctests), perhaps GAP should do this too.

[fingolfin]

@dimpase GAP has no special code or any kind of special behavior for "unprintable"/locale-dependent characters. Thus, it is unclear what you think this "tests" that is not tested by printing regular printable letters like "a". It is "easy to imagine" a lot of things, but I am interested in cold hard facts, not fantasy :-).

I did at first think about coming up with ways to allow us to leave in those \0xff chars, but then I realized that "testing" for them really serves no purpose I can think of...

[dimpase]

I'm saying that by not testing for invalid input you decrease the coverage of the testsuite (@coveralls agrees withg me :-)). One reason for testing for invalid input is to test the environment GAP runs in, which might have quirks beyond anyone's imagination, leading to segfaults.

It's much easier to deal with the sutuation where a user reports a test failure, than with a user report saying "help, I get a segfault, while all the GAP tests pass".

[fingolfin]

I'm saying that by not testing for invalid input you decrease the coverage of the testsuite

There is no "invalid input" in the tests touched by this PR.

(@coveralls agrees withg me :-)).

You do have a point there: I must admit that code coverage was so badly broken for a long time (tons of random fluctuations even in PRs which don't change any code) that I started to ignored it. But I just had a look and it is reasonable now (at least on coveralls; codecov seem to just have stopped working for us; which is a pity as I find its UX much better than that of coveralls)

Anyway, it turns out that we do have a special case for ViewObj of chars and strings equal to/ containing 0xFF (and other similar chars), just not in the kernel, but in the library; and that moreover, the one for strings is replaced by a method in GAPDoc (which of course is always loaded, so the overloaded method is never called). And GAPDoc is what brings in the locale dependency...

All the (limited) unicode handling in GAP is implemented in GAPDoc, so a core component of the system is maintained outside of it; I find this problematic. But for now, this is how it is; testing GAPDoc is out of scope for these CI tests (it would make sense to test it here, but since it is developed externally to this repository, writing tests for it here raises all kinds of concerns, so I'd rather not deal with it right this moment).

I have reinstated the tests for IsChar, though, as here our functions are called (as a GAP char is a single byte, not a UTF-8 multi-byte "character"). There is no way to test the GAP library ViewObj method for strings, though, as explained above.

One reason for testing for invalid input is to test the environment GAP runs in, which might have quirks beyond anyone's imagination, leading to segfaults.

Agreed (which is why I added tons of tests for invalid inputs over the past couple years, as did Markus and Chris; before us, these were almost never tested).

But also irrelevant for this PR, as it does not test "invalid inputs". OK, perhaps it tests "unusual" inputs. But look, testing always is a delicate balance -- there is an essentially infinite number of inputs you "should" test, but only a finite amount of time for actual tests. So we have to weigh tests by probability. Yes, that's flawed, but so is testing in general: In theory I should test printing of every integer, but i can't. So instead we limit ourselves to a certain set of numbers which we think are "interesting" in that they consist of edge cases etc. which we think are more likely to trigger broken behavior than "general" numbers. But by doing so, of course we preselected the tests, and may overlook bugs that would have been uncovered if we just had tested "every" integer -- but we can't.

Thus, just saying there "might" be inputs that lead to segfaults is not enough. You need to analyze the situation and give reasonable arguments why certain inputs ought to be included in the tests.

A reasonable argument here is that analysis of the coveralls report (the changed coverage percentage alone is meaningless) reveals that the removed test tested parsing of character in hex encoding with upper case hex letters. So I added that back, and even more (I added mixed-case hex constants -- in the current code that doesn't matter, but I can think of bugs where it might, so i am adding it proactively). But I am e.g. not adding printing of every hex constant from 00 to 255/FF (even though this would actually be feasible in this particular case). Of course we may eventually learned that printing '0xf3' is broken -- so be it, I am willing to risk that.

It's much easier to deal with the sutuation where a user reports a test failure, than with a user report saying "help, I get a segfault, while all the GAP tests pass".

True, but irrelevant for this PR. There is no segfault here, or any reason to suspect that one could be triggered. If we followed your argument, we'd be back to "let's test printing of every single integer".

[dimpase]

OK, great, works for me.

[fingolfin]

Backported to stable-4.11 in c18b0c4