Issue 229700044: Add support for named value-parameterized tests.

Billy Donahue

https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h File include/gtest/gtest-param-test.h (right): https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h#newcode1415 include/gtest/gtest-param-test.h:1415: nullptr, \ nullptr is a C++11 thing we can't ...

10 years, 8 months ago (2015-05-11 23:26:34 UTC) #1

wan

It would be great to discuss the design of this new API first. Can we ...

10 years, 8 months ago (2015-05-11 23:38:25 UTC) #2

Billy Donahue

100% yes. On Mon, May 11, 2015 at 7:38 PM, Zhanyong Wan (λx.x x) <wan@google.com> ...

10 years, 8 months ago (2015-05-11 23:45:21 UTC) #3

wan

Great. How about starting with a description of the proposed API, Jamie? Thanks, On Mon, ...

10 years, 8 months ago (2015-05-11 23:47:27 UTC) #4

Jamie Madill

Sure! The problem I'm solving is that my parameterized tests end up with non-descriptive names: ...

10 years, 8 months ago (2015-05-12 00:18:11 UTC) #5

Sure!

The problem I'm solving is that my parameterized tests end up with
non-descriptive names:

"D3D11.dEQP_GLES2/1" to D3D11.dEQP_GLES2/240320". I'd like to be able to
replace the number with the text of the test, like

"D3D11.dEQP_GLES2/functional_shaders_linkage_varyings_vec2_vec3"

My proposed solution is to add a new API,
'INSTANTIATE_NAMED_TEST_CASE_P(InstantiationName, TestCaseName, Generator,
NameGenerator)"

This macro is identical to INSTANTIATE_TEST_CASE_P except the addition of
the name generator parameter, which is a callback of the form "std::string
(func)(const ParamType&)", where ParamType is the parameter type of the
test case. For each parameter value, the named test will generate a name
for the test as "InstantiationName/TestCaseName.TestName/ParamName", where
ParamName is the value returned by the name generator for each parameter
value.

So, Billy asked if it was possible to achieve the same functionality
without a new macro. I'm not sure - would it be possible to pack name
generation into a new parameter generator? I'm open to any suggestions here.



On Mon, May 11, 2015 at 7:47 PM, Zhanyong Wan (λx.x x) <wan@google.com>
wrote:

> Great.  How about starting with a description of the proposed API, Jamie?
> Thanks,
>
> On Mon, May 11, 2015 at 4:45 PM, Billy Donahue <billydonahue@google.com>
> wrote:
>
>> 100% yes.
>>
>> On Mon, May 11, 2015 at 7:38 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>> wrote:
>>
>>> It would be great to discuss the design of this new API first.  Can we
>>> do that?  Thanks,
>>>
>>> On Mon, May 11, 2015 at 4:26 PM, billydonahue via Google C++ Testing
>>> Framework <googletestframework@googlegroups.com> wrote:
>>>
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h
>>>> File include/gtest/gtest-param-test.h (right):
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>> include/gtest/gtest-param-test.h:1415: nullptr, \
>>>> nullptr is a C++11 thing we can't allow yet.
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>> include/gtest/gtest-param-test.h:1418: # define
>>>> INSTANTIATE_NAMED_TEST_CASE_P(prefix, test_case_name, generator,
>>>> name_generator) \
>>>> Is there any way to avoid having a new macro?
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>> include/gtest/gtest-param-test.h:1429:
>>>> lose one of the blank lines.
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>> File include/gtest/internal/gtest-param-util.h (right):
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>> include/gtest/internal/gtest-param-util.h:512: test_name_stream <<
>>>> name_func(*param_it);
>>>> Maybe default the name_func to something that returns 'i' as a string.
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>> include/gtest/internal/gtest-param-util.h:546: // Keeps triples of
>>>> <Instantiation name, Sequence generator creation function, Name
>>>> generator function>
>>>> reformat to 80-cols.
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>> include/gtest/internal/gtest-param-util.h:551: ParamNameGeneratorFunc*
>>>> name_func) :
>>>> drop the ':' to the next line, indented by 4.
>>>>
>>>>
>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>> include/gtest/internal/gtest-param-util.h:556: std::string name_;
>>>> These shouldn't have underscores if they're public.
>>>>
>>>> https://codereview.appspot.com/229700044/
>>>>
>>>>
>>>> --
>>>>
>>>> ---You received this message because you are subscribed to the Google
>>>> Groups "Google C++ Testing Framework" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send
>>>> an email to googletestframework+unsubscribe@googlegroups.com.
>>>> For more options, visit https://groups.google.com/d/optout.
>>>>
>>>> --
>>>>
>>>>
>>>
>>>
>>> --
>>> Zhanyong
>>>
>>
>>
>
>
> --
> Zhanyong
>

