Report generator states for failed assertions #2509

gotschmarcel · 2022-08-27T15:20:24Z

Description

As a user of catch I often came across the situation that I wanted to know what the generator values were when a test failed. I'm aware of the CAPTURE API, but I always wanted something automatic.

I'm not entirely certain what kind of format would be preferred, so I'm happy to change and implement it for other reporters too. Currently, I've only implemented this for the console reporter.

Some approval tests don't pass even though I haven't touched these reporters. Any help is appreciated.

GitHub Issues

#2366

src/catch2/interfaces/catch_interfaces_reporter.cpp

gotschmarcel · 2022-08-27T19:51:51Z

src/catch2/interfaces/catch_interfaces_reporter.hpp

@@ -8,10 +8,12 @@
 #ifndef CATCH_INTERFACES_REPORTER_HPP_INCLUDED
 #define CATCH_INTERFACES_REPORTER_HPP_INCLUDED

+#include "catch2/interfaces/catch_interfaces_capture.hpp"


Those probably shouldn't be here, I believe my IDE auto imported those 🙄

src/catch2/internal/catch_run_context.hpp

gotschmarcel · 2022-08-27T19:53:21Z

src/catch2/reporters/catch_reporter_console.cpp

+            return;
+        }
+
+        stream << "with " << pluralise(stats.generatorInfos.size(), "generator"_sr) << "\n";


The newline should be just a character. Same below. Will fix.

gotschmarcel · 2022-08-27T20:07:57Z

src/catch2/internal/catch_run_context.cpp

@@ -319,6 +319,16 @@ namespace Catch {
        return tracker;
    }

+    void RunContext::trackGeneratorState( GeneratorInfo info ) {
+        // Avoid redundant entries, in case a generator is used within a loop.


I added the unique name to the info, so this comparison should be fine even if we have more than one generator with the same arguments on the same line. Both of them should show up. This should be tested. Where would the test go? I assume some test that runs through via the approval tests.

src/catch2/interfaces/catch_interfaces_capture.hpp

gotschmarcel · 2022-08-28T10:04:36Z

tests/SelfTest/Baselines/console.sw.approved.txt

@@ -228,6 +228,9 @@ Generators.tests.cpp:<line number>: PASSED:
  REQUIRE( counter < 7 )
 with expansion:
  3 < 7
+with 1 generator
+  line:267: GENERATE(1, 2)


Unfortunately, we can't print the variable name the generator value is assigned to, like in CAPTURE. This is why I decided to print the GENERATE call including the arguments. Since this can be quite long, I put the value into a separate line. Another option would be to elide long descriptions in the middle, e.g. GENERATE(values{1, 2, ..., 10 }).
We could also offer an optional user description, but this would probably need a dedicated macro to not introduce a breaking change. Alternatively, something like GENERATE(1, 2) << "UserDescription" could work.

gotschmarcel · 2022-08-28T21:27:28Z

src/catch2/generators/catch_generators.hpp

@@ -219,6 +220,7 @@ namespace Detail {
        }

        auto const& generator = static_cast<IGenerator<UnderlyingType> const&>( *tracker.getGenerator() );
+        getResultCapture().trackGeneratorState(GeneratorInfo(generatorName, argumentsDescription, lineInfo, generator.currentElementAsString()));


I was wondering if there's maybe a "cheaper" or better way to only get the generator states, if the test fails.

gotschmarcel · 2022-08-28T21:29:12Z

src/catch2/internal/catch_run_context.cpp

@@ -319,6 +319,16 @@ namespace Catch {
        return tracker;
    }

+    void RunContext::trackGeneratorState( GeneratorInfo const& info ) {
+        // Avoid redundant entries, in case a generator is used within a loop.
+        if ( std::find( m_generatorInfos.cbegin(),


Since info.name is already unique, we could use find_if based on the name only. Saves extra some comparisons.

gotschmarcel · 2022-10-01T09:06:15Z

@horenmar is there anything I can do to make this PR move forward? I'm happy to address change requests or fix tests. In case this implementation is approaching the problem in the completely wrong way, I'm also happy to decline this PR.

horenmar · 2022-10-04T14:09:23Z

I don't think this is a workable approach. The issue I have with it is that I don't see how it would support something like the random generator in a useful manner.

I will write up what I think is a better one later this week, but the jumping off point is a recently added generator feature: 48f3226, but there is also an issue with this approach: #2453, that I am not 100% sure on the resolution-of.

horenmar · 2022-10-07T20:09:03Z

My full thoughts about this:

The description has to be dynamic, to properly capture the current output element from the generator.
The description has to happen on-demand, so that we do not pay stringification when the reporter does not use it. There is some downside to this with multiple reporter potentially each paying the cost, but that should be relatively rare, and if it comes down to it, we can cache the stringified elements.
This means that the basic implementation has to wrap pointers to generators that are currently active in the tracker tree, and the reporters can retrieve the string values from them. However, we should not expose the tracker tree to reporters directly, because that is waaay too low level as an interface for that.
I am not decided whether the interface exposed to reporters should be done via free function getActiveGenerators, which would return limited handles to all the currently active generators (and their relative ordering), or by extending AssertionStats. Both have some pros and cons.

gotschmarcel · 2022-10-10T13:56:50Z

Thanks for putting some thought into this! I already assumed that currentElementAsString was designed for this purpose, which is why I based my first version already on this. I'm not sure what you mean by "the description has to be dynamic, [...]." Do you mean something beyond currentElementAsString?

Good point on doing this all on demand. I was wondering the same

I guess the problem with extending AssertionStats is that we would always need to provide something there, even if the assertion succeeded and some reporters wouldn't benefit from it. It does offer a clean interface though to pass encapsulated information to the reporter. Maybe it would be ok to provide a function to fetch the states as part of AssertionStats.

Do you think it's worth for me to further look into this? I'm happy to put more effort into this feature and help you out with it, if you believe it makes sense.

horenmar · 2022-10-10T19:48:29Z

Right, sorry. I initially only skimmed the PR, and when I found out that you are now taking string of the args to GENERATE, I assumed you do that to retrieve that for the reporters. I read it fully again this time, and turns out I was wrong in that assumption.

The cost to extending AssertionStats is relatively low. If it contains something relatively like std::vector<GeneratorWrapper>, where GeneratorWrapper is a wrapper over a single pointer to the generator, the cost is only 1 allocation per AssertionStats, if there are any active generators. This is generally low enough that we don't mind the cost.

The other option is to add getActiveGenerators function to AssertionStats, that retrieves the generators on-demand. The advantage is that this will not add an extra allocation if the reporter does not ask for the generators, the downside is that if multiple reporters ask, we pay multiple allocations... or we have to introduce caching and mutable and overall the maintenance burden is unlikely to be worth the saved alloc.

In either case GeneratorWrapper should provide only a very limited access to the generator interface, retrieving the current element index, and asking for current element string.

Also I think that the default action of a reporter should be to just use generator indices, and there being an option to get the full element strings. The full element strings are likely to be quite noisy in practice, so they should not be used unless the user asks for them.

gotschmarcel force-pushed the report_generator_state_on_failure branch from 09c6bd4 to b45b106 Compare August 27, 2022 15:26

gotschmarcel commented Aug 27, 2022

View reviewed changes

gotschmarcel commented Aug 28, 2022

View reviewed changes

src/catch2/interfaces/catch_interfaces_capture.hpp Outdated Show resolved Hide resolved

gotschmarcel commented Aug 28, 2022

View reviewed changes

gotschmarcel added 4 commits October 1, 2022 13:35

Report generator states for failed assertions

4c0a291

fixup! Report generator states for failed assertions

48a5431

fixup! Report generator states for failed assertions

3370452

fixup! Report generator states for failed assertions

a83fbae

gotschmarcel force-pushed the report_generator_state_on_failure branch from 7760d2e to a83fbae Compare October 1, 2022 11:35

horenmar added Feature Request Feature and removed Feature Request labels Oct 4, 2022

horenmar mentioned this pull request Oct 8, 2022

Generator::currentElementAsString breaks compilation of a lot of our tests #2453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report generator states for failed assertions #2509

Report generator states for failed assertions #2509

gotschmarcel commented Aug 27, 2022 •

edited

gotschmarcel Aug 27, 2022

gotschmarcel Aug 27, 2022

gotschmarcel Aug 27, 2022

gotschmarcel Aug 27, 2022

gotschmarcel Aug 27, 2022

gotschmarcel Aug 28, 2022 •

edited

gotschmarcel Aug 28, 2022

gotschmarcel Aug 28, 2022

gotschmarcel commented Oct 1, 2022

horenmar commented Oct 4, 2022

horenmar commented Oct 7, 2022

gotschmarcel commented Oct 10, 2022 •

edited

horenmar commented Oct 10, 2022 •

edited

Report generator states for failed assertions #2509

Are you sure you want to change the base?

Report generator states for failed assertions #2509

Conversation

gotschmarcel commented Aug 27, 2022 • edited

Description

GitHub Issues

gotschmarcel Aug 27, 2022

Choose a reason for hiding this comment

gotschmarcel Aug 27, 2022

Choose a reason for hiding this comment

gotschmarcel Aug 27, 2022

Choose a reason for hiding this comment

gotschmarcel Aug 27, 2022

Choose a reason for hiding this comment

gotschmarcel Aug 27, 2022

Choose a reason for hiding this comment

gotschmarcel Aug 28, 2022 • edited

Choose a reason for hiding this comment

gotschmarcel Aug 28, 2022

Choose a reason for hiding this comment

gotschmarcel Aug 28, 2022

Choose a reason for hiding this comment

gotschmarcel commented Oct 1, 2022

horenmar commented Oct 4, 2022

horenmar commented Oct 7, 2022

gotschmarcel commented Oct 10, 2022 • edited

horenmar commented Oct 10, 2022 • edited

gotschmarcel commented Aug 27, 2022 •

edited

gotschmarcel Aug 28, 2022 •

edited

gotschmarcel commented Oct 10, 2022 •

edited

horenmar commented Oct 10, 2022 •

edited