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

Add TEMPLATE_TEST_CASE_n macros #382

Closed
wants to merge 1 commit into from
Closed

Add TEMPLATE_TEST_CASE_n macros #382

wants to merge 1 commit into from

Conversation

garethsb-ghost
Copy link

Add TEMPLATE_TEST_CASE_n macros to meet the requirements of type-parameterised tests via test case templates. This PR is based on a sketch described in #46 and discussed in #357. The sketch implementation was also adapted and tested by @wichtounet.

This is my first use of GitHub so please forgive and let me know any mistakes I've made in the PR procedure!

Issues with the PR as it stands:

  1. No documentation.
  2. No unit tests.
  3. Lack of support for CATCH_CONFIG_VARIADIC_MACROS in implementation of TEMPLATE_TEST_CASE_n because the parameters name, description precede the type list. Not sure if this matters or not.
  4. The decision to provide macros with the length of the type list in the name is a little inelegant, though it does mean no additional machinery for working with type lists (e.g. Boost.MPL) is required.
  5. The type list length is currently arbitrarily limited to 8, but that could easily be increased (though rather more tersely, if machinery like Boost.Preprocessor BOOST_PP_REPEAT were used).
  6. This approach wraps up all the instantiations of the test case template as sections into a single test case. I like this as it means the reported number of test cases doesn't go up when I add more types to the list, but another way of doing this would be to provide the macro to define the test case template and a second macro to instantiate a template test case from that test case template for a particular type (or test cases from a list of types), if you see what I mean.

@martinmoene
Copy link
Collaborator

I expect that PRs should be done using branch develop.

@garethsb-ghost
Copy link
Author

Thanks @martinmoene, I wondered about that, it wasn't completely clear from the GitHub article about using pull requests. I guess I should probably have created a topic branch in my fork too. Anyway, before proceeding further I'd like to know whether a PR for this feature is likely ultimately to be accepted. Best regards,

@martinmoene
Copy link
Collaborator

@garethsb, Agree to await Phil's reaction first ;)
Cheers, Martin

@philsquared
Copy link
Collaborator

Hi @garethsb,

Sorry for the radio silence. I've been away from this project for a while as I've got a couple of major things on (again!).
I'm definitely interested in this area in general and will give your PR due consideration. However I don't really have the time right now to give it the time it deserves - probably in a couple of weeks or so.
As you may have seen I've merged develop back into master since you submitted this - although I'll be keeping develop around for this sort of integration. The point is that GitHub thinks your PR can be merged with master as it is - and develop is now essentially the same so we should be able to work it out.

@garethsb-ghost
Copy link
Author

Thanks, @philsquared, I understand. I'm a git/github newbie, but if I understand correctly, github doesn't allow me to fix this PR to change the base branch to develop rather than master or change the head from my fork's master branch to a topic branch... I'd have to create a new PR... so I'll just leave it alone, until whenever you have a chance to review. Thanks again.

@dcoeurjo
Copy link

Hi all, I've tested @garethsb 's PR in my project and it works perfectly. I'd love to see this feature included in the Catch master...

@ghost
Copy link

ghost commented Sep 22, 2015

Here is another alternative for the templated "fixture".
Please consider adding it too.

#define INTERNAL_CATCH_TEST_CASE_METHOD_TEMPLATE_DEFINE( TemplateClassName, Scope )\
    namespace { \
        struct INSTANCE_CATCH_##Scope { \
            template<typename TestType> \
            struct TemplateTest : TemplateClassName<TestType> { \
                void test(); \
            }; \
        }; \
    } \
    template<typename TestType> \
    void INSTANCE_CATCH_##Scope::TemplateTest<TestType>::test()


#define INTERNAL_CATCH_TEST_CASE_METHOD_TEMPLATE_REGISTER( Scope, TypeInstance, name, desc )\
    Catch::AutoReg INTERNAL_CATCH_UNIQUE_NAME( autoRegistrar ) ( &INSTANCE_CATCH_##Scope::TemplateTest<TypeInstance>::test, #Scope "<" #TypeInstance ">", Catch::NameAndDesc( #name " <" #TypeInstance ">" , #desc ), CATCH_INTERNAL_LINEINFO );

#define CATCH_TEST_CASE_METHOD_TEMPLATE_DEFINE_SCOPE( TemplateClassName, Scope )    INTERNAL_CATCH_TEST_CASE_METHOD_TEMPLATE_DEFINE( TemplateClassName, Scope )
#define CATCH_TEST_CASE_METHOD_TEMPLATE_DEFINE( TemplateClassName )                 INTERNAL_CATCH_TEST_CASE_METHOD_TEMPLATE_DEFINE( TemplateClassName, TemplateClassName )
#define CATCH_TEST_CASE_METHOD_TEMPLATE_REGISTER( Scope, T, name, Desc )            INTERNAL_CATCH_TEST_CASE_METHOD_TEMPLATE_REGISTER( Scope, T, name, Desc )

Usage:

CATCH_TEST_CASE_METHOD_TEMPLATE_DEFINE( MyTest ) { 
TestType t;
...
}
CATCH_TEST_CASE_METHOD_TEMPLATE_REGISTER( MyTest, float, "Geometry predicates - float", "[geometry]" )
CATCH_TEST_CASE_METHOD_TEMPLATE_REGISTER( MyTest, double, "Geometry predicates - double", "[geometry]" )

@garethsb-ghost
Copy link
Author

Thanks, @dcoeurjo, good to hear it works for you.

I also like @zpgaal 's approach, basically as I described in last point of my original PR comment. Some thoughts:
It has possible pros, e.g. less new code in Catch (even when support for no CATCH_ prefix is added), and no explicit or maximum supported type-list length. Possible cons are that it's more verbose for the user and that the _REGISTER 'calls' must follow the 'test case template' body I believe. Possibly the 'test case template' declaration could go first, followed by registration (instantiation), followed by the definition, but that'd be more long-winded again. I'd need this though, for my own shim layer that abstracts whether I'm using Catch, Google Test or Boost.Test. Whether the fact that each type-under-test makes a separate test case rather than a section in one test case could be a pro or a con I guess. One other small decision to make either way, is whether the name of the type-under-test is hard-coded (TestType here) or able to be varied by the test case author as in the PR (I find myself using this for clarity).

@ghost
Copy link

ghost commented Sep 27, 2015

Unfortunatelly my soultion has some other drawback (and all derivation based alternatives). Using gcc and other similar compilers, typedef defined within the fixture cannot be access directly from the test function.

template
struct B { typedef int How_To_Acess_This_From_A_t; };

template
struct A : B { test() {...} };

@akosenkov
Copy link

How about using this trick to get rid of the trailing _n in TEMPLATE_TEST_CASE?
http://efesx.com/2010/07/17/variadic-macro-to-count-number-of-arguments/

@nicola-gigante
Copy link

@garethsb Hi, I'm reading at the code and I don't understand how it is supposed to work.

You define this macro and the corresponding "DEFN" one:

#define INTERNAL_CATCH_TEST_CASE_TEMPLATE_DECL( T ) \
 template<typename T> \
 static void INTERNAL_CATCH_UNIQUE_NAME( ____C_A_T_C_H____T_E_M_P_L_A_TE____T_E_S_T____ )();

At the end, INTERNAL_CATCH_UNIQUE_NAME will be expanded three times and you should get three different names. What am I missing?

@garethsb
Copy link
Contributor

@nicola-gigante The technique relies on the fact that those two INTERNAL macros are ultimately expanded on the same source line so that __LINE__ evaluates the same in both cases. There are several existing Catch macros that also use repeated instances of INTERNAL_CATCH_UNIQUE_NAME to produce the same name a number of times.

The code of this PR, plus some other small Catch extensions, is in a self-contained Catch/Google Test/Boost Test single-header shim being used e.g. here: https://github.com/sony/nmos-cpp/blob/master/Development/bst/test/test.h

Best regards,
Gareth

@horenmar
Copy link
Member

There are several existing Catch macros that also use repeated instances of INTERNAL_CATCH_UNIQUE_NAME to produce the same name a number of times.

@garethsb-sony As far as I know, we don't and it wouldn't work as the use of __COUNTER__ is strongly preferred to use of __LINE__ when constructing unique names. What we do is to have an indirection macro, that evaluates INTERNAL_CATCH_UNIQUE_NAME and passes it forward.

I have no idea what the state of that was back in 2015 though

@garethsb-ghost
Copy link
Author

@horenmar As you say, the code has moved on. The state of the repo in 2015 shows INTERNAL_CATCH_TESTCASE etc. as I described: internal/catch_test_registry.hpp in 2015. The equivalent code in 2017 uses an indirection macro: internal/catch_test_registry.hpp in 2017. It wouldn't take much to update this PR, if it were likely to be merged.

@garethsb
Copy link
Contributor

I updated the [CATCH_]TEMPLATE_TEST_CASE_n macros work with Catch 1.10.0. I've pushed this to the repo at https://github.com/sony/nmos-cpp/blob/7c1ed9dc/Development/bst/test/detail/catch-1.10.0.h#L74.
I won't update this PR unless a Catch maintainer wants it.

@JoeyGrajciar
Copy link
Contributor

I think this can be closed since we have TEMPLATE_TEST_CASE and TEMPLATE_PRODUCT_TEST_CASE.

@horenmar
Copy link
Member

horenmar commented Jan 4, 2019

Agreed

@horenmar horenmar closed this Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants