Assert Info reset need to also reset result disposition to normal to handle uncaught exception correctly #2723
Conversation
…r populateReaction. And reset assertion info would also reset the result disposition to normal, so that any uncaught exception would be reported as failure
Codecov Report
@@ Coverage Diff @@
## devel #2723 +/- ##
==========================================
+ Coverage 91.20% 91.29% +0.08%
==========================================
Files 192 192
Lines 7843 7849 +6
==========================================
+ Hits 7153 7165 +12
+ Misses 690 684 -6 |
| Catch::AssertionHandler catchAssertionHandler( | ||
| "REQUIRE"_catch_sr, | ||
| CATCH_INTERNAL_LINEINFO, | ||
| "Dummy", | ||
| Catch::ResultDisposition::Normal ); |
There was a problem hiding this comment.
I understand what the other two tests are for, but what is this one supposed to check?
There was a problem hiding this comment.
The function is not covered in coverage, as it is preprocessor switched off in tests. But it needs to be fixed for completeness. So adding unit tests for that to satisfy the coverage percentage requirement.
| @@ -447,6 +449,7 @@ namespace Catch { | |||
| AssertionResult result(m_lastAssertionInfo, CATCH_MOVE(tempResult)); | |||
|
|
|||
| assertionEnded(CATCH_MOVE(result) ); | |||
| resetAssertionInfo(); | |||
There was a problem hiding this comment.
Is this actually neeeded? RunContext::assertionEnded (called on the line before this one) already calls resetAssertionInfo.
There was a problem hiding this comment.
Wait sorry, I didn't notice that you changed assertionEnded to not reset assertion info anymore.
|
Thanks. |
Description
why?
After CHECK_ELSE is run, uncaught exception would pass unit tests which causes huge issues.
what?
This pull request added two unit tests to demo how uncaught exception are swallowed silently, and fixes that with resetting the result disposition flag to normal. Since populateReaction uses the last assertion info, it needs to be reset after populateReaction is called.
GitHub Issues
Closes #2719