Optional callback for `REQUIRE`

[fekir]

Some libraries, mostly those that expose a C api like the system API, zlib, openssl, and many others, gives the possibility to access to more detailed error information stored in global objects, or by calling a separate function with an object handle.

Consider for example following test case:

TEST_CASE("dummy){
	auto f = CreateFile(/* ... */);
	REQUIRE_FALSE(f == INVALID_HANDLE_VALUE);
}

if the test fails, no useful information are shown, which is a pity since it is the selling point of REQUIRE et al compared to other test suites.

In order to have an useful error message, we need to write the test case more similar to (pseudocode)

TEST_CASE("dummy){
	auto f = CreateFile(/* ... */);
	if(f == INVALID_HANDLE_VALUE){
		CAPTURE(hComm) ; // replicate output of "hComm != INVALID_HANDLE_VALUE"
		FAIL(GetLastError()) // or gather other information
	}
}

which works, but removes the benefit of using REQUIRE, REQUIRE_FALSE, and other facilities.

It would be great, if REQUIRE and friends could accept an optional callback to call in case an assertion fails, for example

TEST_CASE("dummy){
	auto f = CreateFile(/* ... */);
	REQUIRE(f != INVALID_HANDLE_VALUE, [](const auto& lhs, const auto& rhs){
		// lhs, rhs: optionally give in the callback the possibility to inspect the tested value, in case a function needs to access it in order to provide detailed information
		FAIL(GetLastError())
	}
}

If there a more elegant solution to this issue, that makes such a feature superfluous, then I completely missed it, otherwise I think it would be a great addition for catch to have something like that.

[philsquared]

It's an interesting problem. You can probably get what you want with:

TEST_CASE(){
	auto f = CreateFile(/* ... */);
        INFO( GetLastError() );
	REQUIRE(f != INVALID_HANDLE_VALUE);
}

The only difference with what you're asking for is that with INFO the GetLastError() expression will always be evaluated - which is probably fine in this case, but may not always be desirable.

You could also do:

TEST_CASE(){
	auto f = CreateFile(/* ... */);
	CHECKED_ELSE(f != INVALID_HANDLE_VALUE) {
                FAIL( GetLastError() );
}

Which will evaluated the failure case more lazily and is pretty much what you want (with no callbacks required!).

We should probably document CHECKED_IF and CHECKED_ELSE :-)

[fekir]

Thank you for your suggestions, I was not aware of CHECKED_ELSE.

Lazy evaluation, especially for global errors, it's important. For one the behavior is not specified (I'm thinking more of errno, unsure about GetLastError, but what about other libraries? And even if, I do not want my test to fail because maybe another test did not check it's error. It would make tests more brittle.

Still, it does not seem to be an ideal solution, maybe this example will make my point more clear (here I'm using mkdir, but feel free to replace it with any stdlib function from math, like std::sqrt):

TEST_CASE(){
	REQUIRE(mkdir("....")==0);
}

If I want to have useful information's (check errno in this case), I need to store somewhere the result of mkdir, test it with an if like in my previous sample or in CHECKED_ELSE as I've just learned, and FAIL:

TEST_CASE(){
	auto val = mkdir("....");
	CHECKED_ELSE(val != 0) {
                FAIL(errno);
	}
}

I think

TEST_CASE(){
	REQUIRE(mkdir("....") != 0, [](){
		// FAIL here was wrong, since REQUIRE would already fail... and provide us with the message "mkdir != 0", which already tells us which function failed!
		CAPTURE(errno);
	});
}

which also seems easier to simplify (in case we have many similar calls)

void capture_errno(){CAPTURE(errno);}

TEST_CASE(){
	REQUIRE(mkdir("....") != 0, capture_errno);
}

instead of

void capture_errno(){CAPTURE(errno);}

TEST_CASE(){
	auto f = mkdir("...");
	CHECKED_ELSE(f != 0) {
		capture_errno();
		REQUIRE(f !=0); // use require to have a little more descriptive error message, but it will not include "mkdir" in it's description
	}
}

Granted, the code is very similar, but still you need

• provide a custom message, or duplicate the if condition inside a REQUIRE
• store the result of the function for creating a custom message/duplicate the logic

I know it's probably a minor detail, but I think that writing

REQUIRE(foo(bar) == baz)

is much better then

auto v = foo(bar);
REQUIRE(v==baz);

since in case of failure we have a lot more information from the error message, and CHECKED_IF forces me to write the code in the second form.

It also lowers the barrier to test more functions, since I normally never see functions like mkdir, printf, sqrt and other to be tested on errors. But this might just be my experience.

[fekir]

I'm really sorry, I just noticed that I've already opened a similar issue (https://github.com/catchorg/Catch2/issues/1209), but did never get a reply, and forgot it in the meantime...

I think we can close it (or mark as duplicate), and continue the discussion here...

[philsquared]

Feel free to close #1209. Sorry for not responding before. You got lucky with me paying attention today ;-)

Regarding the inline expression check being better - I don't disagree. I was just trying to show that you could achieve what you want with what Catch already has. It may not be perfect, but there's a balance to be struck between adding lots of specialised functionality to Catch, vs making our tests more expressive.

It's also worth bearing in mind that you don't need to repeat that condition check. If the CHECKED_ELSE fails it will report its expression and values just the same as a normal CHECK/REQUIRE - that's why I suggested that rather than just an if.

That said, for the true, inline, checking experience - since the conversion code is going to be specific to the error type I think it makes sense to use a wrapper type - which implements == (or whatever ops you need) by forwarding to the contained value, but also has a string conversion - either with ostream& << or Catch::StringMaker. Of course if the error type is already strong enough you can just add that string conversion to apply to that - but from what you're talking about they're usually going to be raw ints.

REQUIRE( CheckedError( foo(bar) ) == baz );

[fekir]

Feel free to close #1209. Sorry for not responding before. You got lucky with me paying attention today ;-)

Done :-)

It's also worth bearing in mind that you don't need to repeat that condition check.

Oh, I completely missed that point, should have tested before arguing that a solution with REQUIRE would have been so much better. It seems to cover then all use cases, except the issue of storing the variable separately if it is needed for gaining a more descriptive error message. Honestly for now I'm more than happy even without that option since most of the time I would like this extension for REQUIRE I was dealing with libraries that store error information globally.

[...] I think it makes sense to use a wrapper type [...]

It probably does, but I'm unsure if I'm understanding you correctly. mkdir returns an int (or other integral types, enums, or even structures), 0 in case of success, -1 in case of errors. Other functions might return the same type but using different values for indicating an error. Are you suggesting for every function that returns a different error value to create a wrapper type with the specific error value(s)? (Probably templated in order to reuse the class...)

It is surely doable, I probably need to test it a little more to convince myself that you do not need to write a lot of boilerplate code and "adapters" for working with the test framework...

[philsquared]

It's a little extra boilerplate the first time. I'm guessing you'll use the same error "type" (e.g., set of semantic values) more than once - in which case it will be much less boilerplate.

[kakyoism]

I have a similar problem that I can't think of a better way to get around than using a callback.

My API to test will take memory ownership of its input objects, but will release the ownership optionally.

So in the following test case, I'll get a sure crash if REQUIRE fails:

TEST_CASE("API feature checks out then releases safely", "[memory]") {

     auto intput = std::shared_ptr<MyInput>();

     auto output = std::shared_ptr<MyOutput>();
     MyAPI::DoStuff(input, output);

     REQUIRE(input == output);
     MyAPI::Release(output);
}

Here MyAPI::Release() will not get called if REQUIRE fails. So I'll get

alloc: *** error for object 0x101e2d998: pointer being freed was not allocated

I'd really love to have a Teardown() for cleaning up with my Release() call, but can't seem to find any.

Question

Am I missing obvious macros among the whole bunch in catch.hpp?

[kakyoism]

I found a solution with Catch2's test fixture support.

class MyFixture {
public:
    MyFixture() {
        output_ = std::shared_ptr<MyOutput>();
    }
    virtual ~MyFixture() {
        MyAPI::Release(output_);
    }
public:
     std::shared_ptr<MyOutput> output_;
};

TEST_CASE_METHOD(MyFixture, "API feature checks out then releases safely", "[memory]") {

     auto intput = std::shared_ptr<MyInput>();
     MyAPI::DoStuff(input, output_);

     REQUIRE(input == output_);
}

[puetzk]

similar use-case with COM, where I'd like to capture/decode the Win32/oleautomation GetErrorInfo - but only after receiving a failure HRESULT. So it's the same trouble as with GetLastError(), you need to grab this info after the failed call, but you'd still like to associate the info with the call that failed.

Looking at it that way it's kind of the opposite of UNSCOPED_INFO (which adds to the next assertion) - would it be possible to have something like an AMEND_INFO / ``AMEND_CAPTURE` that added the unscoped info to the immediately preceding assertion (that just failed) rather buffering it for the next one? Because that would do this pretty nicely:

CHECKED_ELSE(f != 0) {
     AMEND_CAPTURE(GetLastError());
}

without actually having to hook directly into the CHECK() itself....