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.

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

Report generator states for failed assertions #2509

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

gotschmarcel
Copy link

@gotschmarcel gotschmarcel commented Aug 27, 2022

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

@gotschmarcel gotschmarcel force-pushed the report_generator_state_on_failure branch from 09c6bd4 to b45b106 Compare August 27, 2022 15:26
src/catch2/interfaces/catch_interfaces_reporter.cpp Outdated Show resolved Hide resolved
@@ -8,10 +8,12 @@
#ifndef CATCH_INTERFACES_REPORTER_HPP_INCLUDED
#define CATCH_INTERFACES_REPORTER_HPP_INCLUDED

#include "catch2/interfaces/catch_interfaces_capture.hpp"
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/catch2/internal/catch_run_context.hpp Outdated Show resolved Hide resolved
return;
}

stream << "with " << pluralise(stats.generatorInfos.size(), "generator"_sr) << "\n";
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@@ -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.
Copy link
Author

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Author

@gotschmarcel gotschmarcel Aug 28, 2022

Choose a reason for hiding this comment

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

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.

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

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

Choose a reason for hiding this comment

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

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

@@ -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(),
Copy link
Author

Choose a reason for hiding this comment

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

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

@gotschmarcel
Copy link
Author

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

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

horenmar commented Oct 4, 2022

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
Copy link
Member

horenmar commented Oct 7, 2022

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
Copy link
Author

gotschmarcel commented Oct 10, 2022

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
Copy link
Member

horenmar commented Oct 10, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants