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

Fix Catch2 XML format compatibility #183

Merged
merged 20 commits into from
Oct 15, 2024
Merged

Fix Catch2 XML format compatibility #183

merged 20 commits into from
Oct 15, 2024

Conversation

cschreib
Copy link
Member

@cschreib cschreib commented Sep 27, 2024

Originally contributed by @CrustyAuklet in #169. Rebased on the latest main branch and refactoring to solve the remaining issues.

Also fixes #179.

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (ecfa3d6) to head (c8bc7e0).
Report is 24 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
+ Coverage   94.03%   94.17%   +0.13%     
==========================================
  Files          29       30       +1     
  Lines        1709     1750      +41     
==========================================
+ Hits         1607     1648      +41     
  Misses        102      102              
Flag Coverage Δ
94.17% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
include/snitch/snitch_registry.hpp 86.36% <ø> (ø)
src/snitch_capture.cpp 92.00% <ø> (ø)
src/snitch_registry.cpp 95.58% <100.00%> (+0.39%) ⬆️
src/snitch_reporter_catch2_xml.cpp 95.45% <ø> (-0.44%) ⬇️
src/snitch_reporter_console.cpp 97.14% <ø> (ø)
src/snitch_reporter_teamcity.cpp 100.00% <100.00%> (ø)
src/snitch_section.cpp 88.46% <100.00%> (+2.74%) ⬆️
src/snitch_test_data.cpp 95.83% <100.00%> (+0.18%) ⬆️
src/snitch_time.cpp 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecfa3d6...c8bc7e0. Read the comment docs.

@cschreib
Copy link
Member Author

Trying to add more tests to catch the lines currently missing from the coverage; I noticed there's an edge case to the current implementation:

struct destructor_asserter {
    ~destructor_asserter() {
        CHECK(false);
    }
};

TEST_CASE("test") {
    SECTION("section 1") {
        destructor_asserter a;
        SECTION("section 2") {
            throw std::runtime_error("oops");
        }
    }
}

This is actually a pre-existing issue introduced in #178.

This currently causes us to miss the section_ended event for "section 2", because the "held" section state (created when an uncaught exception is detected) is currently cleared unconditionally whenever we encounter a new CHECK() (or SECTION(), or CAPTURE(), or INFO()). I think there's a way to address this, but I wonder if I will have to rethink the exception handling logic for sections... Will pick it again a bit later.

@cschreib
Copy link
Member Author

cschreib commented Sep 28, 2024

Solved by reworking the "held state clearing" logic. This is no longer done unconditionally on the first CHECK*()/CAPTURE()/INFO()/SECTION() encountered (which was the implementation from #178), but instead we detect when the stack is being unwinded because of an exception, and skip the clearing if so. Otherwise, we close any section that was left open in the "held state", and then clear the held state. This solves the unbalanced section events.

We also expose this logic in a new public function snitch::notify_exception_handled();. This lets users trigger the clearing manually when they think the time is right (e.g., in their custom catch handler). This solves #179 in passing.

TEST_CASE("foo") {
    try {
        // Your test code that can throw...
    } catch (...) {
        // Your custom handling logic...
        
        // This is now necessary:
        snitch::notify_exception_handled();
    }

    // Now, all following code works fine.
}

If that call is forgotten, most things will still work fine: only a following unhandled exception would have confusing contextual information (i.e., wrong capture/info/section data reported). More detailed explanations are in #179.

@cschreib cschreib force-pushed the cschreib/catch2-compat branch from a426ccc to c8bc7e0 Compare September 30, 2024 08:00
@cschreib
Copy link
Member Author

I'm happy to merge this. @CrustyAuklet if you are able to test this before merge, it would be awesome! Otherwise I'll just merge it sometime this weekend, and we can fix any issues in a follow up PR. Thanks for your help regardless, and sorry this took so long to merge...

@cschreib cschreib merged commit 74bdd8e into main Oct 15, 2024
43 checks passed
@cschreib cschreib deleted the cschreib/catch2-compat branch October 15, 2024 07:16
@cschreib cschreib linked an issue Oct 15, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants