Unscoped info feature

[ozars]

Description

Add assertion scoped information messaging functionality. Newly introduced ASSERTION_INFO macro stores messages kept until next assertion expression, that is, they are cleared right until after assertionEnd every time. This enables passing info message across helper functions in assertion scope, which is not allowed currently due to local scope of INFO macro.

If this looks reasonable, I will go ahead and add complementary SECTION_INFO and TEST_CASE_INFO macros following the same logic for sections and test cases respectively.

I've added a few test cases and updated approval tests accordingly. Please verify correctness of approval tests. They looked good to me.

GitHub Issues

Related to #415, #983.

[codecov]

Codecov Report

Merging #1522 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1522      +/-   ##
==========================================
+ Coverage   80.58%   80.61%   +0.03%     
==========================================
  Files         121      121              
  Lines        3383     3389       +6     
==========================================
+ Hits         2726     2732       +6     
  Misses        657      657

[codecov]

Codecov Report

Merging #1522 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1522      +/-   ##
==========================================
+ Coverage   80.55%   80.63%   +0.07%     
==========================================
  Files         121      121              
  Lines        3389     3391       +2     
==========================================
+ Hits         2730     2734       +4     
+ Misses        659      657       -2

[ozars]

I modified PR per #983 (comment).

It seems to work as intended, however, WARN/FAIL/FAIL_CHECK/SUCCEED also clears unscoped info messages since they all eventually call RunContext::assertionEnded. I am not sure if this should be expected behavior. It makes sense to me to disable clearing on WARN and keep it for other three. Suggestions?

[JoeyGrajciar]

FAIL, FAIL_CHECK, SUCCEED and WARN are basically assertions. First three are to explicitly end the test case. WARN is special one which does not explicitly end the run but continues on failure. I guess that WARN was made like that to display the message even if the test passes. Is it really needed to have those unscoped info messages kept even after WARN?

[JoeyGrajciar]

Please add also documentation about this macro to docs/logging.md. Also it might be good to add an example to the examples. If you add the example add link to it in the documentation.

[ozars]

Is it really needed to have those unscoped info messages kept even after WARN?

Not really. I mixed two things. I'd prefer to have a variation of ordinary info messages that are not locally scoped (a variation of unscoped info messages not reset at each assertion in other words) so that it would behave exactly like INFO, but it could be created from helper functions different from INFO. Unscoped info doesn't behave this way by design. I understand it may be cumbersome to maintain many variations of info, so I am not pushing for this to be maintained officially.

I think "unscoped info" is somewhat a counter-intuitive name for something scoped to assertions though. I see where you are coming from, that unscoped info is something existed before, but you may consider renaming before reintroducing it given that "info" isn't also referred as "scoped info" any more. Something like "assertive info" or "assertion info" sounds more intuitive to me, but then again I never used unscoped info when it existed (I recently started using Catch2).

Please add also documentation about this macro to docs/logging.md. Also it might be good to add an example to the examples. If you add the example add link to it in the documentation.

Sure. I will add some documentation. I didn't see any stand alone examples just for logging messages, so I plan to add some inline examples to the documentation quoting from test cases.

[ozars]

@JoeyGrajciar Please check the documentation I added.

Do you want me to squash everything or leave it as-is?

[JoeyGrajciar]

These make this macro is about differences to INFO? Probably it might be written like These differences make this macro or That makes this macro

[ozars]

Yes, it refers to properties explained just above. It sounds okay to me, but feel free to rephrase it and push.

[horenmar]

I am back, gonna take a look on this.

[horenmar]

I had a hell of a time reasoning through this, and, apart from some cosmetic things that I can just push by myself, I have two remarks:

• I am not sure that unscoped messages should be cleared at the end of a SECTION. IMO, an unscoped message that is unused by the end of a section is the result of an ad-hoc reimplementation of generators with sections:

TEST_CASE("foo") {
    int how_many = 0;
    SECTION("3 elements") { how_many = 3; }
    SECTION("5 elements") { how_many = 5; }
    ...
}

As an example, our current codebase actually has multiple instances of this pattern.

• Under current implementation, WARN will also remove all UNSCOPED_INFOs preceding it, because it is internally implemented as an assertion with specific property set. I don't think this is optimal, and instead I would suggest resurrecting the logic using IStreamingReporter::assertionEnded return value, see catch_interfaces_reporter.h lines 193-194, and catch_run_context.cpp lines 146-168.

[ozars]

I am not sure that unscoped messages should be cleared at the end of a SECTION. IMO, an unscoped message that is unused by the end of a section is the result of an ad-hoc reimplementation of generators with sections:

TEST_CASE("foo") {
    int how_many = 0;
    SECTION("3 elements") { how_many = 3; }
    SECTION("5 elements") { how_many = 5; }
    ...
}

I'm not sure if I understood this use case correctly. What would be the use of SECTION without any assertions in it? Also wouldn't it be semantically more reasonable to put unscoped info in between two sections instead of just before the end of the first section if one wants some info to be printed for the second section?

I agree with WARN case. I will update accordingly.

[horenmar]

Multiple SECTIONs were a way to simulate generators before Catch's generators were finished. The idea is that only one sibling section is run at a time, so it will set a new value for the variable in each run, so the complicated assertion code can be shared. That's not overly important though.

After some deliberation, I think I agree with just fixing WARN for now, because there are even more places that need some rework, e.g. the logic in RunContext::handleExpr for short-circuiting would need to handle this properly -- in the end it is probably better to get the feature in now and rewrite it later.

[ozars]

Okay, I got what you meant with sections above. I will try removing message scope clear at the end of each section, but this change alone probably won't be enough if test cases are ran multiple times to iterate over sections and there are other assertions on that path before reaching the next section, which is quite likely. In this case, it may be hard to implement this without serious refactoring or ugly workarounds.

e.g. the logic in RunContext::handleExpr for short-circuiting

Good thing you brought this up. I think I need to add another check against WARN in RunContext::assertionPassed for the same reason. Also just so you know, I found out by chance that current tests don't cover the execution path for that short-circuit. See the comment in above commit 093b3b8.

I am going to make above changes, update docs and rebase it against master, hopefully sometime this weekend.

[horenmar]

I added some cosmetic changes that you can just squash down.

[ozars]

I will try removing message scope clear at the end of each section

This solution didn't work out as I expected unfortunately. I don't have much idea about how to implement it without major changes either. So, I will depend this behavior being not overly important.

I think I need to add another check against WARN in RunContext::assertionPassed for the same reason.

It turns out this was being handled correctly already.

I splitted commits into three parts. If there are no other changes requested, it is ready to merge.

ps. I rebased it against master. I've just realized some tests are failing on appveyor due to some unrelated changes inherited from the tip of the master.

[horenmar]

Yeah, sorry about that. For some reason, the tests are run in a slightly different order on Windows than they are run on Linux. I'll need to look into it, but didn't have the time just yet.

[ozars]

No problem at all. I doesn't affect me. I just wanted to note it since PR fails to pass appveyor checks.

[horenmar]

Merged, thanks.

[ozars]

Thanks.