Catch2 icon indicating copy to clipboard operation
Catch2 copied to clipboard

Add THEN_REQUIRE and AND_REQUIRE macros for shorter more explicit code

Open Anthony-Nicholls opened this issue 4 years ago • 5 comments

I often find that it's best practice to have very few (normally one) REQUIRE for each THEN statement when using BBD style so it's very clear what has failed. As a result I believe combining these macros would be very useful. I've mocked up the idea and ran it locally, I've no doubt that there may very well be things I've missed.

Goals

  • For each REQUIRE macro there should be a THEN_REQUIRE equivalent.
  • For each REQUIRE macro there should be an AND_REQUIRE equivalent.
  • The first argument to the THEN_REQUIRE and AND_REQUIRE macros should be the description that would otherwise be passed to the THEN and AND_THEN macros, followed by the remaining arguments for the relevant REQUIRE macro.
  • Each macro should support being completed using a semicolon to make it functional like.

Below is the vector example from the documentation rewritten using the THEN_REQUIRE macro. It's less lines, more explicit, and less nested.

SCENARIO( "vectors can be sized and resized", "[vector]" ) {

    GIVEN( "A vector with some items" ) {
        std::vector<int> v( 5 );

        REQUIRE( v.size() == 5 );
        REQUIRE( v.capacity() >= 5 );

        WHEN( "the size is increased" ) {
            v.resize( 10 );

            THEN_REQUIRE( "the size increases", v.size() == 10 );
            THEN_REQUIRE( "the capacity increases", v.capacity() >= 10 );
        }
        WHEN( "the size is reduced" ) {
            v.resize( 0 );
            
            THEN_REQUIRE( "the size decreases", v.size() == 0 );
            THEN_REQUIRE( "the capacity remains unchanged", v.capacity() >= 5 );
        }
        WHEN( "more capacity is reserved" ) {
            v.reserve( 10 );
            
            THEN_REQUIRE( "the size remains unchanged", v.size() == 5 );
            THEN_REQUIRE( "the capacity increases", v.capacity() >= 10 );
        }
        WHEN( "less capacity is reserved" ) {
            v.reserve( 0 );
            
            THEN_REQUIRE( "the size remains unchanged", v.size() == 0 );
            THEN_REQUIRE( "the capacity remains unchanged", v.capacity() >= 5 );
        }
    }
}

I have a mock up that is working with the little testing I've done locally, below I've only included a few of the REQUIRE macros but the same principle applies for the others.

#define FORCE_SEMICOLON( code ) \
    do{ code } while( false )

#define COMBINE_SECTION_WITH_REQUIRE( sectionMacro, requireMacro, desc, ... ) \
    FORCE_SEMICOLON( sectionMacro( desc ) { requireMacro( __VA_ARGS__ ); } )

#define THEN_REQUIRE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( THEN, REQUIRE, desc, __VA_ARGS__ )

#define THEN_REQUIRE_FALSE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( THEN, REQUIRE_FALSE, desc, __VA_ARGS__ )

#define AND_REQUIRE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( AND_THEN, REQUIRE, desc, __VA_ARGS__ )

#define AND_REQUIRE_FALSE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( AND_THEN, REQUIRE_FALSE, desc, __VA_ARGS__ )

If we like this idea I'm happy to put together a pull request for this.


Stretch goal

This should probably be considered independently from the above!

In addition to the above I wanted to see if I could optionally allow the user to drop the semicolon in place of braces to allow nested AND_* macros.

I got the following working with come caveats (see below).

#define RUN_ONCE_WITH_NAME( code, name ) \
    for ( bool name = [&](){ code; return true; }(); name; name = false )

#define COMBINE_SECTION_WITH_REQUIRE( name, sectionMacro, requireMacro, desc, ... ) \
    sectionMacro( desc ) RUN_ONCE_WITH_NAME( requireMacro( __VA_ARGS__ ), name )

#define THEN_REQUIRE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( catch2ThenRequire, THEN, REQUIRE, desc, __VA_ARGS__ )

#define THEN_REQUIRE_FALSE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( catch2ThenRequire, THEN, REQUIRE_FALSE, desc, __VA_ARGS__ )

#define AND_REQUIRE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( catch2AndRequire, AND_THEN, REQUIRE, desc, __VA_ARGS__ )

#define AND_REQUIRE_FALSE( desc, ... ) \
    COMBINE_SECTION_WITH_REQUIRE( catch2AndRequire, AND_THEN, REQUIRE_FALSE, desc, __VA_ARGS__ )
  • The RUN_ONCE_WITH_NAME takes a name so that any with the same name in a nested scope (which can happen accidentally if the semicolon is forgotten!!) should cause at least a warning due to the shadowing of a variable (I suspect we could force the warning and make warnings as errors just for the scope we're interested in).
  • Unfortunately this means you can't nest AND_REQUIRE macros inside other AND_REQUIRE macros which is a limiting factor, however I thought it more important to prevent the code compiling when the semicolon is dropped.

Obviously this extension to the request could just be dropped in favour of what the library already offers.

Anthony-Nicholls avatar Aug 02 '21 19:08 Anthony-Nicholls

Should the AND_REQUIRE macro really be AND_THEN_REQUIRE? It's a bit cumbersome I know (especially if you also add AND_THEN_REQUIRE_FALSE) but AND_REQUIRE sounds more like it should come after a normal REQUIRE, which is not the case. Would be more consistent with other AND_ macros too. Just a thought!

ImJimmi avatar Aug 04 '21 09:08 ImJimmi

@ImJimmi thanks for the comment I did consider it but I don't think so because...

AND_THEN is really just AND i.e. the following code...


SCENARIO( "..." )
{
    GIVEN( "..." )
    {
        WHEN( "..." )
        {
            THEN( "..." )
            {
                AND_THEN( "..." )
                {
                }
            }
        } 
    }
}

Produces this output...

Scenario: ...
      Given: ...
       When: ...
       Then: ...
        And: ...

However, I suspect no one wanted to make a macro called AND for obvious reasons.

To be honest I would be fine with dropping all the AND_* alternatives in favour of just having the THEN_REQUIRE_* macros for now.

The other option would be all THEN_* macros knowing they are inside of other THEN_* macros and printing accordingly but that's probably over complicating the situation.

Anthony-Nicholls avatar Aug 04 '21 09:08 Anthony-Nicholls

Maybe the text should be "And then:" but that could be a breaking change now for anyone analysing the output (As Catch does itself in the approval tests).

Anthony-Nicholls avatar Aug 04 '21 09:08 Anthony-Nicholls

After reviewing the Gherkin documentation I think I'm going to drop the AND_REQUIRE macros.

Anthony-Nicholls avatar Aug 04 '21 18:08 Anthony-Nicholls

Have you thought of adding THEN_CHECK as well?

ImJimmi avatar Sep 12 '22 13:09 ImJimmi