Don't change assertion level in START_TEST and STOP_TEST.

[stevelinton]

As discussed in #527 simply don't set assertion levels in the standard START_TEST and STOP_TEST.
Anyone who really wants to do this can call SetAssertionLevel before starting tests, or patch START_TEST and STOP_TEST when the need to.

[stevelinton]

Incidentally, running testinstall with assertion level raised to 3 or higher throws up some recursion depth traps.

[ChrisJefferson]

This looks good. I thought about adding an option to Test and TestDirectory to set the assertion level, but actually it feels simpler to just tell people to call SetAssertionLevel before running Test.

[olexandr-konovalov]

This should go into stable-4.8 (and I am looking forward to see if some timeouts in package tests will gone or not). No need to submit another PR - I will cherry-pick as soon as it will be merged.

[stevelinton]

Why would we put this in stable? It's a functionality change. In particular package tests might be assertion-level dependent.

[olexandr-konovalov]

@stevelinton: see https://github.com/gap-system/gap/blob/master/Makefile.in - for testpackages/manuals/updates assertion level 2 will be used anyway (what probably dismisses my earlier expectation that this will affect packages).

[stevelinton]

@alex-konovalov I meant tests that package authors themselves run with TestDirectory and so on.

It's not critical, I just see no reason to rush this out.

[ChrisJefferson]

I just checked, and this patch breaks testing in the profiling package (just changes some text). I'll fix it, but there is an concrete example of why @stevelinton suggests we shouldn't put this patch in stable.

[olexandr-konovalov]

@ChrisJefferson @stevelinton thanks, I see - since Assert statements will trigger some extra calculations, output of commands that use randomness may be altered, and that's likely what @ChrisJefferson observes. No need to rush then.

However, we run testpackages with assertion level 2 anyway. Does this mean that if this PR will be merged, then package tests will have to be adapted for GAP 4.9 to run with assertion level 0, and we will run them with assertion level 0 as well?

[olexandr-konovalov]

I suggest to label this "Do not merge" and merge this closer to the time when we will be preparing beta release for GAP 4.9. For a package with test impacted by this change, its a bit too early to update them for the next major release, since the may still need to update them for GAP 4.8.

[fingolfin]

This is something we should look into soon, at the latest during the upcoming GAP Days. But perhaps earlier. At the very least, we should get a full list of package test suites affected by this, so that we can work with their maintainers to resolve that. Ways to resolve include making the tests behave the same on both assertion levels 0 and 2; or to add explicit code which sets explicit assertion levels to the test suites of affected packages.

Somewhat orthogonal to that, we could also think about providing a clean way for package authors to hook into START_TEST and STOP_TEST, to perform their own custom setup...

[fingolfin]

Thinking about this some more, I think we might open ourselves up for a lot of pain by changing the behavior of this function after so many years. It is OK to do this if really no package is affected by this change.

But otherwise, we might be better off leaving this in, and instead adding a way to adjust the assertion level -- e.g. by adding another, optional, argument to START_TEST which can be used to override the assertion level, and then we can do that for all tests in GAP.

Anyway, perhaps I am overly pessimistic -- but until somebody provides some actual data on how this affects existing packages, I'd be very reluctant to merge this. Esp. since it is also unclear to me what exactly this PR gains us.

[olexandr-konovalov]

I think merging this PR would allow us to make tests more flexible - instead of hardcoding assertion level 2 in the test, we may specify it elsewhere (e.g. as an option passed in TestDirectory) and run tests at different assertion levels - e.g. in zero for timing or in level 3 for more thorough checks (this PR would require then some more work first).

[fingolfin]

What's the plan with this PR? We either should proceed with it, or close it.

I am relatively neutral on changing the assertion level to 2 or not at the start of the tests, BUT one thing I'd like to request is that we keep preserving the assertion level: I.e. we should keep setting GAPInfo.TestData.AssertionLevel in START_TEST, and use it to reset the global assertion level in STOP_TEST. This way, if a test messes with the assertion level, it won't affect other tests. Without this, if I wanted to run the test suite on assertion level 3, then any single .tst file which changes the assertion level would also change it for all subsequent test files.

Of course we might want to do this instead inside of Test, not relying on START_TEST/STOP_TEST, which might be overridden; indeed, I just realized TestDirectory overrides it, also breaking this :-/

[olexandr-konovalov]

I'd prefer to take this out of START/STOP_TEST, but only in conjunction with changing all other places to ensure that we keep running tests with the same assertion level as before. May try an alternative to this PR ... I think that's why I've assigned it to myself ...