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.

Sign up for GitHub

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

Draft: Feat/catch2 compat #169

Closed
wants to merge 4 commits into from
Closed

Draft: Feat/catch2 compat #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72cf503
add section event for reporters
CrustyAuklet Apr 28, 2024
4c1f43a
update approval tests
CrustyAuklet May 12, 2024
2bfe6a7
catch2 XML is at version 3
CrustyAuklet May 22, 2024
5f5afc6
Don't use chrono in snitch headers
CrustyAuklet Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion include/snitch/snitch_section.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,19 @@
#include "snitch/snitch_config.hpp"
#include "snitch/snitch_test_data.hpp"

#include <type_traits>

namespace snitch::impl {
struct section_entry_checker {
section data = {};
test_state& state;
bool entered = false;
bool entered = false;
std::size_t asserts = 0;
std::size_t failures = 0;
std::size_t allowed_failures = 0;
#if SNITCH_WITH_TIMINGS
std::make_signed_t<std::size_t> start_time = 0;
#endif

SNITCH_EXPORT ~section_entry_checker();

Expand Down
23 changes: 23 additions & 0 deletions include/snitch/snitch_test_data.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,27 @@ struct test_case_ended {
bool failure_allowed = false;
};

struct section_started {
/// Identifiers (name, description)
section_id id = {};
/// Location (file, line)
source_location location = {};
};

struct section_ended {
/// Identifiers (name, description)
section_id id = {};
/// Location (file, line)
source_location location = {};
bool skipped = false;
std::size_t assertion_count = 0;
std::size_t assertion_failure_count = 0;
std::size_t allowed_assertion_failure_count = 0;
#if SNITCH_WITH_TIMINGS
float duration = 0.0f;
#endif
};

struct assertion_failed {
const test_id& id;
section_info sections = {};
Expand Down Expand Up @@ -231,6 +252,8 @@ using data = std::variant<
test_run_ended,
test_case_started,
test_case_ended,
section_started,
section_ended,
assertion_failed,
assertion_succeeded,
test_case_skipped,
Expand Down
56 changes: 38 additions & 18 deletions src/snitch_reporter_catch2_xml.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,6 @@ void open_close(

template<typename T>
void report_assertion(reporter& rep, const registry& r, const T& e, bool success) noexcept {
for (const auto& s : e.sections) {
open(
rep, r, "Section",
{{"name", make_escaped(s.id.name)},
{"filename", make_escaped(s.location.file)},
{"line", make_string(s.location.line)}});
}

for (const auto& c : e.captures) {
open(rep, r, "Info");
print(rep, r, make_escaped(c));
Expand Down Expand Up @@ -166,10 +158,6 @@ void report_assertion(reporter& rep, const registry& r, const T& e, bool success
close(rep, r, "Expression");
}},
e.data);

for (const auto& s [[maybe_unused]] : e.sections) {
close(rep, r, "Section");
}
}
} // namespace

Expand All @@ -194,7 +182,7 @@ void reporter::report(const registry& r, const snitch::event::data& event) noexc
*this, r, "Catch2TestRun",
{{"name", make_escaped(e.name)},
{"rng-seed", "0"},
{"xml-format-version", "2"},
{"xml-format-version", "3"},
{"catch2-version", SNITCH_FULL_VERSION ".snitch"},
{"filters", make_filters(e.filters)}});
},
Expand All @@ -205,13 +193,15 @@ void reporter::report(const registry& r, const snitch::event::data& event) noexc
e.assertion_count - e.assertion_failure_count -
e.allowed_assertion_failure_count)},
{"failures", make_string(e.assertion_failure_count)},
{"expectedFailures", make_string(e.allowed_assertion_failure_count)}});
{"expectedFailures", make_string(e.allowed_assertion_failure_count)},
{"skips", make_string(e.skip_count)}});

node(
*this, r, "OverallResultsCases",
{{"successes", make_string(e.run_count - e.fail_count - e.allowed_fail_count)},
{"failures", make_string(e.fail_count)},
{"expectedFailures", make_string(e.allowed_fail_count)}});
{"expectedFailures", make_string(e.allowed_fail_count)},
{"skips", make_string(e.skip_count)}});

close(*this, r, "Catch2TestRun");
},
Expand All @@ -228,16 +218,46 @@ void reporter::report(const registry& r, const snitch::event::data& event) noexc
node(
*this, r, "OverallResult",
{{"success", e.state == test_case_state::failed ? "false" : "true"},
{"skips", e.state == test_case_state::skipped ? "1" : "0"},
{"durationInSeconds", make_string(e.duration)}});
# else
node(
*this, r, "OverallResult",
{{"success", e.state == test_case_state::failed ? "false" : "true"}});
{{"success", e.state == test_case_state::failed ? "false" : "true"},
{"skips", e.state == test_case_state::skipped ? "1" : "0"}});
# endif
close(*this, r, "TestCase");
},
[&](const snitch::event::test_case_skipped&) {
// Nothing to do; this gets reported as "success".
[&](const snitch::event::section_started& e) {
open(
*this, r, "Section",
{{"name", make_escaped(e.id.name)},
{"filename", make_escaped(e.location.file)},
{"line", make_string(e.location.line)}});
},
[&](const snitch::event::section_ended& e) {
node(
*this, r, "OverallResults",
{{"successes", make_string(
e.assertion_count - e.assertion_failure_count -
e.allowed_assertion_failure_count)},
{"failures", make_string(e.assertion_failure_count)},
{"expectedFailures", make_string(e.allowed_assertion_failure_count)},
{"skipped", e.skipped?"true":"false"}
# if SNITCH_WITH_TIMINGS
,
{"durationInSeconds", make_string(e.duration)}
# endif
});
close(*this, r, "Section");
},
[&](const snitch::event::test_case_skipped& e) {
open(
*this, r, "Skip",
{{"filename", make_escaped(e.location.file)},
{"line", make_string(e.location.line)}});
print(*this, r, e.message);
close(*this, r, "Skip");
},
[&](const snitch::event::assertion_failed& e) { report_assertion(*this, r, e, false); },
[&](const snitch::event::assertion_succeeded& e) {
Expand Down
2 changes: 2 additions & 0 deletions src/snitch_reporter_console.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ void reporter::report(const registry& r, const event::data& event) noexcept {
make_colored(full_name, r.with_color, color::highlight1), "\n");
#endif
},
[&](const snitch::event::section_started&) {},
[&](const snitch::event::section_ended&) {},
[&](const snitch::event::test_case_skipped& e) {
r.print(make_colored("skipped: ", r.with_color, color::skipped));
print_location(r, e.id, e.sections, e.captures, e.location);
Expand Down
2 changes: 2 additions & 0 deletions src/snitch_reporter_teamcity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ void report(const registry& r, const snitch::event::data& event) noexcept {
send_message(r, "testFinished", {{"name", make_full_name(e.id)}});
# endif
},
[&](const snitch::event::section_started&) {},
[&](const snitch::event::section_ended&) {},
Comment on lines +151 to +152
Copy link
Member

@cschreib cschreib May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TeamCity uses the following:

  • ##teamcity[blockOpened name='...' description='...']
  • ##teamcity[blockClosed name='...']

Then we can probably remove the ad-hoc printing of sections in print_assertion()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the print_assertion() is see where it iterates over the sections, but I don't see how it outputs the test you show here. I don't see and "blockOpened" or "blockCosed"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it :)

[&](const snitch::event::test_case_skipped& e) {
send_message(
r, "testIgnored",
Expand Down
45 changes: 44 additions & 1 deletion src/snitch_section.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,42 @@

#include "snitch/snitch_console.hpp"
#include "snitch/snitch_registry.hpp"
#include "snitch/snitch_test_data.hpp"

#if SNITCH_WITH_EXCEPTIONS
# include <exception>
#endif
#if SNITCH_WITH_TIMINGS
# include <chrono>
#endif

namespace snitch::impl {
#if SNITCH_WITH_TIMINGS
using fsec = std::chrono::duration<float>;
using snitch_clock = std::chrono::steady_clock;
#endif

section_entry_checker::~section_entry_checker() {
if (entered) {
#if SNITCH_WITH_EXCEPTIONS
if (std::uncaught_exceptions() > 0) {
// We are unwinding the stack because an exception has been thrown;
// avoid touching the section state since we will want to report where
// the exception was thrown.
state.reg.report_callback(
state.reg, event::section_ended{
state.sections.current_section.back().id,
state.sections.current_section.back().location, true});
Comment on lines +27 to +30
Copy link
Member

@cschreib cschreib May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in the approval tests; I think this is done too soon. When an exception is in flight, it has not been reported yet, but we now close the section immediately. This results in all unhandled exceptions being reported at the root of the test case, rather than inside the section from which it originated.

I believe this particular case (close the section when unwinding an exception) can be handled in the registry instead, to solve this problem. That would be here:

snitch/src/snitch_registry.cpp

Lines 554 to 558 in d663212

} catch (const std::exception& e) {
report_assertion(false, "unexpected std::exception caught; message: ", e.what());
} catch (...) {
report_assertion(false, "unexpected unknown exception caught");
}

Also, is this particular path currently missing the asserts, failures, and allowed_failures counts? (also duration, skips)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return;
}
#endif

pop_location(state);

asserts = state.asserts - asserts;
failures = state.failures - failures;
allowed_failures = state.allowed_failures - allowed_failures;
Comment on lines +37 to +39
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives two different meanings to section_entry_checker::asserts/failures/allowed_failures depending on context: on creation they hold the initial state, and on destruction they hold the difference. This will be a source of confusion, I fear. But in fact, we only need to store the initial state (and should probably rename the member variables as such, e.g. initial_sate.asserts/initial_state.failures/...); the difference computed here could be stored in local variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could instead store the actual number of assertions (etc) that were recorded, and make the registry propagate the counts to all open sections in register_assertion(). This is a little bit more work for the CPU, but the code might be simpler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could instead store the actual number of assertions (etc) that were recorded, and make the registry propagate the counts to all open sections in register_assertion(). This is a little bit more work for the CPU, but the code might be simpler.

This also might be necessary since I am noticing in my tests that Catch2 counts these things cumulatively for nested sections but with this strategy snitch does not.

given

SECTION("Section1") {
    CHECK(true);
    SECTION("Section1.1") {
        CHECK(false);
    }
}

this code outputs

    <Section name="Section1" filename="all_fail.cpp" line="10">
      <Section name="Section1.1" filename="all_fail.cpp" line="12">
        <Expression success="false" type="CHECK" filename="all_fail.cpp" line="13">
          <Original>
            false
          </Original>
          <Expanded>
            false
          </Expanded>
        </Expression>
        <OverallResults successes="0" failures="1" expectedFailures="0" skipped="false" durationInSeconds=<TIME>/>
      </Section>
      <OverallResults successes="0" failures="0" expectedFailures="0" skipped="false" durationInSeconds=<TIME>/>
    </Section>

Catch2:

    <Section name="Section1" filename="all_fail.cpp" line="10">
      <Section name="Section1.1" filename="all_fail.cpp" line="12">
        <Expression success="false" type="CHECK" filename="all_fail.cpp" line="13">
          <Original>
            false
          </Original>
          <Expanded>
            false
          </Expanded>
        </Expression>
        <OverallResults successes="0" failures="1" expectedFailures="0" skipped="false" durationInSeconds=<TIME>/>
      </Section>
      <OverallResults successes="1" failures="1" expectedFailures="0" skipped="false" durationInSeconds=<TIME>/>
    </Section>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are counting the actual count for each section, then that needs to happen in the registry too? I don't see a way to do it in the section_entry_checker since it is only called on entry and exit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep; I've implemented this so the registry does the propagation of assert counts to all currently open sections.


if (state.sections.depth == state.sections.levels.size()) {
// We just entered this section, and there was no child section in it.
// This is a leaf; flag that a leaf has been executed so that no other leaf
Expand All @@ -29,6 +46,19 @@ section_entry_checker::~section_entry_checker() {
// that we don't know about yet. Popping will be done when we exit from the parent,
// since then we will know if there is any sibling.
state.sections.leaf_executed = true;
#if SNITCH_WITH_TIMINGS
const auto end_time = snitch_clock::now().time_since_epoch();
const auto duration =
std::chrono::duration_cast<fsec>(end_time - snitch_clock::duration{start_time});
state.reg.report_callback(
state.reg, event::section_ended{
data.id, data.location, false, asserts, failures, allowed_failures,
duration.count()});
#else
state.reg.report_callback(
state.reg,
event::section_ended{data.id, data.location, asserts, failures, allowed_failures});
#endif
} else {
// Check if there is any child section left to execute, at any depth below this one.
bool no_child_section_left = true;
Expand All @@ -42,6 +72,10 @@ section_entry_checker::~section_entry_checker() {

if (no_child_section_left) {
// No more children, we can pop this level and never go back.
state.reg.report_callback(
state.reg, event::section_ended{
state.sections.current_section.back().id,
state.sections.current_section.back().location});
state.sections.levels.pop_back();
}
}
Expand All @@ -66,8 +100,13 @@ section_entry_checker::operator bool() {

state.sections.levels.push_back({});
}

#if SNITCH_WITH_TIMINGS
start_time = snitch_clock::now().time_since_epoch().count();
#endif
++state.sections.depth;
asserts = state.asserts;
failures = state.failures;
allowed_failures = state.allowed_failures;

auto& level = state.sections.levels[state.sections.depth - 1];

Expand All @@ -89,6 +128,10 @@ section_entry_checker::operator bool() {
(level.current_section_id == level.previous_section_id &&
state.sections.depth < state.sections.levels.size())) {

// First time entering this section, emit the section start event
if (level.current_section_id == level.previous_section_id + 1) {
state.reg.report_callback(state.reg, event::section_started{data.id, data.location});
}
level.previous_section_id = level.current_section_id;
state.sections.current_section.push_back(data);
push_location(
Expand Down
Loading
Loading