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

CHECK_THROWS() with exceptions on construction? #536

Closed
rcdailey opened this issue Nov 9, 2015 · 13 comments
Closed

CHECK_THROWS() with exceptions on construction? #536

rcdailey opened this issue Nov 9, 2015 · 13 comments

Comments

@rcdailey
Copy link

rcdailey commented Nov 9, 2015

Suppose this:

TEST_CASE("Blah")
{
    CHECK_THROWS(MyClass mc("foo"))
    CHECK(mc.Stuff() == "Stuff")
}

I can't use mc because it's out of scope. I need a way to check construction of an object for throws. How do I do this with this test framework?

@peter-bloomfield
Copy link

@rcdailey That's not really possible in C++, regardless of the testing framework. If a constructor throws an exception then the object basically doesn't exist. Even if you could put CHECK() in the same scope as mc, it would never be reached because exception handling would take over.

Useful article by Herb Sutter which explains this better than I could: http://herbsutter.com/2008/07/25/constructor-exceptions-in-c-c-and-java

@rcdailey
Copy link
Author

rcdailey commented Nov 9, 2015

I understand how exceptions work, what I'm specifically wanting to check is whether or not something throws, but if it doesn't, it should continue to execute the other CHECK assertions. My example is pretty poor, but basically if you swap out the first assertion for CHECK_NOTHROW it makes more sense.

The test framework should allow me to verify NO exceptions are thrown and continue to execute assertions based on the defined variable. But there is no mechanism right now for that. We'd basically need something like this:

BEGIN_CHECK_NOTHROW();
MyClass mc("foo");
END_CHECK_NOTHROW();
CHECK(mc.Stuff() == "Stuff");

Right now I'm having to work around this by constructing two objects:

CHECK_NOTHROW(MyClass{"foo"});
MyClass mc{"foo"};
CHECK(mc.Stuff() == "Stuff");

@refi64
Copy link

refi64 commented Nov 9, 2015

@rcdailey What about:

std::unique_ptr<MyClass> mc;
CHECK_NOTHROW(mc.reset(new MyClass{"foo"}));
CHECK(mc->Stuff() == "Stuff");

@nabijaczleweli
Copy link
Contributor

@kirbyfan64 Nice, but might throw std::bad_alloc or a derivative thereof

@refi64
Copy link

refi64 commented Nov 9, 2015

@nabijaczleweli Ok then...how about something like:

char storage[sizeof(MyClass)] alignas(MyClass);
MyClass* mc = &storage;
CHECK_NOTHROW(new (mc) MyClass);
CHECK(mc->Stuff() == "Stuff");

Really, though, I doubt you'd need to worry about throwing std::bad_alloc in this case. I mean, strings and all allocate memory anyway, so it's just as likely they'll throw that error.

@rcdailey
Copy link
Author

That's horrible and unintuitive. Not a valid solution as far as I'm
concerned.

On Mon, Nov 9, 2015, 5:48 PM Ryan Gonzalez [email protected] wrote:

@nabijaczleweli https://github.com/nabijaczleweli Ok then...how about
something like:

char storage[sizeof(MyClass)] alignas(MyClass);
MyClass* mc = &storage;CHECK_NOTHROW(new (mc) MyClass);

CHECK(mc->Stuff() == "Stuff");

Really, though, I doubt you'd need to worry about throwing std::bad_alloc
in this case. I mean, strings and all allocate memory anyway, so it's just
as likely they'll throw that error.


Reply to this email directly or view it on GitHub
#536 (comment).

@refi64
Copy link

refi64 commented Nov 10, 2015

@rcdailey Not even the first one? That's the best it's going to get. This is a C++ thing, not Catch.

There are benefits to manual memory control. This is not one of them! ;)

@rcdailey
Copy link
Author

A macro based try catch is better for this as I suggested in my earlier example

@refi64
Copy link

refi64 commented Nov 10, 2015

