add checks for exceptions

[sakateka]

No description provided.

[mity]

Thanks for the PR.

But I tend to think that TEST_THROW_WHAT is not the right idea. IMHO the code snippet below...

try {
    code
} catch(std::exception& e) {
    TEST_CHECK(e.what() == "xxx");
}

... is more readable; more intuitive (can you guess from the name of TEST_THROW_WHAT what it does? I couldn't); and also more flexible (longer multi-line code, more freedom for the testing condition(s)). I also think that in general the very idea of comparing the error strings is very fragile practice, and I am not sure whether it is good idea to codify/recommending such practice by offering this kind of macro at all.

When it comes to TEST_THROW, I can imagine adding something like that. But its current interface is inconsistent with the interface of other macros. The 2nd argument is likely not really needed in most cases. So, could you replace it with two macros: TEST_THROW(code) and TEST_THROW_(code, ...) which would behave similarly to TEST_CHECK(cond) and TEST_CHECK_(cond, ...)?

Last but not least, it would be great if you can add some comments documenting the new macros, similarly to the other public macros and add something into examples/cpp-example.cc to show it in action.

[sakateka]

Maybe I should start with issue...
TEST_THROW and TEST_THROW_WHAT are bad names, i agree.
I want something like TEST_CATCH, with take exception as second argument.
Don't know how to do better f6f4365
It's my first month with C++ :-)

If the implementation fits, I'll add comments and examples.

[mity]

Well, I see TEST_CATCH and TEST_THROW as similarly bad names. They focus too much on something I see as implementation detail of it. Maybe something like TEST_CHECK_EXC could be better as it really says what the macro is really supposed to do from the point of view of someone who writes the tests, not from point of view of someone who implements the macro.

Also your macros would break easily if used in some situations. Consider for example this:

if(some_condition)
    TEST_CHECK_EXC(call_something(1, 2), std::logic_error);
else
    TEST_CHECK_EXC(call_something(3, 4), std::logic_error);

Given your implementation in the form #define TEST_THROW { ... };, the preprocessor would produce something like this:

if(some_condition)
    { .... };;
else
    { .... };;

There is one trick which is commonly used to make complex macros friendly for their natural usage in C/C++. It is based on wrapping the macro implementation in extra do { .... } while(0). You can then use such macros anywhere as any standard C/C++ statement. For more detailed explanation, see e.g. https://stackoverflow.com/questions/257418/do-while-0-what-is-it-good-for

It's my first month with C++ :-)

Every single expert was in some previous period of time nothing more then a beginner. ;-)

[gahr]

Another thought: do you actually need to check for any other exceptions? The test driver does that already.

Edit: to clarify, you should be able to just check for your expected exception, and let test_do_run__ handle any other exceptions.

[mity]

@gahr That's exactly what's this PR about, isn't it? The macros make the test fail if it does not throw or if it throws something different then expected.

[gahr]

Well, what I'm mildly disputing is the code-path when the test throws an exception different than the expected one.

1. the test throws the expected exception -> success [current behaviour, ok]
2. the test does not throw the expected exception -> failure [current behaviour, ok]
3. the test throws something else -> failure [current behaviour, I'm disputing this one]

In my opinion, if the test throws anything else, it should end up in the same code path as any other test throwing an unexpected exception, namely test_error__.

[sakateka]

@gahr the problem is that the text cond from TEST_CATCH_EXC is lost and the additional description TEST_CATCH_EXC_ is also lost if an unexpected exception terminates test_check__.

[mity]

I'm disputing this one

I see.

It's true it would simplify the code. But on the other side, if someone writes tests for explicit testing of exception behavior, then it's imho likely better to not make the failure to abort rest of the test on the 1st failing check.

[gahr]

That is fair enough, thanks for sharing you thoughts.

[mity]

@sakateka Please revert 9252107 .

1. The purpose of the macro TEST_CHECK_ is exactly to override the standard message with something different (the condition may be sometimes too ugly, long or whatever).

2. This change is unrelated to the original topic of this PR. As a general rule, if you want to add unrelated feature(s), open new PRs for them, so they may be discussed/reviewed/merged independently.

3. It's also hard to process this PR because you never indicate whether you consider it done from your point of view or not and expect some review/feedback/merge from me as maintainer. This commit has landed just when I started to consider whether it is ready for merginh. So it's good practice to indicate the work-in-progress status; either in some comments or e,g. in title of the PR and then rename it when you consider it done (e.g. "[WIP] Add new feature XY" -> "Add new feature XY").

[sakateka]

3. This commit has landed just when I started to consider whether it is ready for merginh

I'm sorry, I wanted to make it better.
There will be no more commits.
Thanks for the detailed explanation)

[mity]

No problem. I understand the learning curve can be nowadays steep, probably much steeper then in the time when I started to code.

[mity]

I tested it in run-time, and I do not like how the output looks like.

1. In more verbose levels (see command line option -v), your macros do not write anything at all when the check is ok. The verbose mode should include info about passing checks. In other words, every check should call test_check__() exactly once (even on success) from any checking macro.

2. Your macro happily creates message like "Check threw unexpected exception: unexpected... failed" which does not make much sense.

   test_check__() assumes the caller adds something after the "Check " meaning "Check (that) something is happening" and that the message is the same on success as on failure. That sentence is then evaluated with "ok" or "failed". (The "ok" case is written only in the verbose levels > 2.)

   If you want to provide more info about the reason of the failure, use test_message__() after calling test_check__() to output it.

3. I dislike that exceptions not derived from std::exception are not handled. If we provide macro checking type of the thrown exception, it must to do so for everything thrown as exception. Especially old codebases still may use some custom class (or hierarchy of classes) not derived from std::exception at all. I believe this is more important then e.g. checking whether std::exception returns NULL from what().

I guess that fixing all of those points would lead to even more complicated beast then the macros already are, and whether something much more simplistic (even though it provides less info) would not be sufficient in the practice:

#define TEST_CHECK_EXC(code, exctype)                                           \
    do {                                                                        \
        bool exc_ok__ = false;                                                  \
        try {                                                                   \
            code;                                                               \ 
        } catch(const exctype&) {                                               \
            exc_ok__= true;                                                     \
        } catch(...) {                                                          \
        }                                                                       \
                                                                                \
        test_check__(exc_ok__, __FILE__, __LINE__, #code " throws " #exctype);  \
    } while(0)

Interested in your opinions.

[sakateka]

Maybe like this?
I like this code!

#ifdef __cplusplus                                                               
#define TEST_CATCH_EXC(code, exctype)                                          \ 
    do {                                                                       \ 
        bool exc_ok__ = false;                                                 \ 
        const char *msg = #code " throws " #exctype;                           \ 
        try {                                                                  \ 
            code;                                                              \ 
        } catch(const exctype &) {                                             \ 
            exc_ok__= true;                                                    \ 
        } catch(...) {                                                         \ 
            msg = #code " threw not " #exctype;                                \ 
        }                                                                      \ 
        test_check__(exc_ok__, __FILE__, __LINE__, msg);                       \ 
    } while(0)                                                                   
#define TEST_CATCH_EXC_(code, exctype, fmt, ...)                               \ 
    do {                                                                       \ 
        bool exc_ok__ = false;                                                 \ 
        const char *alt_fmt = fmt;                                             \ 
        try {                                                                  \ 
            code;                                                              \ 
        } catch(const exctype &) {                                             \ 
            exc_ok__= true;                                                    \ 
        } catch(...) {                                                         \ 
            alt_fmt = fmt " (threw not " #exctype ")";                         \ 
        }                                                                      \ 
        test_check__(exc_ok__, __FILE__, __LINE__, alt_fmt, __VA_ARGS__);      \ 
    } while(0)                                                                   
#endif                                                                           

[mity]

Well, almost.

I would still keep the message for test_check__() the same. As said, I see it more as a title of the check, not result of it. The result is just the "ok" or "failed". If you want to provide more info, use the test_message__():

#define TEST_CATCH_EXC(code, exctype)                                          \
    do {                                                                       \
        bool exc_ok__ = false;                                                 \
        const char *msg__ = NULL;                                              \
        try {                                                                  \
            code;                                                              \
            msg__ = "No exception thrown.";                                    \
        } catch(const exctype &) {                                             \
            exc_ok__= true;                                                    \
        } catch(...) {                                                         \
            msg__ = "Unexpected exception thrown.";                            \
        }                                                                      \
        test_check__(exc_ok__, __FILE__, __LINE__, #code " throws " #exctype); \
        if(msg__ != NULL)                                                      \
            test_message__("%s", msg__);                                       \
    } while(0)           

And similarly the other macro: That would just pass the custom message into test_check__() as TEST_CHECK_ does.

[sakateka]

test_message__ is a good option.
I committed a fixed version of the code

[mity]

Thanks. Merged.

[mity]

I made some changes to it.

• Removed const from the catch after all. It is more flexible, as it allow exctype to include it.
• After some more thinking, I decided to rename the macros to very explicit TEST_EXCEPTION and TEST_EXCEPTION_.

[gahr]

Removed const from the catch after all. It is more flexible, as it allow exctype to include it.

I don't think this is a good idea. I would expect the API to take the exact type thrown, not the type you could use to bind it to a variable. What flexibility do you have in mind?

[mity]

It is uncommon, but this is still valid C++:

void func()
{
   const char* err = "foo";
   throw err;
}

Then it is natural to write:

CHECK_EXCEPTION(func(), const char*);

It would break if there is yet another const from the macro expansion.

[gahr]

How about changing the expansion to exctype & const

[gahr]

You want to take the type that is thrown const char* by const& -> const char * const&

[mity]

Ohh yeah, that seems to be the solution. Thanks.