Support automatic check for unexpected exception

[vid512]

In my tests with Catch2, I would expect everything in test case to be checked for exceptions. If anything outside of REQUIRE_THROWS / CHECK_THROWS throws an exception, I consider it a failed test, and I want to get a message about which section / line caused the exception.

It seems Snitch doesn't support this scenario currently. For example

#define SNITCH_IMPLEMENTATION
#include "snitch_all.hpp"

#include <exception>

void command1() {}
void command2() {}
void command3() {throw std::runtime_error("error");}

TEST_CASE("commands") {
    SECTION("command 1") {
        command1();
    }
    SECTION("command 2") {
        command2();
    }
    SECTION("command 3") {
        command3();
    }
}

outputs:

starting a with snitch v1.1.1.e557246
==========================================
failed: running test case "commands"
          at <snitch internal>:0
          unhandled std::exception caught; message: whoops
==========================================
error: some tests failed (1 out of 1 test cases, 0 assertions, 9.610000e-05 seconds)

Only if I wrap command3() in REQUIRE_NOTHROW, will I get a nice error message. But wrapping every command in every test in macro is unfeasible.

Am I missing something, or is this a necessary tradeoff for smaller footprint?

[vid512]

Maybe generating the try block at SECTION level, and reporting section which has thrown, would be a big improvement. Possibly opt-in.

[cschreib]

Hi and thanks for reporting this. I have not measured the impact of systematically inserting try / catch blocks around every tested statement, it could be that the overhead is minimal. If so, it would be a good idea to add them. I will see if I can investigate this when I have a moment. But if you or anyone else is willing to try and benchmark the difference, by all means!

[cschreib]

The branch nothrow_test contains the changes required to implement this feature -- still need to check the performance and test it.

[cschreib]

Here is the performance impact (using the benchmark setup from the README):

Debug	before	after
Build framework	3.8s	3.8s
Build tests	68s	87s
Build all	71s	92s
Run tests	38ms	65ms
Library size	7.7MB	7.7MB
Executable size	35.3MB	40.4MB

Release	before	after
Build framework	4.9s	4.9s
Build tests	144s	201s
Build all	149s	206s
Run tests	29ms	26ms
Library size	1.3MB	1.3MB
Executable size	9.9MB	13.2MB

Ouch:

• Binary size: +14% in debug, +33% in release.
• Compilation time of the base library: unchanged.
• Compilation time of the test code: +28% in debug, +39% in release.
• Run time of the test code: +71% in debug, -11% in release (this is a noisy measurement; the release time is probably identical, but the increase in debug is probably real)

Interestingly, with this turned on, our benchmark numbers become very close to that of doctest; perhaps this is the main reason why snitch is faster, if doctest includes this by default and we don't (edit: doctest does enable this by default, but this feature isn't the main reason for it being slower -- see below where snitch numbers improve after some optimisation).

[cschreib]

Based on the above, I would say this is either a no-go, or it has to be configurable.

We could also consider your proposed alternative of only implementing this in SECTION(). The benchmark doesn't use SECTION() at all, so we won't see any impact there, and it will be a bit artificial. Still, it's fair to assume that there will be far fewer SECTION() than there should be CHECK() in any test code base.

[vid512]

I haven't tested this yet, but if I understand it correctly, this adds try blocks only to CHECK_*() macros. There's no way to portably report exact source of exception thrown inside TEST_CASE, but outside of CHECK/REQUIRE macro. For exceptions thrown outside of macros, user will still get only the test case name.

In that case, considering the speed/size impact, I think adding try blocks to SECTIONs could be overall better solution. You can at least narrow down the source of exception to section, not entire test_case, and it works for all exceptions (not just those in CHECK() macros). Could still be opt-in, just to be sure, since there's no good way to measure its impact.

[cschreib]

I haven't tested this yet, but if I understand it correctly, this adds try blocks only to CHECK_*() macros. There's no way to portably report exact source of exception thrown inside TEST_CASE, but outside of CHECK/REQUIRE macro. For exceptions thrown outside of macros, user will still get only the test case name.

That is correct.

In that case, considering the speed/size impact, I think adding try blocks to SECTIONs could be overall better solution. You can at least narrow down the source of exception to section, not entire test_case, and it works for all exceptions (not just those in CHECK() macros). Could still be opt-in, just to be sure, since there's no good way to measure its impact.

I think that's a decent compromise. I can use the snitch self tests as a benchmark; these do use sections. It won't be as good a benchmark as the one above (where the tested code is very simple and cheap to compile, so we know the compilation time is mostly driven by the testing framework). But it's better than nothing.

[cschreib]

One thing I still need to check: the test branch I created uses a macro to add the catch (...) { ... } block wherever it is needed, but the body of the catch is repeated verbatim (inline) everywhere. This may be a major component in the performance hit. I'd like to try putting the body of the catch inside a function instead, and see what happens.

(this would also enable customizable exception handlers later with little effort)

[cschreib]

Debug	before	after
Build framework	3.8s	3.9s
Build tests	68s	71s
Build all	71s	75s
Run tests	38ms	50ms
Library size	7.7MB	7.6MB
Executable size	35.3MB	36.1MB

Release	before	after
Build framework	4.9s	5.0s
Build tests	144s	155s
Build all	149s	160s
Run tests	29ms	26ms
Library size	1.3MB	1.3MB
Executable size	9.9MB	10.5MB

It is indeed substantially better:

• Binary size: +2% in debug, +6% in release.
• Compilation time of the base library: unchanged.
• Compilation time of the test code: +4% in debug, +8% in release.
• Run time of the test code: same as before.

Definitely worth considering as this level.

[cschreib]

Also note: adding try/catch in SECTION isn't feasible, because we'd need it to be of the form SECTION_BEGIN (for try) and SECTION_END (for catch).

But actually it isn't necessary; when an unexpected exception is thrown, we know what SECTION we're in (the test registry keeps track of it for us), and in fact the name of the current section is already displayed in the error message today:

int some_function() {
    throw std::runtime_error("nope");
}

TEST_CASE("test") {
    SECTION("section 1") {
        CHECK(some_function() == 2);
    }
}

Result:

failed: running test case "test"
          in section "section 1"
          at /home/tests/test.cpp:5
          unexpected std::exception caught; message: nope

The only issue, I guess, is that the line number points to the test case declaration, and not the section. That would be fairly easy to change, if desirable. But anyway, there's no need to add try/catch blocks there; we have all the information we need already.

(Edit: my bad; I was on the test branch :facepalm: On main we don't get the section information, because stack unwinding happens and clears out the section state (as it is intended to do). So we'd need to do something more to get it working -- not much though. Nonetheless, the following still holds.)

This makes me think; perhaps we can get even smaller overhead on catching exceptions inside CHECK(...), if instead of adding try/catch blocks, we simply record the current file/line and let the exception propagate up... I'll try that.

[cschreib]

After a bit of refactoring, I managed to get this to work with almost zero net overhead (it did add a bit of overhead, but the refactoring was enough to gain back most of it by simplifying the assertion reporting functions).

Debug	before	after
Build framework	3.8s	3.8s
Build tests	68s	68s
Build all	71s	72s
Run tests	38ms	42ms
Library size	7.7MB	7.6MB
Executable size	35.3MB	35.7MB

Release	before	after
Build framework	4.9s	5.0s
Build tests	144s	145s
Build all	149s	150s
Run tests	29ms	26ms
Library size	1.3MB	1.3MB
Executable size	9.9MB	10.0MB

So I think this can be enabled all the time. PR to follow: https://github.com/snitch-org/snitch/pull/151

[vid512]

Great work, thanks!

There's one more interface thing that I think could be improved. From the message you can't immediately tell difference between exception thrown from inside CHECK/REQUIRE macro, and exception thrown anywhere inside section outside of macro. The latter case reports as if the exception was thrown at line number where SECTION starts. User might be wondering why the SECTION() macro itself throws exception, and suspect implementation error.

Technically it's correct, but maybe you could prevent some confusion by storing one more bit of information (whether the line number is SECTION line number, or precise CHECK/REQUIRE section number) and displaying more explicit message if exception could have been thrown from anywhere inside the section starting at line XY. Different message from reporting when snitch knows the exact line number.

[cschreib]

Would it be okay if the message was simply edited to say "exception throw somewhere inside this code", to make it clear that the location may be approximate? Then we don't need separate messages. It would be clear anyway when the line points to a CHECK that the "approximation" is actually very good.

[vid512]

IMO it sounds better than current state. But, IMO, separate messages are the main point - to clearly distinct between cases when we know the reported location is known to be more/less exact, and when we know the location is just "somewhere inside code block starting at reported location". I would consider separate messages an advantage.

I don't have definitive opinion on this though, would be interested in other people's thoughts. If you don't feel convinced if tgus us a good idea, feel free to ignore it.

[cschreib]

Ok, I'll have a think about how we can best express this in the event API.

[cschreib]

I pushed an attempt at this. In summary:

• If the exception happens inside a CHECK(...), the message will say "somewhere in check at file:line"
• If the exception happens outside a CHECK(...) but inside a SECTION(...), the message will say "somewhere in section at file:line"
• Else, the message will say "somewhere in test case at file:line"

How does that sound?

[vid512]

Sounds good, thanks a lot.

[cschreib]

Completed in https://github.com/snitch-org/snitch/pull/151. Thanks for the suggestion!