Empty generator results in segmentation fault

[nielsbuwen]

Describe the bug
A test case containing an empty generator (one that will never generate any elements) results in a segmentation fault when that test case is run.

runner.cpp:6: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases: 1 | 1 failed
assertions: 1 | 1 failed

Expected behavior
There should be no segfault. I would guess that the test case should not run at all if the generator is empty.

Reproduction steps
Compile this minimal example and run it.

#define CATCH_CONFIG_MAIN
#include <catch.hpp>

TEST_CASE("Empty Generator")
{
    GENERATE(take(0, value(1)));
}

Platform information:

• OS: CentOS Linux release 7.4.1708 (Core)
• Compiler+version: gcc 4.8.5 (with -std=c++11) or clang 7.0.0
• Catch version: v2.7.1

Additional context
Rational why i use empty generators: I modified to command line interface to accept a number of items i want to test:

./tests --item 42 --item 1337

These items are put into a vector which is wrapped in a generator.

TEST_CASE("Test Items")
{
    auto item = GENERATE(items());
    // test item
}   

Some test cases use these items and some don't. If i don't specify any items i get a segfault otherwise it works perfectly. As a work-around i could exclude the item tests from the rest but that is annoying.

It seems impossible to fix that issue with the current IGenerator interface as there is no way to check if the generator is empty before calling next().

I don't know the internals of Catch2, but these ideas came to my mind:

A different interface could look like this:

• bool has_next() const to check whether it is safe to call get_next
• T get_next() to return the current value and advance the generator

Or like python does it (i like python :) )

• a single T next() that return the current item and advances the generator. If the generator exhausted throw an exception instead.

Or use the C++ iterator concepts (these may be entirely wrong):

• T operator*() const to return the current item
• operator++() to create the next value
• operator!=(const IGenerator<T>&) const to compare two generators

Edit:
Another idea, that might work with the current interface: Add a bool empty() const method to the generator interface with a default implementation that simply returns an internally stored flag which starts with the value true. A call to next() can then update the flag. An empty generator can then set that flag to false in its constructor. The part that runs the tests and pulls out elements from the generator can then check the empty() method instead.

[Bu11etmagnet]

$ /usr/bin/c++ -g -I ../single_include/catch2/ -Wall -Wpedantic -Wextra  -std=c++11   empty_gen.cpp 
$ ./a.out 
Filters: 
a.out: ../single_include/catch2/catch.hpp:3711: Catch::Generators::TakeGenerator<T>::TakeGenerator(size_t, Catch::Generators::GeneratorWrapper<T>&&) [with T = int; size_t = long unsigned int]: Assertion `target != 0 && "Empty generators are not allowed"' failed.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
a.out is a Catch v2.7.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
Empty Generator
-------------------------------------------------------------------------------
empty_gen.cpp:4
...............................................................................

empty_gen.cpp:6: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases: 1 | 1 failed
assertions: 1 | 1 failed

Aborted (core dumped)

It's not a segmentation fault, it's an assertion failure, showing that you're using Catch incorrectly.

[nielsbuwen]

I should adjust my terminal color settings. I barely noticed the assertion line.

It turns out my example was too minimal. This one segfaults:

#define CATCH_CONFIG_MAIN
#include <catch.hpp>

TEST_CASE("Empty Generator")
{
    GENERATE(take(1, filter([](int i){ return i == 0; }, value(1))));
}

But it also tells me that it crashed, because the filter didn't match anything so that's fine, i guess.

Should i rather write a feature request to allow for empty generator? The problem with my custom generator still exists.

[horenmar]

Actually, a filter generator running out of elements will only fail that specific test case run, it won't abort the whole test run, so in this example

#define CATCH_CONFIG_MAIN
#include <catch.hpp>

TEST_CASE("Empty Generator")
{
    GENERATE(take(1, filter([](int i){ return i == 0; }, value(1))));
}
TEST_CASE("Bar") {
    REQUIRE(1 != 2);
}

the second test case will still run. This is not true with take, which asserts, but in the case of take passing it 0 is an obvious logic error (but I might be convinced to turn it into an exception as well).

---

As to allowing empty generators, you would need to write a hell of a motivation for that proposal to convince me, because they were disallowed intentionally.

If you have a generator that might be empty, then it should throw upon construction when it is empty so that the test case that uses it explicitly fails. The alternative is to fail it silently, but that is a trap for the user -- consider this simple example:

TEST_CASE("Boring test 1") { SUCCEED(); }
TEST_CASE("Boring test 2") { SUCCEED(); }
TEST_CASE("Generator test") {
    auto val = GENERATE(empty_generator());
    FAIL("Not yet implemented");
}

If the user compiles and runs the test binary with the current behaviour, the third TEST_CASE fails as it should, and the user knows that not all test cases actually pass. On the other hand, if you skip over empty generators, then the only signal the user gets that some of the tests have not actually run is that there are fewer tests reported in the summary.

[nielsbuwen]

Throwing inside the constructor looks good to me. I probably missed that in the reference.

However, in my case the test case should not fail when the generator is empty. What about a "special" exception we can throw that doesn't fail the test like throw GeneratorIsEmptyButThatsOk(...);. I am worried about dozens of lines of failed test cases that clutter my output making it harder to concentrate on the real problem. I could also live with a single error message like "Some tests were skipped, because of missing data" or something similar.

And sorry about the confusion about take/filter. I spend too much time to reproduce my error with the generators provided by the library.

[geoff-m]

If you have a generator that might be empty, then it should throw upon construction when it is empty so that the test case that uses it explicitly fails. The alternative is to fail it silently...

Fail? I don't follow. An empty generator does not represent a failure or an error of any kind. In general, generators output a variable and unpredictable numbers of items. Zero is such a number, just like one, two, a million, and so on. That is why whenever using any kind of generator/iterator, we write loops that check whether the generator has an item before attempting to use it, not after. By breaking with this convention and expecting to get an item out of a generator without checking whether it's available, Catch2's behavior here is a bug, and quite a surprising and annoying one.