Catch2 emulation: list tests has a footer

[CrustyAuklet]

This MR changes the console reporter from free functions to a struct, so that it can maintain state and count the tests when listing them.

I am stuck trying to figure out how to properly initialize the report_callback in the registry. When the console report function was a free function it could just get a reference to that function. But now that it is a struct there needs to be an instance somewhere. The registry is constinit so there shouldn't be a static order issue, but it also can't have a non constexpr constructor.

Some tests also required a reporter instance, and there I declared one either in the test function itself or in an anonymous namespace in the test file if the lifetime needed to outlast the spot it was used.

[cschreib]

This is a bit of an annoyance from the early design of the reporter configuration. The public report_callback is there only for backwards compatibility, and should eventually be removed, I think.

I think the core or the issue is that registry currently doesn't own the reporter state. We currently rely on global std::optional variables, see e.g.: https://github.com/snitch-org/snitch/blob/fe24bc41c5e2096914575e74a3b3350994bbd2dc/include/snitch/snitch_macros_reporter.hpp#L12-L16

It could be that the signature of the callbacks may have to change, see https://github.com/snitch-org/snitch/issues/80. Or we could reserve some storage space in the registry to allocate (in place) the reporter instance.

For this PR, I think we probably should stick with the current API and make it work (probably means having to create a global console reporter, or maybe store it as a member of the registry). If we can't, then we'll have to bite the bullet, break compatibility, and move to v2.0.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.88%. Comparing base (fe24bc4) to head (4617561).

Additional details and impacted files

[](https://app.codecov.io/gh/snitch-org/snitch/pull/168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org) ```diff @@ Coverage Diff @@ ## main #168 +/- ## ========================================== - Coverage 94.06% 93.88% -0.18% ========================================== Files 27 28 +1 Lines 1701 1651 -50 ========================================== - Hits 1600 1550 -50 Misses 101 101 ``` | [Files](https://app.codecov.io/gh/snitch-org/snitch/pull/168?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org) | Coverage Δ | | |---|---|---| | [include/snitch/snitch\_registry.hpp](https://app.codecov.io/gh/snitch-org/snitch/pull/168?src=pr&el=tree&filepath=include%2Fsnitch%2Fsnitch_registry.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org#diff-aW5jbHVkZS9zbml0Y2gvc25pdGNoX3JlZ2lzdHJ5LmhwcA==) | `82.85% <ø> (ø)` | | | [include/snitch/snitch\_reporter\_console.hpp](https://app.codecov.io/gh/snitch-org/snitch/pull/168?src=pr&el=tree&filepath=include%2Fsnitch%2Fsnitch_reporter_console.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org#diff-aW5jbHVkZS9zbml0Y2gvc25pdGNoX3JlcG9ydGVyX2NvbnNvbGUuaHBw) | `100.00% <100.00%> (ø)` | | | [src/snitch\_registry.cpp](https://app.codecov.io/gh/snitch-org/snitch/pull/168?src=pr&el=tree&filepath=src%2Fsnitch_registry.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org#diff-c3JjL3NuaXRjaF9yZWdpc3RyeS5jcHA=) | `95.13% <100.00%> (+0.06%)` | :arrow_up: | | [src/snitch\_reporter\_console.cpp](https://app.codecov.io/gh/snitch-org/snitch/pull/168?src=pr&el=tree&filepath=src%2Fsnitch_reporter_console.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org#diff-c3JjL3NuaXRjaF9yZXBvcnRlcl9jb25zb2xlLmNwcA==) | `97.22% <100.00%> (-1.71%)` | :arrow_down: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/snitch-org/snitch/pull/168?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/snitch-org/snitch/pull/168?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org). Last update [fe24bc4...4617561](https://app.codecov.io/gh/snitch-org/snitch/pull/168?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snitch-org).

[CrustyAuklet]

For this PR, I think we probably should stick with the current API and make it work (probably means having to create a global console reporter, or maybe store it as a member of the registry). If we can't, then we'll have to bite the bullet, break compatibility, and move to v2.0.

making it a member wasn't a bad idea, at least for maintaining compatibility. Since the console reporter is the "default" reporter it's not that absurd I don't think.

I made it a static member, and added some private static functions that call it so that it can be registered with the add_reporter() function. Then I added a public function (with the private API comment) called add_console_reporter() that does the actual work of registering the console reporter so that it is in the registry as expected. These static functions can also be used to assign the report_callback and finish_callback members.

The add_console_reporter() function can be called to initialize a static const variable to replace the macro that registered the console reporter before. The same function can be used to register the console reporter in the mock framework in the tests to avoid adding more global instances.

[cschreib]

Thanks, this is looking good!

I have only one issue with the implementation: the use of a static member rather than a regular member. The instance of the reporter should really be tied to the instance of the registry that uses it, otherwise there could be unexpected sharing of state. Granted, it's unlikely that there will be two registry instances alive in a normal scenario, but it does happen in the snitch tests (testing snitch with itself).

The fault is actually mine; the REGISTER_REPORTER macro that registers reporter classes already use global instances, rather than one instance per registry, which perhaps influenced your choice. It was a mistake on my part, and it needs correcting.

I have been experimenting on top of your PR to fix this, and have a working solution to remove this global state for all reporters, but this is out of scope of this PR. If this is OK with you, I propose we merge your PR as is, and I can follow up with my own changes in another PR to take it a step further.

[CrustyAuklet]

I have only one issue with the implementation: the use of a static member rather than a regular member. The instance of the reporter should really be tied to the instance of the registry that uses it, otherwise there could be unexpected sharing of state. Granted, it's unlikely that there will be two registry instances alive in a normal scenario, but it does happen in the snitch tests (testing snitch with itself).

Agreed. The main reason was that the default value of report_callback needed to be set, but if I added a constructor to registry it would destroy it's ability to be constructed with aggregate initialization.

I took a closer look at the function_ref type though and realized that it does in fact have storage for a pointer in addition to the function callable reference. This constructor fits the bill:

template<typename ObjectType, auto MemberFunction>
constexpr function_ref(ObjectType& obj, constant<MemberFunction>) noexcept;

So I just pushed a change where the console reporter isn't static and I use this constructor to set the default value of report_callback and in registering the reporter.

[CrustyAuklet]

Looks like CI got updated to MSVC 19.39 😄

140

could change #if defined(SNITCH_COMPILER_MSVC) && _MSC_VER >= 1937 && _MSC_VER <= 1938 to #if defined(__cpp_consteval) && __cpp_consteval <= 202211L as suggested by horenmar in that issue.

[cschreib]

If you don't mind, please!