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

~LogCapture() should be noexcept (false) ? #343

Closed
ripopov opened this issue May 8, 2020 · 8 comments
Closed

~LogCapture() should be noexcept (false) ? #343

ripopov opened this issue May 8, 2020 · 8 comments
Labels

Comments

@ripopov
Copy link
Contributor

ripopov commented May 8, 2020

In a g3log.hpp I read:

// 'Design By Contract' stream API. For Broken Contracts:
// unit testing: it will throw std::runtime_error when a contract breaks
// I.R.L : it will exit the application by using fatal signal SIGABRT

So my understanding is that for unit testing I can install setFatalPreLoggingHook that will throw, so I can unit-test contracts with EXPECT_THROW(). However LogCapture() destructor is noexcept (by default), so any attempt to throw from CHECK() will lead to std::terminate.

@KjellKod
Copy link
Owner

KjellKod commented May 8, 2020

Hi,

g3log uses SIGABRT for exiting when a failed contract is discovered.
For unit testing you can replace the default fatal handling with a custom one using

/** REPLACE fatalCallToLogger for fatalCallForUnitTest
using: setFatalExitHandler(...)

You can see test_io.cpp which does this frequently using RestoreFileLogger::RestoreFileLogger(...) which sets this.

setFatalExitHandler(&mockFatalCall);

@KjellKod
Copy link
Owner

KjellKod commented May 8, 2020

The setFatalPreLoggingHook was added by community request to trigger specific actions before going into the fatal handling. It's the setFatalExitHandler that you are after for unit testing.

@ripopov
Copy link
Contributor Author

ripopov commented May 8, 2020

In either case it will be called from destructor, so

g3::setFatalExitHandler([](g3::FatalMessagePtr){
    throw std::runtime_error("unit test exception");
});

will also lead to abort.

So I don't think those comments in source code are correct:

// 'Design By Contract' stream API. For Broken Contracts:
//         unit testing: it will throw std::runtime_error when a contract breaks
//         I.R.L : it will exit the application by using fatal signal SIGABRT
#define CHECK(boolean_expression)        \

// Design By Contract, printf-like API syntax with variadic input parameters.
// Throws std::runtime_eror if contract breaks
#define CHECKF(boolean_expression, printf_like_message, ...)    \
// Backwards compatible. The same as CHECKF.
// Design By Contract, printf-like API syntax with variadic input parameters.
// Throws std::runtime_eror if contract breaks
#define CHECK_F(boolean_expression, printf_like_message, ...)    \

@KjellKod
Copy link
Owner

KjellKod commented May 8, 2020

Check the test_io.cpp . I'm running many, many tests with FATAL LOG calls and CHECK(...)

@KjellKod
Copy link
Owner

KjellKod commented May 8, 2020

You are correct that the code comment should be updated.
With the information I have provided you should be able to run your own unit tests

@ripopov
Copy link
Contributor Author

ripopov commented May 8, 2020

Sorry, I still don't get it, probably I've missing something. I've checked test_io.cpp and I don't see EXPECT_THROW tests there, instead mock installs fatal handler that will push message to logger but will allow code with broken invariant to continue.

@ripopov
Copy link
Contributor Author

ripopov commented May 8, 2020

I think throwing exception from CHECK, as comments suggest is a good idea for unit testing.

@KjellKod
Copy link
Owner

KjellKod commented May 8, 2020

As it’s currently implemented that’s not how it’s done. I’ve provided the pattern for verifying failed CHECK.

If that doesn’t work for your requirements then please provide a PR that targets your concerns and I will take it under evaluation

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

No branches or pull requests

2 participants