@rcdailey Issue there is that standard C++ gives NO way of trapping an exception without opening a new scope.

@philsquared
Copy link
Collaborator

First off - do you want to check for throwing or not-throwing (you started with the former then went on to talk about the latter)? This is important as the reasoning is different in each case.

If you want to check that the constructor throws, as your first example showed, then why do you need to do anything else with the object once constructed? The test would have already failed by the point. I see you are using CHECK instead of REQUIRE so the execution would continue - but that capability is intended for catching multiple orthogonal properties (e.g. several fields on a returned object). Here you're testing an invalid object.

If you want to check that the constructor doesn't throw then you have a few options. The simplest is to not use an assertion at all! Catch will still catch the exception thrown, translate it for reporting purposes, and mark the test as failed. The only downside is that you don't get the exact file/ line reported or the failing expression captured. But it's not bad for no effort and is usually the trade-off I make.

The second option has already been proposed: just use heap allocation. If you're worried about bad_alloc, as @nabijaczleweli suggested, I believe you are over-thinking it. If you might get a bad alloc there then you might get it anywhere else.

There is a third option, which is to move the code that uses the object off into another function. Either construct the object in the function too, or construct it at the call site and pass it in by const-ref. E.g.:

struct TestClass {
    int m_seven;

    TestClass( int seven, bool shouldThrow ) : m_seven( seven ) {
        if( shouldThrow )
            throw std::domain_error( "you asked for it!" );
    }
};

void verifySeven( TestClass const& obj) {
    REQUIRE( obj.m_seven == 7 );
}

TEST_CASE( "Constructors can be exception checked" ) {

    CHECK_NOTHROW( verifySeven( TestClass( 7, false ) ) );
    CHECK_THROWS( verifySeven( TestClass( 7, true ) ) );
}

The shortcoming of this, as written, is that if something in the function throws it would also be caught by the top level assertion (either as a false positive or a false negative). This could be addressed by using CHECK_THROWS_AS to test for a specific exception type or CHECK_THROWS_WITH to test the exception message. Either way this is spiritually the same as your split-macro proposal - but without the need for any additional syntax in Catch (note that your BEGIN/END aren't quite right as they'd have to expand to include try{ and }catch - thus creating a scope - so you'd have to move the other checks inside.

@rcdailey
Copy link
Author

Again my first example was wrong, I apologize. My intention was to check that it doesn't throw. Basically I wanted to handle the case where if it throws, it stops. Otherwise, execution and further tests continue.

However (probably because it was Monday?), I had confused myself about this whole thing. I had forgotten that execution won't stop if it throws, so thank you for that.

What I've done is split my code into 2 test cases:

TEST_CASE("Throws Exception")
{
    CHECK_THROWS(TestClass{"Invalid Value"});
}

TEST_CASE("No Throwing; normal flow")
{
    TestClass tc{"Valid Value"};
    CHECK(tc.Stuff() == "Stuff");
}

This makes more sense and we track the 2 scenarios separately. I unfortunately don't get to use CHECK_NOTHROW on the 2nd test case, but like you say execution will stop if it throws.

I don't want to use the heap because IMHO, especially for test driven development, the test should also represent how objects are meant to be used. This object specifically doesn't need to be on the heap. Many people write tests before writing their class interfaces, and it doesn't make sense to "hack" the test by using the heap for objects when it isn't necessary.

I feel the solution I'm using now makes more sense from a test-case perspective, so I'll go ahead with that.

@nabijaczleweli
Copy link
Contributor

@kirbyfan64 Nice one (the aligned storage method)

@philsquared
Copy link
Collaborator

All that talk of heap allocations and now I'm going to Garbage Collect this issue ;-)
It looks like it was resolved to @rcdailey's satisfaction - and that was a year ago.
But if you feel there was any more to discuss please feel free to re-open or raise a new issue.

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

No branches or pull requests

5 participants