Sign in to reply to this message.

wan

Thanks for the write-up, Jamie! This is very helpful. On Mon, May 11, 2015 at ...

10 years, 8 months ago (2015-05-12 02:58:46 UTC) #6

Thanks for the write-up, Jamie!  This is very helpful.

On Mon, May 11, 2015 at 5:17 PM, Jamie Madill <jmadill@chromium.org> wrote:

> Sure!
>
> The problem I'm solving is that my parameterized tests end up with
> non-descriptive names:
>
> "D3D11.dEQP_GLES2/1" to D3D11.dEQP_GLES2/240320".
>

Side notes:

- You should avoid _ in test names.  See
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...
for why.

- Do you really generate 240320 copies of the parameterized tests?  That's
a *lot*.  Exhaustive testing often is not the best way to use your
resources.  You may be able to achieve the same level of confidence with a
much smaller number of test parameters.

I'd like to be able to replace the number with the text of the test, like
>
> "D3D11.dEQP_GLES2/functional_shaders_linkage_varyings_vec2_vec3"
>

This looks reasonable to me.

>
> My proposed solution is to add a new API,
> 'INSTANTIATE_NAMED_TEST_CASE_P(InstantiationName, TestCaseName, Generator,
> NameGenerator)"
>
> This macro is identical to INSTANTIATE_TEST_CASE_P except the addition of
> the name generator parameter, which is a callback of the form "std::string
> (func)(const ParamType&)", where ParamType is the parameter type of the
> test case. For each parameter value, the named test will generate a name
> for the test as "InstantiationName/TestCaseName.TestName/ParamName", where
> ParamName is the value returned by the name generator for each parameter
> value.
>
> So, Billy asked if it was possible to achieve the same functionality
> without a new macro. I'm not sure - would it be possible to pack name
> generation into a new parameter generator? I'm open to any suggestions here.
>

I understand Billy's aversion to macros.  It is best if we can achieve your
goal without adding a new macro.  I can see two approaches that can work:

1. Changing INSTANTIATE_TEST_CASE_P() to a variadic macro, s.t. it can
accept a name generator optionally.

2. Allowing the generator argument of INSTANTIATE_TEST_CASE_P() to return
either a test parameter or a NamedParameter<T>, which is a struct/class
that holds a string name and a test parameter value.

I'm inclined to #1, as it allows the parameter-generating logic and the
name-generating logic to be reused separately.  (For example, the same name
generator can be used with different parameter generators.)

There are other goals I think the design should achieve:

- We should define what constitutes a valid name (e.g. what character are
allowed).  On top of my head, I'd say we should disallow '/', which is
already used as the separator between the base test name and the parameter
name.  We'd also want to avoid any unprintable characters, for obvious
reasons.  Since the test names will be used in the --gtest_filter
commandline flag, we probably want to avoid characters that have specially
meanings on the commandline (e.g. parentheses, ?, *, etc).  To be safe, we
may start with just allowing _, English letters, or digits.

- gtest should report an error if a name generator returns an invalid name,
or if it returns the same name for two parameters.

- The new INSTANTIATE_TEST_CASE_P() should be general enough to support a
wide range of use cases.  In particular, it should be able to express the
original INSTANTIATE_TEST_CASE_P() as a special case.  This means that a
callback of the form "std::string (func)(const ParamType&)" is inadequate,
as it doesn't provide the generator with the index of the parameter.

One way to fix this may be letting the name generator function take the
parameter index as part of the input.  For extensibility, I'd use a
struct/class as the input argument type instead of passing the index and
the parameter separately.

Another way to fix it is to pass all parameters as a collection to the
generator function and have it return a vector of names, one for each
parameter in the input.  This is more general than the previous fix.  For
example, it allows the generator to look at all parameters and then decide
on a naming scheme that's globally optimal.  Not sure if we need this kind
of generality though.

- Instead of a function pointer, I may allow the generator to be an
arbitrary functor.  This way it may have local state if needed, and may
have a templated operator() that allows the same generator to be reused for
different types of parameters.

- We don't have to do it now, but we should eventually extend the
name-generating ability to TYPED_TEST() and TYPED_TEST_CASE_P(), for
consistency.  We need to make sure that the design we use for
INSTANTIATE_TEST_CASE_P() can naturally be extended to typed tests and
type-parameterized tests, with a similar look-and-feel.



>
>
> On Mon, May 11, 2015 at 7:47 PM, Zhanyong Wan (λx.x x) <wan@google.com>
> wrote:
>
>> Great.  How about starting with a description of the proposed API,
>> Jamie?  Thanks,
>>
>> On Mon, May 11, 2015 at 4:45 PM, Billy Donahue <billydonahue@google.com>
>> wrote:
>>
>>> 100% yes.
>>>
>>> On Mon, May 11, 2015 at 7:38 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>>> wrote:
>>>
>>>> It would be great to discuss the design of this new API first.  Can we
>>>> do that?  Thanks,
>>>>
>>>> On Mon, May 11, 2015 at 4:26 PM, billydonahue via Google C++ Testing
>>>> Framework <googletestframework@googlegroups.com> wrote:
>>>>
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h
>>>>> File include/gtest/gtest-param-test.h (right):
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>> include/gtest/gtest-param-test.h:1415: nullptr, \
>>>>> nullptr is a C++11 thing we can't allow yet.
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>> include/gtest/gtest-param-test.h:1418: # define
>>>>> INSTANTIATE_NAMED_TEST_CASE_P(prefix, test_case_name, generator,
>>>>> name_generator) \
>>>>> Is there any way to avoid having a new macro?
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>> include/gtest/gtest-param-test.h:1429:
>>>>> lose one of the blank lines.
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>> File include/gtest/internal/gtest-param-util.h (right):
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>> include/gtest/internal/gtest-param-util.h:512: test_name_stream <<
>>>>> name_func(*param_it);
>>>>> Maybe default the name_func to something that returns 'i' as a string.
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>> include/gtest/internal/gtest-param-util.h:546: // Keeps triples of
>>>>> <Instantiation name, Sequence generator creation function, Name
>>>>> generator function>
>>>>> reformat to 80-cols.
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>> include/gtest/internal/gtest-param-util.h:551: ParamNameGeneratorFunc*
>>>>> name_func) :
>>>>> drop the ':' to the next line, indented by 4.
>>>>>
>>>>>
>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>> include/gtest/internal/gtest-param-util.h:556: std::string name_;
>>>>> These shouldn't have underscores if they're public.
>>>>>
>>>>> https://codereview.appspot.com/229700044/
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> ---You received this message because you are subscribed to the Google
>>>>> Groups "Google C++ Testing Framework" group.
>>>>> To unsubscribe from this group and stop receiving emails from it, send
>>>>> an email to googletestframework+unsubscribe@googlegroups.com.
>>>>> For more options, visit https://groups.google.com/d/optout.
>>>>>
>>>>> --
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Zhanyong
>>>>
>>>
>>>
>>
>>
>> --
>> Zhanyong
>>
>
>


-- 
Zhanyong

Sign in to reply to this message.

Jamie Madill

One question regarding varadic macros.. really struggling there. On Mon, May 11, 2015 at 10:58 ...

10 years, 8 months ago (2015-05-12 14:32:33 UTC) #7

One question regarding varadic macros.. really struggling there.

On Mon, May 11, 2015 at 10:58 PM, Zhanyong Wan (λx.x x) <wan@google.com>
wrote:

> - You should avoid _ in test names.  See
>
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...
> for why.
>

OK, will keep that in mind.


> - Do you really generate 240320 copies of the parameterized tests?  That's
> a *lot*.  Exhaustive testing often is not the best way to use your
> resources.  You may be able to achieve the same level of confidence with a
> much smaller number of test parameters.
>

We're integrating a test package, go/dEQP <https://goto.google.com/dEQP>,
which has a couple modules with over 100k tests. The ES3 module I believe
has about 300k. We could group some of these tests into 'meta-tests' which
would reduce the number of 'gtest tests' by 10x or more, but we didn't
write these ourselves.


> I understand Billy's aversion to macros.  It is best if we can achieve
> your goal without adding a new macro.  I can see two approaches that can
> work:
>
> 1. Changing INSTANTIATE_TEST_CASE_P() to a variadic macro, s.t. it can
> accept a name generator optionally.
>
> 2. Allowing the generator argument of INSTANTIATE_TEST_CASE_P() to return
> either a test parameter or a NamedParameter<T>, which is a struct/class
> that holds a string name and a test parameter value.
>
> I'm inclined to #1, as it allows the parameter-generating logic and the
> name-generating logic to be reused separately.  (For example, the same name
> generator can be used with different parameter generators.)
>

Is there any option other than variadic macros that would work? I'm really
struggling with the complexities of implementing default macro arguments in
a standards-compliant way. I can upload what I have, but I just can't make
it work.


> There are other goals I think the design should achieve:
>
> - We should define what constitutes a valid name (e.g. what character are
> allowed).  On top of my head, I'd say we should disallow '/', which is
> already used as the separator between the base test name and the parameter
> name.  We'd also want to avoid any unprintable characters, for obvious
> reasons.  Since the test names will be used in the --gtest_filter
> commandline flag, we probably want to avoid characters that have specially
> meanings on the commandline (e.g. parentheses, ?, *, etc).  To be safe, we
> may start with just allowing _, English letters, or digits.
>

This seems good, done.


> - gtest should report an error if a name generator returns an invalid
> name, or if it returns the same name for two parameters.
>

It's ok to call GTEST_LOG_(ERROR) << msg for this?


> - The new INSTANTIATE_TEST_CASE_P() should be general enough to support a
> wide range of use cases.  In particular, it should be able to express the
> original INSTANTIATE_TEST_CASE_P() as a special case.  This means that a
> callback of the form "std::string (func)(const ParamType&)" is inadequate,
> as it doesn't provide the generator with the index of the parameter.
>
> One way to fix this may be letting the name generator function take the
> parameter index as part of the input.  For extensibility, I'd use a
> struct/class as the input argument type instead of passing the index and
> the parameter separately.
>

sounds good.


> Another way to fix it is to pass all parameters as a collection to the
> generator function and have it return a vector of names, one for each
> parameter in the input.  This is more general than the previous fix.  For
> example, it allows the generator to look at all parameters and then decide
> on a naming scheme that's globally optimal.  Not sure if we need this kind
> of generality though.
>
> - Instead of a function pointer, I may allow the generator to be an
> arbitrary functor.  This way it may have local state if needed, and may
> have a templated operator() that allows the same generator to be reused for
> different types of parameters.
>

I'll test this design locally to make sure it works.


> - We don't have to do it now, but we should eventually extend the
> name-generating ability to TYPED_TEST() and TYPED_TEST_CASE_P(), for
> consistency.  We need to make sure that the design we use for
> INSTANTIATE_TEST_CASE_P() can naturally be extended to typed tests and
> type-parameterized tests, with a similar look-and-feel.
>
>
>
>>
>>
>> On Mon, May 11, 2015 at 7:47 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>> wrote:
>>
>>> Great.  How about starting with a description of the proposed API,
>>> Jamie?  Thanks,
>>>
>>> On Mon, May 11, 2015 at 4:45 PM, Billy Donahue <billydonahue@google.com>
>>> wrote:
>>>
>>>> 100% yes.
>>>>
>>>> On Mon, May 11, 2015 at 7:38 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>>>> wrote:
>>>>
>>>>> It would be great to discuss the design of this new API first.  Can we
>>>>> do that?  Thanks,
>>>>>
>>>>> On Mon, May 11, 2015 at 4:26 PM, billydonahue via Google C++ Testing
>>>>> Framework <googletestframework@googlegroups.com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h
>>>>>> File include/gtest/gtest-param-test.h (right):
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>> include/gtest/gtest-param-test.h:1415: nullptr, \
>>>>>> nullptr is a C++11 thing we can't allow yet.
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>> include/gtest/gtest-param-test.h:1418: # define
>>>>>> INSTANTIATE_NAMED_TEST_CASE_P(prefix, test_case_name, generator,
>>>>>> name_generator) \
>>>>>> Is there any way to avoid having a new macro?
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>> include/gtest/gtest-param-test.h:1429:
>>>>>> lose one of the blank lines.
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>> File include/gtest/internal/gtest-param-util.h (right):
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>> include/gtest/internal/gtest-param-util.h:512: test_name_stream <<
>>>>>> name_func(*param_it);
>>>>>> Maybe default the name_func to something that returns 'i' as a string.
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>> include/gtest/internal/gtest-param-util.h:546: // Keeps triples of
>>>>>> <Instantiation name, Sequence generator creation function, Name
>>>>>> generator function>
>>>>>> reformat to 80-cols.
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>> include/gtest/internal/gtest-param-util.h:551: ParamNameGeneratorFunc*
>>>>>> name_func) :
>>>>>> drop the ':' to the next line, indented by 4.
>>>>>>
>>>>>>
>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>> include/gtest/internal/gtest-param-util.h:556: std::string name_;
>>>>>> These shouldn't have underscores if they're public.
>>>>>>
>>>>>> https://codereview.appspot.com/229700044/
>>>>>>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> ---You received this message because you are subscribed to the Google
>>>>>> Groups "Google C++ Testing Framework" group.
>>>>>> To unsubscribe from this group and stop receiving emails from it,
>>>>>> send an email to googletestframework+unsubscribe@googlegroups.com.
>>>>>> For more options, visit https://groups.google.com/d/optout.
>>>>>>
>>>>>> --
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Zhanyong
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Zhanyong
>>>
>>
>>
>
>
> --
> Zhanyong
>

Sign in to reply to this message.

wan

On Tue, May 12, 2015 at 7:32 AM, Jamie Madill <jmadill@chromium.org> wrote: > One question ...

10 years, 8 months ago (2015-05-12 16:39:24 UTC) #8

On Tue, May 12, 2015 at 7:32 AM, Jamie Madill <jmadill@chromium.org> wrote:

> One question regarding varadic macros.. really struggling there.
>
> On Mon, May 11, 2015 at 10:58 PM, Zhanyong Wan (λx.x x) <wan@google.com>
> wrote:
>
>> - You should avoid _ in test names.  See
>>
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...
>> for why.
>>
>
> OK, will keep that in mind.
>
>
>> - Do you really generate 240320 copies of the parameterized tests?
>> That's a *lot*.  Exhaustive testing often is not the best way to use
>> your resources.  You may be able to achieve the same level of confidence
>> with a much smaller number of test parameters.
>>
>
> We're integrating a test package, go/dEQP <https://goto.google.com/dEQP>,
> which has a couple modules with over 100k tests. The ES3 module I believe
> has about 300k. We could group some of these tests into 'meta-tests' which
> would reduce the number of 'gtest tests' by 10x or more, but we didn't
> write these ourselves.
>

I see.


>
>
>> I understand Billy's aversion to macros.  It is best if we can achieve
>> your goal without adding a new macro.  I can see two approaches that can
>> work:
>>
>> 1. Changing INSTANTIATE_TEST_CASE_P() to a variadic macro, s.t. it can
>> accept a name generator optionally.
>>
>> 2. Allowing the generator argument of INSTANTIATE_TEST_CASE_P() to return
>> either a test parameter or a NamedParameter<T>, which is a struct/class
>> that holds a string name and a test parameter value.
>>
>> I'm inclined to #1, as it allows the parameter-generating logic and the
>> name-generating logic to be reused separately.  (For example, the same name
>> generator can be used with different parameter generators.)
>>
>
> Is there any option other than variadic macros that would work? I'm really
> struggling with the complexities of implementing default macro arguments in
> a standards-compliant way. I can upload what I have, but I just can't make
> it work.
>

You don't have to simulate default macro arguments.  One way to do it is to
forward __VA_ARGS__ to an overloaded function.  If __VA_ARGS__ is empty,
the nullary version of the function will be invoked, which can return the
standard name generator that just uses the index as the name; if
__VA_ARGS__ is one argument, the unary version of the function will be
invoked.

>
>
>> There are other goals I think the design should achieve:
>>
>> - We should define what constitutes a valid name (e.g. what character are
>> allowed).  On top of my head, I'd say we should disallow '/', which is
>> already used as the separator between the base test name and the parameter
>> name.  We'd also want to avoid any unprintable characters, for obvious
>> reasons.  Since the test names will be used in the --gtest_filter
>> commandline flag, we probably want to avoid characters that have specially
>> meanings on the commandline (e.g. parentheses, ?, *, etc).  To be safe, we
>> may start with just allowing _, English letters, or digits.
>>
>
> This seems good, done.
>
>
>> - gtest should report an error if a name generator returns an invalid
>> name, or if it returns the same name for two parameters.
>>
>
> It's ok to call GTEST_LOG_(ERROR) << msg for this?
>

The error should cause the test program to fail.  IIRC, GTEST_LOG_(ERROR)
doesn't do that.


