Criterion hangs - cannot define global max timeout (2.3.3 & 2.3.2)

[Keenuts]

Hi,

~~So I was using Criterion 2.3.2-1 (from the AUR) and had some issues with clang++ and cr_assert_throw. Seeing on of the issues solved in the 2.3.3, I recompiled Criterion to use it. Now, I have previously successful tests hanging. The timeout option does not seems to change anything.~~ Maybe related to #82

./binary --help says:

--timeout [TIMEOUT]: set a timeout (in seconds) for all tests

However it's only set if the test defines a timeout != than 0. Might be handy to be able to set a maximum timeout for every tests.

All the best,

Nathan

[Keenuts]

Mmm, core/runner_coroutine.c:337 -> if timeout is set to 0 by the test, the timeout option is unused. And default timeout might be 0 (not checked yet).

[Snaipe]

Do you have a self-contained example that exposes your hanging problems?

This aside, letting the user define a max timeout value could be useful, but I'm somewhat unconvinced that this is something that criterion should provide, given that timeout(1) is a coreutils command that does the exact same thing.

[Keenuts]

A simple one could be:

Test(Core, feature_a)
{
    for(;;)
        continue;
}

Yes but do I want to kill the whole test-suit for 1 failing test ? Plus, I would need a huge timeout, because it would apply to the whole test-suit, imagine using timeout to control piglit, it would be insane 🤠 By global max-timeout, I wanted to say a timeout which would apply to all tests, overriding the default behaviour of -1 if no timeout has been set when declaring the test. Not a 'global' timeout on the test-suit itself 🙂

[Snaipe]

Oh, sorry, I thought you were having hanging issues for some of your tests when you upgraded criterion, and wanted to know if you had any reproducer.

I definitely interpreted "global timeout" as a timeout spanning the total runner, but then I'm somewhat unsure if overriding timeouts is a good thing.

If you explicitly define a timeout for a test, that means that you know the upper run time boundary for this test; if --timeout overrides that, then a shorter value makes no sense, because now some runs might time out, and a larger value adds no benefit either. Currently, --timeout sets a timeout if it is 0, and the default timeout is 0, so it should be defining the timeout for all tests where setting one actually matter.

I do agree that in this case the naming of --timeout is a bit unfortunate. It should have been --default-timeout instead.

[Snaipe]

Moving this to the v3.0.0 milestone to rename --timeout to --default-timeout, since we can't rename flags without breaking backward-compatibility.