Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GENERATE macro causes assertion failure if used after a SECTION #1938

Closed
sdutoit opened this issue May 26, 2020 · 4 comments
Closed

GENERATE macro causes assertion failure if used after a SECTION #1938

sdutoit opened this issue May 26, 2020 · 4 comments
Labels

Comments

@sdutoit
Copy link

sdutoit commented May 26, 2020

Describe the bug

Using GENERATE after a SECTION causes an assertion failure on tracker.isOpen().

Expected behavior
The following code:

TEST_CASE("repro generator bug") {
  SECTION("foo") {}
  auto m = GENERATE(1);
}

runs cleanly.

Actual behavior

An assertion failure occurs during the GENERATE code:

repro: single_include/catch2/catch.hpp:12662: virtual Catch::IGeneratorTracker& Catch::RunContext::acquireGeneratorTracker(const Catch::SourceLineInfo&): Assertion `tracker.isOpen()' failed.

Commenting out the SECTION("foo") {} line causes the test to succeed.

Reproduction steps

$ cat repro.cc
#define CATCH_CONFIG_MAIN
#include "single_include/catch2/catch.hpp"
TEST_CASE("repro generator bug") {
  SECTION("foo") {}
  auto m = GENERATE(1);
}
$ g++-9 -std=c++17 -o repro repro.cc -g && ./repro
repro: single_include/catch2/catch.hpp:12662: virtual Catch::IGeneratorTracker& Catch::RunContext::acquireGeneratorTracker(const Catch::SourceLineInfo&): Assertion `tracker.isOpen()' failed.

Platform information:

  • OS: Linux 4.14.97
  • Compiler+version: gcc 9.2.1
  • Catch version: v2.12.2

Additional context
Not sure if this is expected to work (would be nice if it did though!), but if not, it would be great if the message was friendlier!

@horenmar horenmar added the Bug label May 30, 2020
@horenmar
Copy link
Member

horenmar commented Jun 1, 2020

This is definitely not intended, but I have some misgivings about the semantics after this is fixed.

TEST_CASE() {
    SECTION("A") { ... }
    int i = GENERATE(1, 2);
    SECTION("B") { ... }
}

feels like it should run "A" once and "B" twice, but the only reasonable implementation requires "A" to be run twice as well.

@sdutoit
Copy link
Author

sdutoit commented Jun 2, 2020

Agreed that it would make sense to only run A once in that example. That's certainly what I'd expect.

@horenmar
Copy link
Member

horenmar commented Jul 4, 2020

Well, I have a prototype (and very, very, very ugly) implementation for this. After playing around with it, I am not sure how happy I am with how the tests look vs how they run.

The problem is that while the simple case makes sense, and the following runs 3 assertions

TEST_CASE() {
    SECTION("first") { SUCCEED(); }
    int _ = GENERATE(1, 2);
    SECTION("second") { SUCCEED(); }
}

nesting this further can make the result quite confusing. Even though I just implemented the feature, I had to draw a diagram to figure out why the code below reports 14 assertions.

TEST_CASE("#1938 - mixed sections and generates") {
    auto i = GENERATE(1, 2);
    SECTION("A") {
        SUCCEED("A");
    }
    auto j = GENERATE(3, 4);
    SECTION("B") {
        SUCCEED("B");
    }
    auto k = GENERATE(5, 6);
    CAPTURE(i, j, k);
    SUCCEED();
}

So, the way I see it, there are 3 possibilities here

  1. Keep the status quo, document that GENERATE cannot follow a SECTION at the same level, and strengthen the check from an assert, to make sure it fires in Release mode as well as in Debug mode.
  2. Make GENERATE a virtual section. This would provide the desired behaviour for the first example, at the cost of less obvious behaviour in more complex examples.
  3. Make GENERATE only interact with their parent SECTION/TEST_CASE to have it run multiple times. This would make it so that the first example reports 4 assertions (each SECTION would be run twice), and the second example would report 32 asserts (8 elements in generators * 2 sections * 2 asserts in each run).

I will have to think about which one I prefer.

@sdutoit
Copy link
Author

sdutoit commented Jul 11, 2020

What you have there (I think described as case 2) seems totally expected to me. I wasn't surprised to see 14 assertions:

2 cases for the first section
+ 2 * 2 = 4 cases for the second section
+ 2 * 2 * 2 = 8 cases for the third section
leading to 14 cases in total.

Option (3) seems like it would just lead to extra runtime without any benefit, since those later generators cannot possibly be seen by earlier code.

Option (1) seems like a shame, as disabling this really limits the use of GENERATE.

I assume that if someone wrote

TEST_CASE("") {
  SECTION("A") {
    auto i = GENERATE(1, 2);
    SUCCEED("A");
  }
  SECTION("B") {
    SUCCEED("B");
  }
}

this would still only lead to 3 assertions? 2 in section A and one in section B?

horenmar added a commit that referenced this issue Jul 26, 2020
This means that code such as

```cpp
TEST_CASE() {
    SECTION("first") { SUCCEED(); }
    auto _ = GENERATE(1, 2);
    SECTION("second") { SUCCEED(); }
}
```

will run and report 3 assertions, 1 from section "first" and 2
from section "second". This also applies for greater and potentially
more confusing nesting, but fundamentally it is up to the user to
avoid overly complex and confusing nestings, just as with `SECTION`s.

The old behaviour of `GENERATE` as first thing in a `TEST_CASE`,
`GENERATE` not followed by a `SECTION`, etc etc should be unchanged.

Closes #1938
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants