RapidCheck Integration - Githubissues

I was poking around the mettle source (of which I am no expert) to figure out how one might add a properties based test, in particular by calling the RapidCheck property library. The standard way in RapidCheck to call a test is rc::check(test_name, callable) which is tantalizingly close syntax to the way that mettle invokes a unit test.

I thought that a good approach might be to extend mettle's suite_builder<> template with a new member function called property(), which would behave as close as possible to test(), and would basically call the same property based test as rc::check(), with the same arguments even.

Now mettle is really generic and extendable, but I am not sure that augmenting suite_builder<> with a new member function in user code is easy or possible. It also makes no sense to add a dependency on RapidCheck. I think the correct way is to actually add the extension to RapidCheck itself, alongside similar features for boost::test and GoogleTest.

Is there an easy and elegant way to get my property() member function alongside test(), or is this hard?

If you don't mind waiting a couple weeks, I'll see if I can whip up some code (probably as a separate project) that you could use to glue mettle and RapidCheck together. I'm just finishing up a few loose ends before I release 1.0a1 of mettle, so after that would be a good time to experiment with integrating with other tools. If you want to look into doing this yourself, read on.

Off the top of my head, I can think of two different ways to do this: first, you could write a wrapper function that takes a RapidCheck callback and does whatever magic is required to turn it into a callable that mettle understands. Something like this:

// This should really be a function template that can take arguments for any fixtures...
std::function<void(void)>
wrap_rc(some_class callable) {
  return [callable]() {
    if (!run_rapidcheck(callable))
      raise mettle::expectation_error(rapidcheck_msg);
  };
}

// Somewhere inside a suite:
_.test("my rapidcheck test", wrap_rc([](const myclass &x) { /* ... */ }));

What really matters in the above is that you spit out a function with the right arity and which raises a mettle::expectation_error on failure (although really, you could raise other errors and it would just have slightly different output).

Another way to do it would be to create your own rc_suite (and possibly rc_subsuite) classes. mettle::suite_builder isn't actually important for running tests. The most important class is actually mettle::compiled_suite<T> (and mettle::runnable_suite, which is just an alias for mettle::compiled_suite<mettle::test_result(void)>). Compiled suites are really just three things: 1) a suite name, 2) a vector of mettle::basic_test_infos (which are themselves a test name, callable, list of attributes, and a UID), and 3) a vector of compiled subsuites.

If you look in suite/make_suite.hpp and suite/global_suite.hpp (glue.hpp has a couple relevant bits too), you can see how mettle::make_suites and mettle::make_subsuites work. You can also see a possibly-helpful example of how you can create subsuites in separate functions here: https://github.com/jimporter/mettle/blob/master/test/driver/test_filters.cpp#L216-L217. The functions themselves (defined above that snippet), call mettle::make_subsuites, which returns a compiled suite. If you make a function or class that also returns a mettle::compiled_suite, you can add those to an existing suite (or to the list of top-level suites) with no real problems.

I would probably lean towards creating an rc_suite class, unless you really want to have mettle tests and RapidCheck tests alongside each other in the same suite.

Jim, thanks for the guidance. I will call out @emil-e on this message too. After many fits and starts, I have an example that looks pretty cool. My first version just created a compiled suite as you suggested. I kept beating on it and this is what I have now.

First of all, this is what the suite looks like. It mixes both unit tests and property tests together. The new thing is the property(...) builder. The property itself is the first example from the rapidcheck README, except I replaced the rapidcheck assertion with mettle's version. Both work (I checked).

mettle::property_suite<> properties("properties", [](auto &_) {
    _.test("passing test", []() {
        expect(true, mettle::equal_to(true));
        });

     _.test("failing test", []() {
        expect(true, mettle::equal_to(false));
        });

    _.property("passing property",
            [](const std::vector<int>& l0) {
                auto l1 = l0;
                std::reverse(begin(l1), end(l1));
                std::reverse(begin(l1), end(l1));
                expect(l0, mettle::equal_to(l1));
            });

    _.property("failing property",
            [](const std::vector<int>& l0) {
                auto l1 = l0;
                std::reverse(begin(l1), end(l1));
                std::reverse(begin(l1), end(l1));
                expect(l0, mettle::not_equal_to(l1));
            });
});

Running this suite spews

.!.!

2/4 tests passed
  properties > failing test FAILED
    expected: false
    actual:   true
  properties > failing property FAILED
    Falsifiable after 1 tests

    std::tuple<std::vector<int>>:
    ([])

    Exception thrown with message:
    expected: not []
    actual:   []

which is great, because we get the normal mettle behavior but also the rapidcheck behavior also.

For mettle infrastructure, there is only one class that is actually new. It provides exactly two things: the property(...) builder function, and a wrapper to invoke rc::detail::checkTestable(...). So the impedance matching between mettle and rapidcheck is wholly contained in this wrap_property() member:

namespace mettle {

template<typename Exception, typename Factory, typename ...T>
class property_suite_builder : public suite_builder<Exception, Factory, T...> {
  using base = suite_builder<Exception, Factory, T...>;

public:
    using base::suite_builder;

    template <typename Testable>
    void property(std::string name, Testable&& f) {
        base::tests_.push_back({name, wrap_property(f), {} });
    }

private:
    template <typename Testable>
    auto wrap_property(Testable testable) {
        return [testable]() {

            const auto result = rc::detail::checkTestable(testable);

            if (result.template is<rc::detail::SuccessResult>()) {
                std::ostringstream message;
                const auto success = result.template get<rc::detail::SuccessResult>();
                if (!success.distribution.empty())
                    printResultMessage(result, message);
                return mettle::test_result { true, message.str() };
            } else {
                std::ostringstream message;
                printResultMessage(result, message);
                throw mettle::expectation_error(message.str());
            }
        };
    }
};

So all of that looks pretty cool.

The nasty part is punching the new property_suite_builder<>, which is just a subclass of suite_builder<>, through mettle's stock classes. What follows is all copied from mettle, with suite_builder replaced by property_suite_builder:

template<typename Exception, typename ...Fixture, typename ...Args>
inline runnable_suite
make_basic_property_suite(const std::string &name, const attributes &attrs,
                 Args &&...args) {
    using Applied = detail::apply_type<property_suite_builder, Exception>;
    return detail::do_build<Applied::template type, Fixture...>(
            name, attrs, std::forward<Args>(args)...
            );
}

template<typename Exception, typename ...Fixture, typename ...Args>
inline auto
make_basic_property_suites(const std::string &name, const attributes &attrs,
                 Args &&...args) {
    using Applied = detail::apply_type<property_suite_builder, Exception>;
    return detail::do_builds<Applied::template type, Fixture...>(
            name, attrs, std::forward<Args>(args)...
            );
}

template <typename... Fixture, typename Factory, typename F>
inline runnable_suite
make_property_suite(const std::string& name, const attributes& attrs,
    Factory&& factory, F&& f) {
    return make_basic_property_suite<mettle::expectation_error, Fixture...>(
        name, attrs, std::forward<Factory>(factory), std::forward<F>(f)
    );
}

template<typename ...Fixture, typename Factory, typename F>
inline std::array<mettle::runnable_suite, std::max(sizeof...(Fixture), std::size_t(1))>
make_property_suites(const std::string &name, const mettle::attributes &attrs,
           Factory &&factory, F &&f) {
    return make_basic_property_suites<expectation_error, Fixture...>(
            name, attrs, std::forward<Factory>(factory), std::forward<F>(f)
  );
}

template <typename Exception, typename... Fixture>
struct property_suite_base
{
    template <typename... Args>
    property_suite_base(const std::string& name, Args&&... args)
    {
        auto&& suites = make_basic_property_suites<Exception, Fixture...> (
            name, {}, std::forward<Args>(args)...);

        suites_list& list = detail::all_suites();

        for (auto&& i: suites)
            list.push_back(std::move(i));
    }
};

template <typename... T>
using property_suite = property_suite_base<expectation_error, T...>;

} // namespace mettle

Maybe this last part is the right solution, or maybe we should pass the builder type as yet another template parameter. This current solution, probably, does not fully support all of mettle's features like fixtures, attributes, and subsuites. To get all of that, I would have to copy/paste/replace a lot more of make_suite.hpp and friends.

So this experiment is pretty exciting. It has a really nice user interface modeled after test(...), and the only dependency on rapidcheck is wrap_property() which would require a rapidcheck include. This pattern suggests that property_suite_builder could actually be distributed not with mettle but with rapidcheck, where many other unit test frameworks are similarly supported. It feels like we could just make suite_builder a template parameter in make_suite and friends and recover the rest of mettle's features to be used in properties.

Hmm, thinking about it more, it might be simpler to just expose wrap_property as a public function and then use it like this:

_.test("my property test", wrap_property([](const std::vector<int>& l0) {
  // ...
}));

That makes it a lot easier to inject RapidCheck into mettle, since the only thing you should need is the wrap_property function. Of course, the downside is that the syntax is a little less pretty. Still, what you mentioned above does make sense and it would probably be useful to make it easier to create new subclasses of mettle::suite_builder. I'm not quite sure how I want to do that yet, but I'll try out a few experiments and see what I like. I may end up rewriting some of that stuff wholesale, since it's a bit complex.

For my implementation, I was actually thinking about making property_suite define the "fixture" that RapidCheck is testing. Then, it would be easy(-ish) to create sub-property_suites and get a combinatorial explosion of tests. :) However, I haven't looked into RapidCheck's implementation in detail yet, and this may not be the right way to do things.

That said, I'm pretty stoked that things came together so well! Aside from the make_suite issues, there are a couple of things I see in your code that indicate unnecessary limitations in mettle (such as how you can't give mettle a message to print when a test passes). I'll take a look at those first, since they should apply to a wide variety of mettle extensions.

I don't have any particular opinion on how this fits in to mettle, of course, but when it comes to RapidCheck, it would be awesome if you could provide TestMetadata. At least the id field since that is used by RapidCheck when you want to reproduce a failing test case using the reproducing feature. That'd be the second argument to the checkTestable function.

It seems like the wrap_property method makes it hard to create a stable and unique identifier for a given test.

Yeah, I am assuming that there will always be unsupported mettle features until every bit of make_suite.hpp and friends is adapted to accept a suite_builder subclass, either by template or a lot of copy-and-paste. There are a lot of ways to build the suites, and every line in there is there supports some combination use useful features. Of all those combinations, the property_suite_builder only supports one so far.

Going forward to get all the features, it makes a big difference on how you decide to get the coverage inside of make_suite().

Emil, that is a good suggestion to get better coverage on RapidCheck features. I have not yet actually tried the reproducing feature, so I was not thinking about it. 8-) This sounds like a solvable problem. I used a lambda for wrap_property(...) out of laziness, but that could be improved to support all of RapidCheck with some bigger functor.

To go forward, we kind of need a branch(es) to work on. I might let Jim mull the new pattern over for a few days first. If we get a new mettle branch from Jim that allows a builder subclass, then I'll use that and I will only have to fork RapidCheck and not mettle. Actually, there will be a RapidCheck fork in either case to add rapidcheck/extras/mettle and we can merge that back into your master branch if and when you like it. The uncertainty at the moment is just how much mettle boilerplate is required inside of rapidcheck.

I'd be more than happy to merge in support for mettle if it seems appropriate (w.r.t. feature coverage, test coverage, code quality etc.).

@JohnGalbraith getting back to this, are there any major issues with my suggestion here? You could provide some code like this:

template <typename Testable>
auto property(Testable testable) {
  return [testable]() {
    // see above...
  };
}

And then use it like:

mettle::suite<> test_properties("properties", [](auto &_) {
  _.test("passing property", property([](const std::vector<int> &vec) {
    // see above...
  });
});

I like this way because it's easy for multiple third-parties to create their own wrapper functions like property() that test authors can mix-and-match at will in their tests. If we go the route of creating a special "property suite", then someone else might make a "foobar suite", but a test author would be unable to put a property-test and a foobar-test in the same suite (unless they made their own property-and-foobar suite type).

Granted, this way is a bit more verbose (you have to type test() and property()) and the semantics are worse (you can't just look at the beginning of the line to know what kind of test this is), but it's more flexible and keeps the suite types simple: they only have two kinds of things: tests (i.e. functions) and nested collections of more tests.

I can definitely see an argument for allowing subclasses of suites that hold all-new kinds of things, but it feels to me like this is ultimately just an ordinary test that needs a metafunction to be called on the test function first.

@JohnGalbraith Another option that would give RapidCheck properties (mostly) equal footing with regular mettle tests would be for me to add test() as a free function, similar to how there's a subsuite() free function (though the latter exists to simplify calling it with template args). Then you could easily make a property() free function that does all the necessary setup and gives you nice syntax.

I am in total agreement about the pros and cons of making a property just a special kind of test vs. it's own non-test stature. It reads way better to say _.property(...) but it certainly scales less well when you want to add not just properties, but foobars also. So yeah, what you said. There is probably a variadic template lurking here somewhere that could accept property and foobar classes to augment the interface jointly to instantiate the property_and_foobar_suite but it might get kind of hairy. I know that I already have trouble with Mettle compile times with big parameterized suites (my favorite Mettle feature!) and getting even more cute with templates could make this worse.

The free function idea would seem to improve the semantics somewhat and make it more clear that we are configuring a property or foobar, and is clearly easy to extend. The extra argument _ is kind of annoying, but not too big of a deal. It's probably worth the clarity that this object is a property and not a test.

What about a pattern such as:

_.test<property>("passing property", ...)

Which is kind of a middle ground, or even

_.add<test>("my unit test", ...);
_.add<property>("my passing property", ...);
_.add<foobar>("another flight", ...);

to make unit tests, properties, and foobars all equal footing, plus easy to extend also. After all this thinking about it, I am coming around to liking the free function the best, perhaps. It could look like:

mettle::subsuite<...>(_, "unit tests", []() {
        _.test("my unit test", ...);
    });

mettle::property_suite<...>(_, "important properties", []() {
         _.property("passing property", ...);
});

which allows me to parameterize the property which is pretty cool, keeps clear semantics, plus I can add member functions in addition to property(...) as I want to, in my user customized class property_suite. Maybe I could even use multiple inheritance to get all of my features into one suite this way.

I know that I already have trouble with Mettle compile times with big parameterized suites (my favorite Mettle feature!) and getting even more cute with templates could make this worse.

Yeah, this is something I'm very conscious of and want to improve, not regress. :)

The extra argument _ is kind of annoying, but not too big of a deal.

True; I'd love to eliminate it entirely, but that would involve a little more black magic than I'm comfortable with, and it would probably break some of the more-esoteric ways to work with mettle.

What about a pattern such as:

_.test<property>("passing property", ...)

We can't actually do this, unfortunately, since _ has a templated type (from [](auto &_)), so we'd have to call it as _.template test<property>(...). That's the same reason I have the alternate form for subsuites: mettle::subsuite<...>(_, ...). (Concepts might fix this by letting me provide a concept that describes the suite builder API so that auto above becomes mettle::Builder, but that requires C++20.)

After all this thinking about it, I am coming around to liking the free function the best, perhaps. It could look like: [snip]

That would work, but implies that property_suites are always subsuites of a root suite. I don't think that's necessary, unless there's special stuff you want to do at the suite level for properties (based on the code you've posted above, it doesn't look like you need that though). I'm imagining something like this:

template <typename Builder, typename Testable>
auto property(Builder &_, std::string name, Testable testable) {
  _.test(name, [testable]() {
    // Do all the appropriate setup for a RapidCheck property. See
    // `wrap_property()` above.
  });
}

mettle::suite<...> my_root_suite("my root suite", [](auto &_) {
  // `test()` is in the `mettle` namespace and should be picked up via ADL from
  // the `_` argument. It has the same effect as `_.test()` and is provided as
  // an alternate form for symmetry with `subsuite()` and third-party functions
  // like `property()`.
  test(_, "my basic test", []() {
    // Do some testing here.
  });

  property(_, "my property", [](const std::vector<int> &vec) {
    // Test the property here.
  });
});

The benefit here is that all you have to write is the property() function at the beginning of that block and then you get all of the mettle::suite stuff for free, since you're just using ordinary suites.

@JohnGalbraith I just noticed that I failed to "@" you in the previous comment. :/ Doing so here in case you missed it.

Looking at the example, it really shows that using:

   _.test("my test", ...);

is really no more or less annoying that using:

   test(_, "my test", ...);

One still has to figure out what the _ means in either case. This was the main dissonance I had to deal with when trying out Mettle the first time. My first application renamed _ to some name, until I came around to your original pattern. Maybe the free function is actually nicer, because at least I read test first, instead of _. which is more meaningful. Now I immediately see that _ is just a required argument on every function, very similar to implicit this in C++ or self in Python. So the free functions are still looking good.

There might be a bit of trickiness if the suite<..> and parameters so the free function now requires a fixture argument. Not a show stopper, but it might present authors of property() and foobar() some of the overloading complexity that currently is hidden inside of Mettle. Not sure yet if this is a big deal.

Does the presence of attributes affect anything?

Still, it seems like the free function can solve all the problems we have been discussing.

One still has to figure out what the _ means in either case. This was the main dissonance I had to deal with when trying out Mettle the first time.

That's a good point. It would probably be a good idea to add a little bit of "how the sausage is made" in the docs. (Some of this might be more obvious when I add concepts support, since the [](auto &_) idiom can become [](Builder &_).)

There might be a bit of trickiness if the suite<..> and parameters so the free function now requires a fixture argument. Not a show stopper, but it might present authors of property() and foobar() some of the overloading complexity that currently is hidden inside of Mettle. Not sure yet if this is a big deal.

This should be straightforward:

template <typename Builder, typename Testable>
auto foobar(Builder &_, std::string name, Testable testable) {
  // Accept any fixture parameters in our wrapper lambda, and then forward them
  // onto our real function.
  _.test(name, [testable](auto &...fixtures) {

    std::string extra_arg = "from foobar()";
    // Fixtures are always references, so I don't think we need to jump through
    // the perfect-forwarding hoops here.
    testable(extra_arg, fixtures...);

  });
}

In theory, you could also get the types of the fixtures from the suite builder (so that you're not saying auto &...fixtures on line 3 above). This would let you turn that lambda from a generic one to a regular one, which might help compile times, but I'm not sure it's worth the effort. Still, a type trait to get more info about the full set of fixtures (instead of just the last one via mettle::fixture_type_t) would probably be a useful addition to mettle.

Does the presence of attributes affect anything?

Attributes should only mean you need to have two overloads of property(), one with 3 args (no attrs) and one with 4 (with attrs).

@JohnGalbraith As I see it, there are basically three things I should fix in mettle here:

[x] Add a mettle::test() free function for symmetry
[ ] Add more-complete type traits for fixtures (not really needed, but might make some things easier)
[ ] Add some documentation explaining the more-interesting parts of mettle's internals

Does that sound reasonable?

jimporter / mettle

RapidCheck Integration #24