Draft: Feat/catch2 compat #169

CrustyAuklet · 2024-05-12T03:56:39Z

Very rought draft, but I wanted to open it so others can look and comment.

This is work to make the Catch2 XML output match up with Catch2. Like the other open MRs I notices this when integrating with my teams IDE and (more importantly) CI test reporting systems.

Without these changes CI and IDEs work ok, but failures in a section just fail the whole test. When we were using Catch2 a failure would be specific to a section allowing us to quickly drill down to the problem. So looking into the difference it is just the lack of section output in the "high" verbosity mode. Catch2 outputs section XML data by default.

It was fairly easy to make it work with basic testing, pass and fail. There are more complex edge cases around skips, nested sections, exceptions being thrown, and different verbosity levels. I am still ironing those out and need to make sure there is testing for it all.

As I have time this week i will add some examples here, and some pictures to show the different in CI/IDEs.

cschreib · 2024-05-12T10:27:11Z

Thanks for spotting this. I could have sworn I got the output for sections to match Catch2 when I last tested this; but perhaps I had looked at a too simple case and missed the bigger picture.

As I have time this week i will add some examples here, and some pictures to show the different in CI/IDEs.

It would be very interesting to compare the actual XML output between the two in a real-world case; the test project I use for testing and benchmarks doesn't actually use sections.

cschreib · 2024-05-27T07:30:26Z

tests/approval_tests/data/expected/reporter_catch2_xml_allfail

+    <Failure filename="*testing_reporters.cpp" line="*">
+      unexpected std::exception caught; message: unexpected error
+    </Failure>


I think this message is now printed too late; I believe we want it to appear inside the <Section> where the exception was thrown, as it was before. See comment in ~section_entry_checker().

cschreib · 2024-05-27T07:33:52Z

src/snitch_section.cpp

+            state.reg.report_callback(
+                state.reg, event::section_ended{
+                               state.sections.current_section.back().id,
+                               state.sections.current_section.back().location, true});


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)

cschreib · 2024-05-27T11:55:12Z

include/snitch/snitch_section.hpp

@@ -3,12 +3,21 @@

 #include "snitch/snitch_config.hpp"
 #include "snitch/snitch_test_data.hpp"
+#if SNITCH_WITH_TIMINGS
+#    include <chrono>


We had, so far, managed to avoid including <chrono> in public headers. Could we keep it that way, to keep compilation time down? I just ran some tests, measuring GCC time to parse an empty .cpp with #include:

include snitch_all.hpp as of today's main branch: 38ms.

same, but add <chrono>: 46ms (+8ms, or +22%).

This feels like a high price to pay for just storing a time point. We could perhaps store the numerical value of the time point in the struct here, as an anonymous std::size_t or similar that is >= sizeof(std::chrono::steady_clock::rep), and then convert back and forth to actual time points in the *.cpp files.

Makes sense, I see that <type_traits> is included already so I use that instead to create a signed version of size_t (since not all platforms define ssize_t). I added some aliases in the snitch::impl for chrono in the cpp file as well to make the chrono code less verbose and avoid bugs if anyone wants to change clocks or duration types in the future.

I ended up adding a new file snitch_time.cpp to wrap this up, since we needed chrono functionality elsewhere too (registry). In the end, all we need is a simple API with a function returning the "current time point" in some unspecified unit (I picked "number of nanoseconds since test run start" -- always positive with a steady_clock so a std::size_t is OK, and will only wrap around after ~600 years of execution, also OK) and another function to calculate the time difference between two time points.

cschreib · 2024-05-27T12:00:39Z

src/snitch_reporter_teamcity.cpp

+            [&](const snitch::event::section_started&) {},
+            [&](const snitch::event::section_ended&) {},


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()

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"?

cschreib · 2024-05-27T12:10:57Z

src/snitch_section.cpp

+        asserts          = state.asserts - asserts;
+        failures         = state.failures - failures;
+        allowed_failures = state.allowed_failures - allowed_failures;


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.

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.

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>

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.

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

src/snitch_section.cpp

cschreib · 2024-05-27T12:48:17Z

src/snitch_section.cpp

+            state.reg.report_callback(
+                state.reg,
+                event::section_ended{
+                    data.id, data.location, false, asserts, failures, allowed_failures, duration});
+#else
+            state.reg.report_callback(
+                state.reg,
+                event::section_ended{data.id, data.location, asserts, failures, allowed_failures});


I think all the reporting logic should be outsourced to registry; the report_callback isn't really meant to be used outside the registry class (it is exposed as a public member primarily so it can be assigned, otherwise it would be private). We probably want helper members functions like registry::report_section_entered / registry::report_section_exited to achieve this.

This sounds much better. Adding the reporting here was the stuff I was most uncomfortable with and it seems very hacky.

CrustyAuklet · 2024-05-28T06:31:10Z

thanks for the feedback, I will look it over in depth this coming week.

I've been working on making tests and comparing Catch2 to Snitch output in a repo on my account here: https://github.com/CrustyAuklet/snitch-xml-output. So far it's pretty close but I will have to add some more variations, especially to cover the exception stuff.

cschreib · 2024-06-27T19:42:51Z

Hi there! Just wondering if this was still on your mind. Let me know if not, and I can take over.

CrustyAuklet · 2024-06-30T18:32:11Z

It is, I have just had a ton on my plate. I am going to try to re-familiarize myself with it this afternoon and see if have any questions on your feedback.

I won't be offended at all if you think it's an easy fix for you to do and you want to get it done 😄

Catch2 has a section enter and exit event. This allows for finer resolution display of test failures in IDEs and CI dashboards without giving up the benefits of using sections.

As suggested in code review, don't include chrono in the snitch headers since this increases compile times. Use a basic integral representation type and then convert back and forth to chrono types in the source files when actually reading clocks or calculating times.

CrustyAuklet · 2024-07-02T19:41:31Z

fixed some of the small easy stuff and rebased, but spent a lot of time running some example tests in that other repo with exceptions. I think most of this code needs to be moved to the registry like you said.

cschreib · 2024-09-13T14:05:25Z

Thanks for looking at it further. Does snitch reporting work as expected in your tests now? If so, I can do another pass of review, and if all looks fine, I can also take care of the final refactoring.

FYI: I just hit some of the issues you were having, so I can confirm (if this was even needed!) that things are currently broken on the main branch. I'm implementing a test runner for SublimeText, and for the sake of the exercise, I implemented a test adaptor for Catch2 first, and then tried to use it with snitch without touching it. Didn't work out of the box! I'm hoping this will be solved with your PR.

CrustyAuklet · 2024-09-16T17:54:34Z

Without exceptions it works as expected and I am using it on some of my work projects. But with exceptions It doesn't match up with the output of Catch2, and that is where I got a bit stuck and maybe the code does need to move into the registry as you suggested.

The test using exceptions in the repo https://github.com/CrustyAuklet/snitch-xml-output shows some of the differences. Catch2 is more verbose but the real issue is that is seems that exceptions thrown from within sections are not output until the parent section.

Here are the output XML files from that test:
exceptions.catch2.txt
exceptions.snitch.txt

cschreib · 2024-09-27T20:50:21Z

I believe the remaining issues are now sorted if you try the branch cschreib/catch2-compat. I created a PR for it #183 to get the CI runs. If you have no objection, I suggest switching to the new PR and closing this one (unless you feel like doing the git magic to pull my changes back into your branch; you'll need to rebase though). If you wanted to review the changes I pushed over your last commit, feel free!

By the way, if you need to do another PR later for some other change, you should have permissions to create a branch direclty in this repo without having to work from a fork. This would make it possible for us to work on the same branch when needed (which I could not do here, since this PR's branch belongs exclusively to you).

cschreib · 2024-10-15T07:16:43Z

Superseded by #183; thanks again for kick-starting this!

cschreib mentioned this pull request May 12, 2024

Catch2 XML reporter does not output sections properly #170

Closed

cschreib linked an issue May 12, 2024 that may be closed by this pull request

Catch2 XML reporter does not output sections properly #170

Closed

CrustyAuklet force-pushed the feat/catch2-compat branch 3 times, most recently from 8b1d448 to cded7ab Compare May 17, 2024 02:56

CrustyAuklet force-pushed the feat/catch2-compat branch from cded7ab to 0930b34 Compare May 20, 2024 17:03

cschreib reviewed May 27, 2024

View reviewed changes

src/snitch_section.cpp Outdated Show resolved Hide resolved

cschreib reviewed May 27, 2024

View reviewed changes

CrustyAuklet force-pushed the feat/catch2-compat branch from e8b7f09 to e49def5 Compare July 2, 2024 17:18

CrustyAuklet added 4 commits July 2, 2024 11:20

add section event for reporters

72cf503

Catch2 has a section enter and exit event. This allows for finer resolution display of test failures in IDEs and CI dashboards without giving up the benefits of using sections.

update approval tests

4c1f43a

catch2 XML is at version 3

2bfe6a7

CrustyAuklet force-pushed the feat/catch2-compat branch from af19651 to 5f5afc6 Compare July 2, 2024 18:20

cschreib mentioned this pull request Sep 27, 2024

Fix Catch2 XML format compatibility #183

Merged

cschreib closed this Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Feat/catch2 compat #169

Draft: Feat/catch2 compat #169

CrustyAuklet commented May 12, 2024

cschreib commented May 12, 2024

cschreib May 27, 2024 •

edited

Loading

cschreib Sep 27, 2024

cschreib May 27, 2024 •

edited

Loading

cschreib Sep 27, 2024

cschreib May 27, 2024

CrustyAuklet Jul 2, 2024

cschreib Sep 27, 2024

cschreib May 27, 2024 •

edited

Loading

CrustyAuklet Jul 2, 2024

cschreib Sep 27, 2024

cschreib May 27, 2024

cschreib May 27, 2024

CrustyAuklet Jul 2, 2024

CrustyAuklet Jul 2, 2024

cschreib Sep 27, 2024

cschreib May 27, 2024

CrustyAuklet Jul 2, 2024

cschreib Sep 27, 2024

CrustyAuklet commented May 28, 2024

cschreib commented Jun 27, 2024

CrustyAuklet commented Jun 30, 2024

CrustyAuklet commented Jul 2, 2024

cschreib commented Sep 13, 2024 •

edited

Loading

CrustyAuklet commented Sep 16, 2024

cschreib commented Sep 27, 2024 •

edited

Loading

cschreib commented Oct 15, 2024

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

		[&](const snitch::event::section_started&) {},
		[&](const snitch::event::section_ended&) {},

Draft: Feat/catch2 compat #169

Draft: Feat/catch2 compat #169

Conversation

CrustyAuklet commented May 12, 2024

cschreib commented May 12, 2024

cschreib May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschreib May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschreib May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrustyAuklet commented May 28, 2024

cschreib commented Jun 27, 2024

CrustyAuklet commented Jun 30, 2024

CrustyAuklet commented Jul 2, 2024

cschreib commented Sep 13, 2024 • edited Loading

CrustyAuklet commented Sep 16, 2024

cschreib commented Sep 27, 2024 • edited Loading

cschreib commented Oct 15, 2024

cschreib May 27, 2024 •

edited

Loading

cschreib May 27, 2024 •

edited

Loading

cschreib May 27, 2024 •

edited

Loading

cschreib commented Sep 13, 2024 •

edited

Loading

cschreib commented Sep 27, 2024 •

edited

Loading