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.:

snitch/include/snitch/snitch_macros_reporter.hpp

Lines 12 to 16 in fe24bc4

	#define SNITCH_REGISTER_REPORTER_IMPL(NAME, TYPE, COUNTER) \
	static std::optional<TYPE> SNITCH_MACRO_CONCAT(reporter_, COUNTER); \
	static void SNITCH_MACRO_CONCAT(reporter_init_, COUNTER)(snitch::registry & r) noexcept { \
	SNITCH_MACRO_CONCAT(reporter_, COUNTER).emplace(r); \
	} \

It could be that the signature of the callbacks may have to change, see #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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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	Coverage Δ
include/snitch/snitch_registry.hpp	82.85% <ø> (ø)
include/snitch/snitch_reporter_console.hpp	100.00% <100.00%> (ø)
src/snitch_registry.cpp	95.13% <100.00%> (+0.06%)	⬆️
src/snitch_reporter_console.cpp	97.22% <100.00%> (-1.71%)	⬇️

---

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 fe24bc4...4617561. Read the comment docs.

[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!

[cschreib]

Thank you!

There's one minor comment I would make, but I don't think it's blocking this PR from being merged. Although the console reporter is a bit special (in that, it is the default), we should still try to keep it as standard as possible, so it doesn't deviate from other reporters. The main reason is that, it being the default, I think it will be where most people will look for an example if they want to create their own reporter.

With this in mind, I would prefer to keep the constructor taking a registry&, and keeping the REGISTER_REPORTER macro at the bottom of snitch_reporter_console.cpp.

I'm happy to leave it as it is now, and I can address this when I tackle issue #171; I think it will be easier then (see e.g. d3d5f8a).