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

Integration with Catch C++ #31

Open
ovanes opened this issue May 17, 2017 · 16 comments
Open

Integration with Catch C++ #31

ovanes opened this issue May 17, 2017 · 16 comments

Comments

@ovanes
Copy link

ovanes commented May 17, 2017

I was previously using Turtle Mock with Boost.Test and was pretty happy, but now we decided to switch to Catch C++ and Turtle Mock does not work any more :(

I had a look at different C++ Mock frameworks and still convinced no other framework is that good as Turtle Mock for various reasons.

I read this documentation Link on how to integrate Turtle Mock with other Frameworks but still can't understand all the details.

Documentation example

By default the library expects to be used in conjunction with Boost.Test e.g. :

  • logs using the logger from Boost.Test
  • throws mock::exception deriving from boost::execution_aborted via boost::enable_current_exception
  • adds Boost.Test checkpoints whenever possible
  • verifies and resets all remaining (static or leaked objects) with a global fixture

However integrating with any given unit test framework can be done by defining a custom error policy implementing the following concept:

template< typename Result >
 struct custom_policy
{
    static Result abort()
    {
        // ...
    }
    template< typename Context >
    static void fail( const char* message, const Context& context, const char* file = "unknown location", int line = 0 )
    {
        // ...
    }
    template< typename Context >
    static void call( const Context& context, const char* file, int line )
    {
        // ...
    }
    static void pass( const char* file, int line )
    {
        // ...
    }
};

The context, which stands for "something serializable to an std::ostream", is actually built only if an attempt to serialize it is made, thus enabling lazy serialization of all elements (e.g. constraints and parameters). File and line show were the expectation has been configured.

The policy can then be activated by defining MOCK_ERROR_POLICY prior to including the library:

#define MOCK_ERROR_POLICY custom_policy
#include <turtle/mock.hpp>

My Questions

  • If the above custom_policy is going to be provided will there be any kind of dependency to Boost.Test?
  • What exactly does custom_policy::abort do and what kind of Result instance should it return? Especially if Result type is used by Turtle Mock in template instantiation.
  • pass seems to be easily mappable to Catch INFO
  • call and fail introduce a context in addition to the pass data being used. I think the context can be mapped to Catch CAPTURE call as it is also lazy:

The message is logged to a buffer, but only reported with the next assertion that is logged. This allows you to log contextual information in case of failures which is not shown during a successful test run (for the console reporter, without -s). Messages are removed from the buffer at the end of their scope, so may be used, for example, in loops.

Many thanks in advance for answering these questions!

@mat007
Copy link
Owner

mat007 commented May 18, 2017

Hi,

If you define MOCK_ERROR_POLICY to use your own policy, there will not be any boost test related code pulled in, besides a convenience class which is just a tool and has nothing to do with the actual test framework (something called basic_cstring which is sort of a std::string_view from before it went into the standard).

custom_policy::abort is supposed to abort in case of irrevocable error (like what BOOST_REQUIRE does for instance) probably by throwing an exception of some sort (or std::exit'ing or something like that but who really wants that ?).
Therefore there should be no need to return any Result value, it is only here to make the caller code compile.

From what I can read the mapping would be

  • abort -> FAIL
  • pass -> INFO
  • call -> INFO
  • fail -> FAIL_CHECK

Indeed the documentation section in turtle could be improved by explaining when each of the policy function gets actually called...

@ovanes
Copy link
Author

ovanes commented May 22, 2017

In less then 60 lines I wrote the integration. Thanks for the help.

Do you plan to completely move Sourceforge repo to GitHub? What do you think to introduce an integrations directory containing integrations to various frameworks like Catch, Google Test etc.

@mat007
Copy link
Owner

mat007 commented May 23, 2017

That's good to hear!

I wouldn't mind moving away from Sourceforge, but I would like to keep the static html documentation as it is for now and would still need to host it somewhere.

I don't think an 'integrations' directory would even be needed, I expect each framework integration to fit into a single header file and we could just include a catch.hpp instead of mock.hpp which would set the policy and then include mock.hpp.
If you wanted to contribute your catch integration implementation, I would gladly add it to turtle!

@ovanes
Copy link
Author

ovanes commented May 23, 2017

I wouldn't mind moving away from Sourceforge, but I would like to keep the static html documentation as it is for now and would still need to host it somewhere.

  • That would be great! GitHub also offers release management, where one can just put zipped or tarred archives or even a bare header file, like this ones: https://github.com/philsquared/Catch/releases
  • Regarding the static directory, it's possible to create a minimalistic README.md (which is already in place) in the root of Turtle and point to documentation at sourceforge (also there). In the sourceforge's Repo Description add a statement, that newer versions are all at GitHub with the link and close the sourceforge repository for commits :) Look, how much nicer it is here to manage issues, which are incredibly readable ;)
    I think it'll increase the popularity of Turtle Mocks. It's really a shame, that it's not so well known. Look at the https://github.com/eranpeer/FakeIt. It has only half of the features Turtle Mock offers and especially worth looking is the integration with other frameworks. Just compare how Catch was integrated in there and here ;)
    My subjective conclusion is: FakeIt probably makes use of all OO-Design features to make it look as bold as possible.

I don't think an 'integrations' directory would even be needed, I expect each framework integration to fit into a single header file and we could just include a catch.hpp instead of mock.hpp which would set the policy and then include mock.hpp.

Yes, my proposal would be to have: turtle/integration/catch.hpp or a like.

If you wanted to contribute your catch integration implementation, I would gladly add it to turtle!

Not sure where to put it, thus not a Pull Request, but just a header I had. Probably it should be hardened agains GCC or Visual C++, since I only used Clang to compile my tests. And as we run Clang in a very restrictive mode I got a bunch of warnings, which probably are inevitable if Turtle Mocks should stay a header lib (e.g. weak-vtables).

#ifndef TURTLE_WITH_CATCH_INTEGRATION_HPP_
# define TURTLE_WITH_CATCH_INTEGRATION_HPP_

#pragma once

#include <catch.hpp>

template<typename Result>
struct catch_turtle_mock_adapter
{
  static Result abort()
  {
    FAIL("Aborted");

    return Result(); // unreachable, used to avoid warnings
  }
  
  template<typename Context>
  static void fail( const char* message
                  , const Context& context
                  , const char* file = "file://unknown-location"
                  , int line=0
                  )
  {
    CAPTURE(context);
    FAIL_CHECK(message << " in: " << file << ":" << line);
  }
  
  template<typename Context>
  static void call( const Context& context, const char* file, int line)
  {
    CAPTURE(context);
    file = file ? file : "file://unknown-location";
    INFO(file << ":" << line);
  }
  
  static void pass(const char* file, int line )
  {
    file = file ? file : "file://unknown-location";
    INFO(file << ":" << line);
  }
};

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated"
#pragma clang diagnostic ignored "-Wextra-semi"
#pragma clang diagnostic ignored "-Wweak-vtables"
#pragma clang diagnostic ignored "-Wdisabled-macro-expansion"
#pragma clang diagnostic ignored "-Wundef"
#pragma clang diagnostic ignored "-Wunknown-pragmas"

#define MOCK_ERROR_POLICY catch_turtle_mock_adapter
#include <turtle/mock.hpp>

#pragma clang diagnostic pop

#endif

@mat007
Copy link
Owner

mat007 commented May 28, 2017

I made a PR with the Catch integration header, see #32
The prama and stuff for clang could also be added to turtle, however as this is somewhat separate from Catch I'll make another PR at some point (after adding clang support to the travis CI build).

I mostly changed only two things which I have pointed out in the PR, tell me what you think.

@mat007
Copy link
Owner

mat007 commented May 28, 2017

Obviously I haven't used FakeIt (and haven't looked at the internals), however there seems to be one major difference standing out: unlike turtle it does not require to write (and compile) mock objects beforehand, probably because it makes use of (non portable compiler dependent) hook or trampoline functions of some sort.
This makes it a little easier to use and probably much quicker to compile test code.

Anyway, I'll release turtle here on github and keep only the web site on sourceforge for the time being as you suggested, not that I think it will really change much but it's easy enough.

@mat007
Copy link
Owner

mat007 commented May 29, 2017

Actually I just realized github provides support for static web sites, e.g. https://help.github.com/articles/creating-project-pages-using-the-command-line/ so there is no real need for keeping sourceforge around.
I'll give it a shot.

I have also started #33 feel free to comment in there if you wish.
Closing this issue now as I believe everything as been addressed, thanks for your feedback and help!

@mat007 mat007 closed this as completed May 29, 2017
@ovanes
Copy link
Author

ovanes commented Jun 18, 2017

Hi Mathieu,

I hope to find more time within the next 2 weeks and finish the port to Catch++. Sorry for the delay. That's not that easy, but somehow, just not enough time right now.

Greetings,
Ovanes

@mat007
Copy link
Owner

mat007 commented Jul 7, 2017

Hey no worries, I know how it is 🏃

Cheers,
MAT.

@mat007
Copy link
Owner

mat007 commented Jul 7, 2017

FWIW here is how INFO is defined

#define INFO( msg ) INTERNAL_CATCH_INFO( "INFO", msg )

with

#define INTERNAL_CATCH_INFO( macroName, log ) \
    Catch::ScopedMessage INTERNAL_CATCH_UNIQUE_NAME( scopedMessage ) = Catch::MessageBuilder( macroName, CATCH_INTERNAL_LINEINFO, Catch::ResultWas::Info ) << log;

and finally

#define CATCH_INTERNAL_LINEINFO ::Catch::SourceLineInfo( __FILE__, static_cast<std::size_t>( __LINE__ ) )

so I suppose instead of using INFO in the turtle integration layer we should do

static void pass( const char* file, int line )
{
    Catch::ScopedMessage INTERNAL_CATCH_UNIQUE_NAME( scopedMessage ) = Catch::MessageBuilder( "INFO", ::Catch::SourceLineInfo( file, static_cast<std::size_t>( line ) ), Catch::ResultWas::Info ) << ( file << ":" << line );
}

(Actually I am not entirely sure we should log anything in this case.)

@ovanes
Copy link
Author

ovanes commented Sep 23, 2017

Hi Mathieu,

I am now experimenting with the Catch support and resolution of the line number problems discussed above. Currently I face the following behaviour and I am not sure it is right. Let me describe it. Suppose I have a simple function:

void foo(message_type const& m, std::promise<std::string>&& p)
{
  if(!protocol_type::has_reply(m.id)
  {
    p.set_value(encoder_type::encode_ack(m.id));
    return;
  }
  reply_queue_.enqueue(std::make_tuple(m.id, std::move(p));
}

Now I mocked protocol_type, reply_queue_ and encoder_type. In general my test case seems to work fine and the errors appear with the correct line numbers, but. If I just for test reverse the condition and something unexpected gets called, like: I configure that reply_queue_.enqueue should never be called, that entire execution gets aborted. Is this really OK?

Actually there are 2 possible flows, which seem to be not so descriptive:

  • I do not state in the TC that enqueue should never be called, than I just receive this error:
/Users/ovanes/.conan/data/Turtle/1.3.0/ovanes/stable/package/52c8e8d320ef379a383bf2e9b53a55b75223f482/include/turtle_with_catch_integration.hpp:15: Failure:
explicitly with messages:
  => aborting test case due to failing mock expectations!
  • I do state, that enqueue must never be called, an than I receive:
unknown location:0: Failure:
explicitly with messages:
  context := registry_.message_registry_mock::enqueue( ? )
v never().with( any )
  unexpected call in: unknown location:0

/Users/ovanes/.conan/data/Turtle/1.3.0/ovanes/stable/package/52c8e8d320ef379a383bf2e9b53a55b75223f482/include/turtle_with_catch_integration.hpp:15: Failure:
explicitly with messages:
  => aborting test case due to failing mock expectations!

Another issue the implementation I changed the way how abort() was implemented. The problem is that Result isn't always default constructible, so compiler was issuing an error where it should not :(. Thus the implementation I've chosen is:

[[noreturn]] static Result abort()
{
  do
  {
    FAIL("=> aborting test case due to failing mock expectations!");
  }while(true);
}

Would be great to hear your thoughts, than I can submit a patch.

@ovanes
Copy link
Author

ovanes commented Sep 23, 2017

I think I now better understand the entire flow and structure. I've posted to Catch Google Group this question about Turtle integration:
https://groups.google.com/forum/?fromgroups#!topic/catch-forum/mpXDjIZIV9A

@mat007
Copy link
Owner

mat007 commented Sep 24, 2017

Hi Ovanes, I'll try and answer as best as I can.

First I don't think you should bother much with 'pass'. If a test framework does not have that feature it shouldn't be a big deal and you could always leave it empty if INFO does not meet the need. See www.boost.org/libs/test/doc/html/boost_test/test_output/test_tools_support_for_logging/checkpoints.html for more information.

Now in order to avoid the infinite do/while you can throw an exception.
Either by using a dummy one

static Result abort()
{
    FAIL("=> aborting test case due to failing mock expectations!");
    throw std::runtime_error( "never raised" );
}

or you can try directly

static Result abort()
{
    throw Catch::TestFailureException();
}

because in theory 'abort' should only cope with aborting the test, the actual logging being taken care of by 'fail'.

As for the issue with 'never', what does your implementation of 'fail' look like ?
Is it https://github.com/mat007/turtle/blob/master/include/turtle/catch.hpp ?

@mat007 mat007 reopened this Sep 24, 2017
@ovanes
Copy link
Author

ovanes commented Sep 24, 2017

Hi Mathieu,

this is my current sketch. After it I'll quote and explain some things...

#include <catch.hpp>
#include <cassert>

template<typename Result>
struct catch_turtle_mock_adapter
{
  [[noreturn]] static Result abort()
  {
    FAIL("=> aborting test case due to failing mock expectations!");
    assert(false && "Unreachable throw statement reached!");
    throw std::runtime_error("Unreachable throw statement reached!");
  }

  static auto source_line_info(const char* file, int line)
  {
    file = (file) ? file : "file://unknown-location";
    return ::Catch::SourceLineInfo{file, static_cast<size_t>(line)};
  }

  template<class SourceLineInfo>
  static auto message_builder( std::string const& severity
                             , SourceLineInfo const& info
                             , ::Catch::ResultWas::OfType numeric_severity
                                 = ::Catch::ResultWas::Info
                             )
  {
    return ::Catch::MessageBuilder{severity, info, numeric_severity};
  }

  template<typename Context>
  static void fail( const char* message
                  , const Context& context
                  , const char* file = "file://unknown-location"
                  , int line=0
                  )
  {
    using namespace Catch;
    auto info = source_line_info(file, line);
    ScopedMessage capturedContext
      { message_builder("CAPTURE", info)
          << "context := " << ::Catch::toString(context)
      }
    ;

    do{
      Catch::ResultBuilder result
        {"FAIL_CHECK", info, "", Catch::ResultDisposition::ContinueOnFailure}
      ;
      result << message << " in: " << file << ":" << line
             << "" + ::Catch::StreamEndStop()
      ;
      result.captureResult(Catch::ResultWas::ExplicitFailure);
      INTERNAL_CATCH_REACT(result)
    } while(::Catch::alwaysFalse());
  }

  template<typename Context>
  static void call(const Context& context, const char* file, int line)
  {
    file = file ? file : "file://unknown-location";
    using namespace Catch;

    auto info = source_line_info(file, line);
    ScopedMessage capturedContext
      { message_builder("CAPTURE", info)
          << "context := " << ::Catch::toString(context)
      }
    ;
    ScopedMessage scopedMessage
      { message_builder("INFO", info) << file << ":" << line }
    ;
  }
  
  static void pass(const char* file, int line )
  {
    file = file ? file : "file://unknown-location";
    using namespace Catch;

    auto info = source_line_info(file, line);

    ScopedMessage scopedMessage
      { message_builder("INFO", info) << file << ":" << line }
    ;
  }
};

In Catch ScopedXXX types make use of RAII. Their logic is:

  • Add a message in ctor to the checkpoint stack.
  • Pop that message from the stack in dtor if no failures occurred.

First I don't think you should bother much with 'pass'.

Given the above execution logic, it is not only about pass, but also about call. We need here some callbacks like: call_started and call_finished. Because otherwise prior the call checkpoint is pushed to the stack and immediately (again prior the execution) popped from it. And this is what makes Catch loosing the context if I understand it right... The same applies to pass respectively.

We could work around it by having an own stack of objects, where xxx_started pushes ScopedXXX objects to it and xxx_finished pops that object from it.

In general, what I think of is to have a typedef inside the adapter type with the context type. A reference to the instance of that type is passed to every adapter member then and is managed by Turtle itself. Each adapter function then decides if it wants to put something in this context or not. It can be cheaply to implement that behaviour in case of current adapters, because they'd just use some some empty type as context and do nothing with it.

What do you think?

@mat007
Copy link
Owner

mat007 commented Sep 24, 2017

I think discussing the code would be easier if it were on a branch 😄

What I notice first is that 'fail' is not supposed to do more than log. It should not throw, see for instance https://github.com/mat007/turtle/blob/master/include/turtle/detail/function_impl_template.hpp#L54 where 'fail' is being called in a loop.

Actually both 'call' and 'pass' purpose is only to log information to the user.
With Boost.Test their logs only appear when using very verbose log levels.
On the other hand 'fail' logs ordinary failures and I believe you might want first to focus on implementing it.

Also why couldn't we bypass the error message stack entirely? After all we already know there is an error by the time 'fail' gets called.
Let's just push a message and never pop it back, for instance

getResultCapture().pushScopedMessage( MessageInfo( … ) );

e.g. what ScopedMessage constructor does?

@ovanes
Copy link
Author

ovanes commented Sep 24, 2017

Regarding getResultCapture().push. I saw it as well. We can only use it for fail an not for the two-phase like functions which have to mark the execution start/finish.

On the other hand, I am not sure if Catch expects fail to throw...

I think discussing the code would be easier if it were on a branch

I agree. Let's do it this way. I will fork your repo at Github and implement my ideas, than we can review them...

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

No branches or pull requests

2 participants