>
>
>> - The new INSTANTIATE_TEST_CASE_P() should be general enough to support a
>> wide range of use cases.  In particular, it should be able to express the
>> original INSTANTIATE_TEST_CASE_P() as a special case.  This means that a
>> callback of the form "std::string (func)(const ParamType&)" is inadequate,
>> as it doesn't provide the generator with the index of the parameter.
>>
>> One way to fix this may be letting the name generator function take the
>> parameter index as part of the input.  For extensibility, I'd use a
>> struct/class as the input argument type instead of passing the index and
>> the parameter separately.
>>
>
> sounds good.
>
>
>> Another way to fix it is to pass all parameters as a collection to the
>> generator function and have it return a vector of names, one for each
>> parameter in the input.  This is more general than the previous fix.  For
>> example, it allows the generator to look at all parameters and then decide
>> on a naming scheme that's globally optimal.  Not sure if we need this kind
>> of generality though.
>>
>> - Instead of a function pointer, I may allow the generator to be an
>> arbitrary functor.  This way it may have local state if needed, and may
>> have a templated operator() that allows the same generator to be reused for
>> different types of parameters.
>>
>
> I'll test this design locally to make sure it works.
>
>
>> - We don't have to do it now, but we should eventually extend the
>> name-generating ability to TYPED_TEST() and TYPED_TEST_CASE_P(), for
>> consistency.  We need to make sure that the design we use for
>> INSTANTIATE_TEST_CASE_P() can naturally be extended to typed tests and
>> type-parameterized tests, with a similar look-and-feel.
>>
>>
>>
>>>
>>>
>>> On Mon, May 11, 2015 at 7:47 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>>> wrote:
>>>
>>>> Great.  How about starting with a description of the proposed API,
>>>> Jamie?  Thanks,
>>>>
>>>> On Mon, May 11, 2015 at 4:45 PM, Billy Donahue <billydonahue@google.com
>>>> > wrote:
>>>>
>>>>> 100% yes.
>>>>>
>>>>> On Mon, May 11, 2015 at 7:38 PM, Zhanyong Wan (λx.x x) <wan@google.com
>>>>> > wrote:
>>>>>
>>>>>> It would be great to discuss the design of this new API first.  Can
>>>>>> we do that?  Thanks,
>>>>>>
>>>>>> On Mon, May 11, 2015 at 4:26 PM, billydonahue via Google C++ Testing
>>>>>> Framework <googletestframework@googlegroups.com> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h
>>>>>>> File include/gtest/gtest-param-test.h (right):
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>>> include/gtest/gtest-param-test.h:1415: nullptr, \
>>>>>>> nullptr is a C++11 thing we can't allow yet.
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>>> include/gtest/gtest-param-test.h:1418: # define
>>>>>>> INSTANTIATE_NAMED_TEST_CASE_P(prefix, test_case_name, generator,
>>>>>>> name_generator) \
>>>>>>> Is there any way to avoid having a new macro?
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>>> include/gtest/gtest-param-test.h:1429:
>>>>>>> lose one of the blank lines.
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>> File include/gtest/internal/gtest-param-util.h (right):
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>> include/gtest/internal/gtest-param-util.h:512: test_name_stream <<
>>>>>>> name_func(*param_it);
>>>>>>> Maybe default the name_func to something that returns 'i' as a
>>>>>>> string.
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>> include/gtest/internal/gtest-param-util.h:546: // Keeps triples of
>>>>>>> <Instantiation name, Sequence generator creation function, Name
>>>>>>> generator function>
>>>>>>> reformat to 80-cols.
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>> include/gtest/internal/gtest-param-util.h:551:
>>>>>>> ParamNameGeneratorFunc*
>>>>>>> name_func) :
>>>>>>> drop the ':' to the next line, indented by 4.
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>> include/gtest/internal/gtest-param-util.h:556: std::string name_;
>>>>>>> These shouldn't have underscores if they're public.
>>>>>>>
>>>>>>> https://codereview.appspot.com/229700044/
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> ---You received this message because you are subscribed to the
>>>>>>> Google Groups "Google C++ Testing Framework" group.
>>>>>>> To unsubscribe from this group and stop receiving emails from it,
>>>>>>> send an email to googletestframework+unsubscribe@googlegroups.com.
>>>>>>> For more options, visit https://groups.google.com/d/optout.
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Zhanyong
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Zhanyong
>>>>
>>>
>>>
>>
>>
>> --
>> Zhanyong
>>
>
>


-- 
Zhanyong

Sign in to reply to this message.

Jamie Madill

PTAL.. I'm sure there are some items that need to be cleaned up here. I ...

10 years, 8 months ago (2015-05-12 20:17:18 UTC) #9

PTAL.. I'm sure there are some items that need to be cleaned up here.

I used a std::set and fatal failures to report errors. The error messages
could be more descriptive if necessary. They make the program fail without
running tests, though the output is a bit odd.

On Tue, May 12, 2015 at 12:39 PM, Zhanyong Wan (λx.x x) <wan@google.com>
wrote:

>
>
> On Tue, May 12, 2015 at 7:32 AM, Jamie Madill <jmadill@chromium.org>
> wrote:
>
>> One question regarding varadic macros.. really struggling there.
>>
>> On Mon, May 11, 2015 at 10:58 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>> wrote:
>>
>>> - You should avoid _ in test names.  See
>>>
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...
>>> for why.
>>>
>>
>> OK, will keep that in mind.
>>
>>
>>> - Do you really generate 240320 copies of the parameterized tests?
>>> That's a *lot*.  Exhaustive testing often is not the best way to use
>>> your resources.  You may be able to achieve the same level of confidence
>>> with a much smaller number of test parameters.
>>>
>>
>> We're integrating a test package, go/dEQP <https://goto.google.com/dEQP>,
>> which has a couple modules with over 100k tests. The ES3 module I believe
>> has about 300k. We could group some of these tests into 'meta-tests' which
>> would reduce the number of 'gtest tests' by 10x or more, but we didn't
>> write these ourselves.
>>
>
> I see.
>
>
>>
>>
>>> I understand Billy's aversion to macros.  It is best if we can achieve
>>> your goal without adding a new macro.  I can see two approaches that can
>>> work:
>>>
>>> 1. Changing INSTANTIATE_TEST_CASE_P() to a variadic macro, s.t. it can
>>> accept a name generator optionally.
>>>
>>> 2. Allowing the generator argument of INSTANTIATE_TEST_CASE_P() to
>>> return either a test parameter or a NamedParameter<T>, which is a
>>> struct/class that holds a string name and a test parameter value.
>>>
>>> I'm inclined to #1, as it allows the parameter-generating logic and the
>>> name-generating logic to be reused separately.  (For example, the same name
>>> generator can be used with different parameter generators.)
>>>
>>
>> Is there any option other than variadic macros that would work? I'm
>> really struggling with the complexities of implementing default macro
>> arguments in a standards-compliant way. I can upload what I have, but I
>> just can't make it work.
>>
>
> You don't have to simulate default macro arguments.  One way to do it is
> to forward __VA_ARGS__ to an overloaded function.  If __VA_ARGS__ is empty,
> the nullary version of the function will be invoked, which can return the
> standard name generator that just uses the index as the name; if
> __VA_ARGS__ is one argument, the unary version of the function will be
> invoked.
>
>>
>>
>>> There are other goals I think the design should achieve:
>>>
>>> - We should define what constitutes a valid name (e.g. what character
>>> are allowed).  On top of my head, I'd say we should disallow '/', which is
>>> already used as the separator between the base test name and the parameter
>>> name.  We'd also want to avoid any unprintable characters, for obvious
>>> reasons.  Since the test names will be used in the --gtest_filter
>>> commandline flag, we probably want to avoid characters that have specially
>>> meanings on the commandline (e.g. parentheses, ?, *, etc).  To be safe, we
>>> may start with just allowing _, English letters, or digits.
>>>
>>
>> This seems good, done.
>>
>>
>>> - gtest should report an error if a name generator returns an invalid
>>> name, or if it returns the same name for two parameters.
>>>
>>
>> It's ok to call GTEST_LOG_(ERROR) << msg for this?
>>
>
> The error should cause the test program to fail.  IIRC, GTEST_LOG_(ERROR)
> doesn't do that.
>
>
>>
>>
>>> - The new INSTANTIATE_TEST_CASE_P() should be general enough to support
>>> a wide range of use cases.  In particular, it should be able to express the
>>> original INSTANTIATE_TEST_CASE_P() as a special case.  This means that a
>>> callback of the form "std::string (func)(const ParamType&)" is inadequate,
>>> as it doesn't provide the generator with the index of the parameter.
>>>
>>> One way to fix this may be letting the name generator function take the
>>> parameter index as part of the input.  For extensibility, I'd use a
>>> struct/class as the input argument type instead of passing the index and
>>> the parameter separately.
>>>
>>
>> sounds good.
>>
>>
>>> Another way to fix it is to pass all parameters as a collection to the
>>> generator function and have it return a vector of names, one for each
>>> parameter in the input.  This is more general than the previous fix.  For
>>> example, it allows the generator to look at all parameters and then decide
>>> on a naming scheme that's globally optimal.  Not sure if we need this kind
>>> of generality though.
>>>
>>> - Instead of a function pointer, I may allow the generator to be an
>>> arbitrary functor.  This way it may have local state if needed, and may
>>> have a templated operator() that allows the same generator to be reused for
>>> different types of parameters.
>>>
>>
>> I'll test this design locally to make sure it works.
>>
>>
>>> - We don't have to do it now, but we should eventually extend the
>>> name-generating ability to TYPED_TEST() and TYPED_TEST_CASE_P(), for
>>> consistency.  We need to make sure that the design we use for
>>> INSTANTIATE_TEST_CASE_P() can naturally be extended to typed tests and
>>> type-parameterized tests, with a similar look-and-feel.
>>>
>>>
>>>
>>>>
>>>>
>>>> On Mon, May 11, 2015 at 7:47 PM, Zhanyong Wan (λx.x x) <wan@google.com>
>>>> wrote:
>>>>
>>>>> Great.  How about starting with a description of the proposed API,
>>>>> Jamie?  Thanks,
>>>>>
>>>>> On Mon, May 11, 2015 at 4:45 PM, Billy Donahue <
>>>>> billydonahue@google.com> wrote:
>>>>>
>>>>>> 100% yes.
>>>>>>
>>>>>> On Mon, May 11, 2015 at 7:38 PM, Zhanyong Wan (λx.x x) <
>>>>>> wan@google.com> wrote:
>>>>>>
>>>>>>> It would be great to discuss the design of this new API first.  Can
>>>>>>> we do that?  Thanks,
>>>>>>>
>>>>>>> On Mon, May 11, 2015 at 4:26 PM, billydonahue via Google C++ Testing
>>>>>>> Framework <googletestframework@googlegroups.com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h
>>>>>>>> File include/gtest/gtest-param-test.h (right):
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>>>> include/gtest/gtest-param-test.h:1415: nullptr, \
>>>>>>>> nullptr is a C++11 thing we can't allow yet.
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>>>> include/gtest/gtest-param-test.h:1418: # define
>>>>>>>> INSTANTIATE_NAMED_TEST_CASE_P(prefix, test_case_name, generator,
>>>>>>>> name_generator) \
>>>>>>>> Is there any way to avoid having a new macro?
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-tes...
>>>>>>>> include/gtest/gtest-param-test.h:1429:
>>>>>>>> lose one of the blank lines.
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>>> File include/gtest/internal/gtest-param-util.h (right):
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>>> include/gtest/internal/gtest-param-util.h:512: test_name_stream <<
>>>>>>>> name_func(*param_it);
>>>>>>>> Maybe default the name_func to something that returns 'i' as a
>>>>>>>> string.
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>>> include/gtest/internal/gtest-param-util.h:546: // Keeps triples of
>>>>>>>> <Instantiation name, Sequence generator creation function, Name
>>>>>>>> generator function>
>>>>>>>> reformat to 80-cols.
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>>> include/gtest/internal/gtest-param-util.h:551:
>>>>>>>> ParamNameGeneratorFunc*
>>>>>>>> name_func) :
>>>>>>>> drop the ':' to the next line, indented by 4.
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-...
>>>>>>>> include/gtest/internal/gtest-param-util.h:556: std::string name_;
>>>>>>>> These shouldn't have underscores if they're public.
>>>>>>>>
>>>>>>>> https://codereview.appspot.com/229700044/
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> ---You received this message because you are subscribed to the
>>>>>>>> Google Groups "Google C++ Testing Framework" group.
>>>>>>>> To unsubscribe from this group and stop receiving emails from it,
>>>>>>>> send an email to googletestframework+unsubscribe@googlegroups.com.
>>>>>>>> For more options, visit https://groups.google.com/d/optout.
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Zhanyong
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Zhanyong
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Zhanyong
>>>
>>
>>
>
>
> --
> Zhanyong
>

Sign in to reply to this message.

Billy Donahue

https://codereview.appspot.com/229700044/diff/20001/include/gtest/internal/gtest-param-util.h File include/gtest/internal/gtest-param-util.h (right): https://codereview.appspot.com/229700044/diff/20001/include/gtest/internal/gtest-param-util.h#newcode62 include/gtest/internal/gtest-param-util.h:62: int index_; no underscores for public members. indexes should ...

10 years, 8 months ago (2015-05-12 20:49:09 UTC) #10

Jamie Madill

PTAL. Will test on Linux tomorrow, I've been using Windows MSVC. https://codereview.appspot.com/229700044/diff/20001/include/gtest/internal/gtest-param-util.h File include/gtest/internal/gtest-param-util.h (right): ...

10 years, 8 months ago (2015-05-12 23:53:37 UTC) #11

Review ping!

Sign in to reply to this message.

Issue 229700044: Add support for named value-parameterized tests. (Closed)

Patch Set 1 #

Patch Set 2 : Address design discussion #

Patch Set 3 : Addressed review feedback #

Patch Set 4 : Fix build on Linux #

Patch Set 5 : Clean up GTEST_CHECKs #

Messages