Add support for named value-parameterized tests. (issue 229700044 by jmadill@chromium.org)

[billyd...@google.com]

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 allow yet.

https://codereview.appspot.com/229700044/diff/1/include/gtest/gtest-param-test.h#newcode1418
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-test.h#newcode1429
include/gtest/gtest-param-test.h:1429:
lose one of the blank lines.

https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-param-util.h
File include/gtest/internal/gtest-param-util.h (right):

https://codereview.appspot.com/229700044/diff/1/include/gtest/internal/gtest-param-util.h#newcode512
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-param-util.h#newcode546
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-param-util.h#newcode551
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-param-util.h#newcode556
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/

[Zhanyong Wan (λx.x x)]

It would be great to discuss the design of this new API first.  Can we do that?  Thanks,

--

---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 googletestframe...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

--

Zhanyong

[Billy Donahue]

100% yes.

[Zhanyong Wan (λx.x x)]

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

--

Zhanyong

[Zhanyong Wan (λx.x x)]

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

On Mon, May 11, 2015 at 5:17 PM, Jamie Madill <jma...@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_and_test_names_contain_underscore 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. 

--

Zhanyong

[Zhanyong Wan (λx.x x)]

On Tue, May 12, 2015 at 7:32 AM, Jamie Madill <jma...@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) <w...@google.com> wrote:

- You should avoid _ in test names.  See https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_and_test_names_contain_underscore 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, 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.

--

Zhanyong

[billyd...@google.com]

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 probably be unsigned or size_t.

https://codereview.appspot.com/229700044/diff/20001/include/gtest/internal/gtest-param-util.h#newcode383
include/gtest/internal/gtest-param-util.h:383: inline std::string
(*GetParamNameGen())(const ParamNameArgs<ParamType>&) {
Screams for a typedef, I think.

https://codereview.appspot.com/229700044/diff/20001/include/gtest/internal/gtest-param-util.h#newcode491
include/gtest/internal/gtest-param-util.h:491: typedef
std::string(ParamNameGeneratorFunc)(const ParamNameArgs<ParamType>&);
Move this typedef out to where it can help GetParamNameGen.

https://codereview.appspot.com/229700044/diff/20001/include/gtest/internal/gtest-param-util.h#newcode494
include/gtest/internal/gtest-param-util.h:494:
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_0123456789";
This can't be in a header, portably.

How about a function returning const char* instead?

https://codereview.appspot.com/229700044/