SECTION execution regression

[stefanholek]

Given this test:

TEST_CASE("catch/siblingSections", "")
{
    printf("0");
    SECTION("s1", "") {
        printf("1");
    }
    SECTION("s2", "") {
        printf("2");
    }
    SECTION("s3", "") {
        printf("3");
    }
    printf("\n");
}

Pre-1.0 the output used to be:

01
02
03

With current tip the test prints instead:

0
01
02
03

More curiously, given this test:

TEST_CASE("catch/nestedSections", "")
{
    printf("0");
    SECTION("s1", "") {
        printf("1");
        SECTION("s2", "") {
            printf("2");
            SECTION("s3", "") {
                printf("3");
            }
        }
    }
    printf("\n");
}

Pre-1.0 the output used to be:

0123

While with current tip the test prints:

0
01
012
0123

[wichert]

The first line is a bit unexpected, but the rest is by design as far as I can see: CATCH will treat all code at higher levels as initialisation code and re-run it for every SECTION at the same or lower level. I use this to re(set) fixtures in between sections.

[stefanholek]

Hi Wichert! Yes, what you describe is the pre-1.0 behavior. Currently however, each TEST_CASE and SECTION is first run without any sub-SECTIONs, and I can't see how this would be useful.

The problem is more obvious when using the BDD macros:

SCENARIO("catch/nestedSectionsBDD")
{
    printf("0");
    GIVEN("s1") {
        printf("1");
        WHEN("s2") {
            printf("2");
            THEN("s3") {
                printf("3");
            }
        }
    }
    printf("\n");
}

There is no good reason this should print anything but:

0123

Or is there?

[wichert]

There isn't any that I can see. For what it's worth I would prefer a more explicit style, but I think Phil disagrees with that. Something like (naming shamefully stolen from Jasmine:

TEST_CASE("my-case") {
   MyFixture fixture;

   BEFORE_EACH() {
       ....
   }

   AFTER_EACH() {
       fixture.reset();
   }

   SECTION(...) {
       ....
    }
}

This makes it explicit which code you want to run before/after every test (the after option is missing right now), and allows you to do one-time setup code that doesn't need to be rerun for every test.

[philsquared]

@stefanholek I rewrote the section tracking code. The old code was crufty and a bit too convoluted for something that's pretty central to how Catch works - and it had at least two known serious bugs.

In doing so I changed the order that sections are scanned and run as this was a big part of the complexity. Previously I was aiming to run everything the minimum number of times - which is why your second example (nested sections) ran through in a single execution.
However, as well as adding extra tracking complexity that also led to some inconsistencies as sometimes it would still run have to outer sections and/ or the test case more than once.

Now it runs each level through entirely, noting nested sections as it goes - then goes back and runs the nested sections in the same way. This is much more uniform, runs consistently (even with errors) and is easier to reason about. Most of the time you won't notice the difference. Obviously instrumenting the tests with printfs or such shows it up.

Does this give you any issues or was it purely curiosity?

One of my future plans is to suppress output from any assertions that have already successfully passed on the way to running new sections - so this should minimise unnecessary output - but the code will still be executed.

@wichert I prefer the more natural scoping that nested sections provides, yes. The only thing about it that is different to what you might immediately expect is the way only a single leaf section is executed at a time.
As for "the after option is missing right now" there are two ways you can achieve this, if you need it:

1. Use some sort of RAII class that manage any resources that need cleaning up. You can even use a really generic one like ScopeGuard to run arbitrary code on clean-up.
2. As a last resort (I've only ever had to do this once - under very specialised conditions) you can wrap your code (including nested sections) in a try{ /* ... */ } catch( ... ) { /* clean-up */; throw; } (that's about as close as you can get to try-finally in C++ - but you may have to repeat the clean-up within the try).

Since (1) is how I generally approach all my C++ code anyway this has not really been a problem for me. But I realise that there are different coding styles. Have you really found this an issue in practice?

Finally - don't forget you can still do "tradtional" style fixture classes - with setup constructors and teardown destructors. Just use TEST_CASE_METHOD.

[stefanholek]

Does this give you any issues or was it purely curiosity?

No issues. I just noticed my assertion count going up after upgrading Catch and was wondering. It's bound to increase test runtime though, especially for the BDD case, isn't it?

[philsquared]

Technically it will increase test time when you're using sections, yes. In practice this should not normally be noticeable - at least for unit tests. If you're using Catch for integration tests and your setup is expensive I can see this might be a bit more annoying - good point.

I might take another look and see if I can start from what I have now and change it to depth first, again. If I can do so without re-introducing too much extra complexity I'll consider that. Thanks for the prod.

[bkuhns]

I believe I just spent an hour or so discovering this bug. I pulled build 6 yesterday and most of my tests work fine. I then was running some newer tests today that already used the new BDD macros. Suddenly tests that I thought should work were failing. I didn't immediately think to connect the failures to Catch's build 6 and spent some time scratching my head instead.

Here's what one of my tests looks like https://gist.github.com/bkuhns/135c079e0248a752c948

With build 5, this test passes normally. Under build 6, the mocking framework reports that the expected function (DataConverter::convert) isn't called. It seems the mocks get destructed differently in Catch build 6 such that methods on them aren't invoked first, so the mocking framework throws an exception.

Update: It seems in build 6, the GIVEN section where I set up the mocks and their expectations doesn't delve into the WHEN section during the same execution step, so the mock(s) get destructed without the expected functions called.

[bkuhns]

I just pulled build 23 and this behavior still exists. Here's a "real world" example of using regular sections rather than the BDD macros. I create a mock object in my set up, each section then sets different expectations on the mock, the actual test runs after the sections, and finally we set up the requirement on the result:

TEST_CASE("Connector should or shouldn't send a message depending on whether it's connected.")
{
    auto implMock = new ConnectorImplMock();
    auto connector = Connector(unique_ptr<ConnectorImpl>(implMock));

    SECTION("Connector is connected.")
    {
        MOCK_EXPECT(implMock->isConnected).once().returns(true);
        MOCK_EXPECT(implMock->send).once();
    }

    SECTION("Connector is NOT connected.")
    {
        MOCK_EXPECT(implMock->isConnected).once().returns(false);
        MOCK_EXPECT(implMock->send).never();
    }

    auto message = getDummyMessage();
    connector.send(message);

    REQUIRE_NOTHROW(mock::verify());
}

Somewhere around build 4 or 5, CATCH would go immediately into the first SECTION on the first run. With build 23, it skips both sections and executes the test. But because the mock's expectations are set within the sections, the first run of the test (which skips sections) executes without setting up the mock. With no expectations set, the mock framework throws an exception on the first run of the test.

This makes sections in a unit test with mock objects unusable.

[philsquared]

Prompted by #552 I had a look through this issue again.
I can't remember exactly when it went in now, but I did manage to change the behaviour back to the pre-1.0 ordering (and more recently tightened it up even more with another section-tracking overhaul).
For anyone still watching this issue who had problems before could you confirm (or otherwise) whether recent versions still have any issues for you?

[ruudsieb]

Just started using catch 1.5.1 today for integration tests, and immediately stumbled upon this issue.
When comparing my tests to the OP, the output is;
01
02
03
When all sections PASS
When section3 fails, the output is
01
02
03
0
(newline is not very visible, but present ;)
So the whole testcase is executed again after the failing section, which is not needed and takes a lot of extra time.

[philsquared]

Hi @ruderik1,
Unfortunately this is unavoidable.
Catch needs to execute the code in order to discover the sections. When everything passes it comes out of section 3, notices there are no more sections to discover, and so completes.
But in the failing case it doesn't know if there were still more sections to discover so it needs to do one more run through to be sure. Possibly in this particular case it could be made to work (if it knows there are no more sections that it has skipped on previous run-throughs) but that doesn't generalise. For example: consider what happens if section 1 throws. It has never even seen section 2, so the only way it can discover it is to run through again, skipping section 1.

[ruudsieb]

Hi Phil,
Thanks for the explanation. I didn't realize that the sections are dynamically discovered, is there a reason for that ('cause they are statically defined, i would assume dynamic discovery is not needed)?
I like the concept of catch, and specially the ease of use. I'm going to try if i can use setup/teardown instead of code in the testcase which then hopefully doesn't get executed when there is no section to execute anymore.
Thanks,
Ruud

[philsquared]

Since nobody else (other than @ruudsieb) chimed in on this thread in the intervening couple of years I'm going to assume that there are no outstanding issues?
I'm marking this for closure unless anyone objects in the next few days.

[bkuhns]

Looks good to me, I just tried out the following test:

TEST_CASE("A SECTION should be run on the first pass.")
{
	auto sectionRun = false;
	SECTION("If this section runs, then the test should always succeed.") {
		sectionRun = true;
	}
	REQUIRE(sectionRun);
}

And the test passes, implying it runs through the section on the first run.

[philsquared]

Cool - thanks @bkuhns