google / googletest

GoogleTest - Google Testing and Mocking Framework
https://google.github.io/googletest/
BSD 3-Clause "New" or "Revised" License
34.72k stars 10.14k forks source link

Ability to pass custom type to `WithParamInterface` and use with `testing::Combine` #3781

Open MikeWeller opened 2 years ago

MikeWeller commented 2 years ago

Does the feature exist in the most recent commit?

No

Why do we need this feature?

More strongly typed parameterized test types, and improved/customizable output of those parameters

Describe the proposal

I would like the ability to pass a custom (non-std::tuple) type to WithParamInterface / TestWithParam while using the testing::Combine generator to generate constructor arguments for this type. Google Test would construct the type with the std::tuple<TYPES...> that is currently produced (or TYPES... directly), and the type of GetParam() would be of the custom type.

The following code demonstrates what I mean:

https://godbolt.org/z/Wr36Eqc8z

#include <gtest/gtest.h>
#include <gmock/gmock.h>

#include <string>
#include <tuple>

using testing::Eq;
using testing::Combine;
using testing::Values;

// 1. It's very useful to define a struct for test parameters, to give things proper names
// and strong typing, automate any destructuring from a 'testing::Combine' tuple, and define
// a customized 'operator<<' (or 'PrintTo') for improved error messages:
struct MyTestParameters {
    bool aBool;
    int anInt;
    std::string aString;

    using GTestTuple = std::tuple<bool, int, std::string>;

    // Either of these defined
    MyTestParameters(bool aBool_, int anInt_, const std::string& aString_)
    : aBool(aBool_)
    , anInt(anInt_)
    , aString(aString_)
    {
    }

    MyTestParameters(const GTestTuple& tuple)
    : aBool(std::get<0>(tuple))
    , anInt(std::get<1>(tuple))
    , aString(std::get<2>(tuple))
    {
    }
};

std::ostream& operator<<(std::ostream& stream, const MyTestParameters& params)
{
    // Customized description to include names etc.
    stream << "{ ";
    stream << "aBool = " << params.aBool;
    stream << ", anInt = " << params.anInt;
    stream << ", aString = " << params.aString;
    stream << " }";

    return stream;
}

// 2. HOWEVER, if we want to use 'testing::Combine' and other generators, the type passed
// to 'WithParamInterface' (or `TestWithParam`) and returned from 'GetParam()' must be of 'std::tuple<...>', since
// the Google Test internals don't know how to construct a custom type:
class MyTest : public testing::Test
             , public testing::WithParamInterface<MyTestParameters::GTestTuple> {
};

// If we try removing the '::GTestTuple' we get:
//
// error: could not convert 'testing::Combine(const Generator& ...) [with Generator = {testing::internal::ParamGenerator<bool>, testing::internal::ValueArray<int, int>, testing::internal::ValueArray<const char*, const char*>}](testing::Values<int, int>(123, 456), testing::Values<const char*, const char*>(((const char*)"foo"), ((const char*)"bar")))' from 'testing::internal::CartesianProductHolder<testing::internal::ParamGenerator<bool>, testing::internal::ValueArray<int, int>, testing::internal::ValueArray<const char*, const char*> >' to 'testing::internal::ParamGenerator<MyTestParameters>'

// Since the parameter type has to be 'std::tuple<...>' we lose the custom printing in
// failure messages:
//
// [  FAILED  ] Parameterized/MyTest.foo/0, where GetParam() = (false, 123, "foo") (0 ms)

// The feature request is therefore:
//
// Have the ability for 'WithParamInterface' to accept a type that is constructible from
// either the 'std::tuple<TYPES...>' type that the generators can output, or the 'TYPES...'
// directly.

INSTANTIATE_TEST_CASE_P(
    Parameterized,
    MyTest,
    testing::Combine(
        testing::Bool(),
        testing::Values(123, 456),
        testing::Values("foo", "bar")));

TEST_P(MyTest, foo) {
    // Currently: implicit conversion/construction from the tuple type returned by 'GetParam'
    //
    // If the 'WithParamInterface' type supported 'MyTestParameters' directly, this will
    // require no conversion.
    const MyTestParameters& params = GetParam();

    // ...

    FAIL();
}

// godbolt doesn't support -lgmock_main/gtest_main
int main(int argc, char **argv) {
  ::testing::InitGoogleTest(&argc, argv);
  return RUN_ALL_TESTS();
}

Note: I'm using WithParamInterface above since our use case is that we have our own base class that inherits from testing::Test so need to add the WithParamInterface separately. However this feature request applies equally to using TestWithParam.

Is the feature specific to an operating system, compiler, or build system version?

No

MikeWeller commented 2 years ago

Note: I would be interested in working on this myself assuming there is agreement on this being useful.

BMBurstein commented 2 years ago

I would like this, too. Any idea if this is being worked on?

BMBurstein commented 2 years ago

I have a fix for this that I am using. It Is not exactly as in the issue. It involves changing this:

INSTANTIATE_TEST_CASE_P(
    Parameterized,
    MyTest,
    testing::Combine(
        testing::Bool(),
        testing::Values(123, 456),
        testing::Values("foo", "bar")));

to this

INSTANTIATE_TEST_SUITE_P(
    Parameterized,
    MyTest,
    testing::ConvertGenerator<MyTestParameters::GTestTuple>(
    testing::Combine(
        testing::Bool(),
        testing::Values(123, 456),
        testing::Values("foo", "bar"))));

@MikeWeller @asoffer Is that an acceptable solution? Should I open a PR with this?

MikeWeller commented 2 years ago

@BMBurstein this looks great, though I think the testing::ConvertGenerator<MyTestParameters::GTestTuple> in your example should instead just be testing::ConvertGenerator<MyTestParameters>( which should match the tuple constructor during conversion.

BMBurstein commented 2 years ago

@BMBurstein this looks great, though I think the testing::ConvertGenerator<MyTestParameters::GTestTuple> in your example should instead just be testing::ConvertGenerator<MyTestParameters>( which should match the tuple constructor during conversion.

Ideally, I would have liked it to work that way, too. The problem is that the ConvertGenerator needs to know the intermediate type through which to do the conversion. The whole reason it is needed in the first place is so that it can do a "double conversion", converting the wrapped generator into the template type, and then converting that type into the final type.

BMBurstein commented 2 years ago

This was merged in #3967

MikeWeller commented 2 years ago

This was merged in #3967

Awesome!