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

Splitting catch configuration into multiple translation units not possible #991

Closed
rcdailey opened this Issue Aug 11, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@rcdailey
Contributor

rcdailey commented Aug 11, 2017

I have the following:

TestMain.cpp:

#define CATCH_CONFIG_RUNNER
#include <catch.hpp>

#include <algorithm>

#include "Detail/MockFrameworkSetup.hpp"
#include "Detail/LogRedirect.hpp"

int main(int argc, char** argv)
{
   int result{};
   Catch::Session catch_session;

   result = catch_session.applyCommandLine(argc, argv);
   if (result != 0)
   {
      return result;
   }

   Detail::SetupMockFramework(catch_session);
   Detail::SetupLogRedirect(catch_session);

   // Returns the number of failed tests
   result = catch_session.run();

   // Do a convenient debug break on MSVC (Windows) when tests fail
#if defined(_MSC_VER) && defined(_DEBUG)
   if (result != 0)
   {
      __debugbreak();
   }
#endif

   // Note that on unix only the lower 8 bits are usually used, clamping
   // the return value to 255 prevents false negative when some multiple
   // of 256 tests has failed
   return std::min(result, 0xff);
}

I have separate translation units that implement Detail::SetupMockFramework and Detail::SetupLogRedirect. Some implement reporters, others use the session object to configure Catch in some way.

I cannot decompose my code this way because catch.hpp's implementation is only designed to be in one translation unit. I get linker errors due to ODR violations because of this.

How can I include catch.hpp without violating ODR so that I can modularize my unit test setup code?

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Aug 15, 2017

Member

Currently you can't.

We definitely intend to change this in the future, but as of now you have to eventually throw everything into the same file.

Member

horenmar commented Aug 15, 2017

Currently you can't.

We definitely intend to change this in the future, but as of now you have to eventually throw everything into the same file.

horenmar added a commit that referenced this issue Aug 17, 2017

Add define that pulls in reporter and listeners interfaces
This allows users to define reporters and listeners in files different
from the main file.

Related to #991, #986
@rcdailey

This comment has been minimized.

Show comment
Hide comment
@rcdailey

rcdailey Sep 20, 2017

Contributor

I see a few commits for this have been made. Is this possible yet?

Contributor

rcdailey commented Sep 20, 2017

I see a few commits for this have been made. Is this possible yet?

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Sep 20, 2017

Member

@rcdailey I am going to quote myself from the last commit

Reporter / Listener interfaces

CATCH_CONFIG_EXTERNAL_INTERFACES // Brings in neccessary headers for Reporter/Listener implementation

Brings in various parts of Catch that are required for user defined Reporters and Listeners. This means >that new Reporters and Listeners can be defined in this file as well as in the main file.

Implied by both CATCH_CONFIG_MAIN and CATCH_CONFIG_RUNNER.

That is a yes, but only for Catch 2.

Member

horenmar commented Sep 20, 2017

@rcdailey I am going to quote myself from the last commit

Reporter / Listener interfaces

CATCH_CONFIG_EXTERNAL_INTERFACES // Brings in neccessary headers for Reporter/Listener implementation

Brings in various parts of Catch that are required for user defined Reporters and Listeners. This means >that new Reporters and Listeners can be defined in this file as well as in the main file.

Implied by both CATCH_CONFIG_MAIN and CATCH_CONFIG_RUNNER.

That is a yes, but only for Catch 2.

@horenmar horenmar removed the In progress label Nov 3, 2017

@rcdailey

This comment has been minimized.

Show comment
Hide comment
@rcdailey

rcdailey Nov 3, 2017

Contributor

So I finally got around to testing this, and I'm having some difficulty. I am using the official 2.0.1 release of Catch. Here is my reporter:

#define CATCH_CONFIG_EXTERNAL_INTERFACES
#include <catch.hpp>

class ErrorLogReporter : public Catch::ConsoleReporter
{
public:

   ///////////////////////////////////////////////////////////////////////////////
   ///
   ///////////////////////////////////////////////////////////////////////////////
   ErrorLogReporter(Catch::ReporterConfig const& config)
      : Catch::ConsoleReporter(config)
   {}

   ///////////////////////////////////////////////////////////////////////////////
   /// Describes this reporter to Catch. Required by the template functions used
   /// when the INTERNAL_CATCH_REGISTER_REPORTER macro is called.
   ///////////////////////////////////////////////////////////////////////////////
   static std::string getDescription()
   {
      return "ErrorLogReporter";
   }

   ///////////////////////////////////////////////////////////////////////////////
   /// Invoked when a catch test case (`TEST_CASE` macro) is about to start. Clear
   /// the buffered logs each time a test case starts.
   ///////////////////////////////////////////////////////////////////////////////
   void testCaseStarting(Catch::TestCaseInfo const& info) override
   {
      Catch::ConsoleReporter::testCaseStarting(info);
      CatchSink::ClearLogs();
   }

   ///////////////////////////////////////////////////////////////////////////////
   /// Invoked when a section (`SECTION` macro) is about to start. Clear the
   /// buffered logs each time a section starts.
   ///////////////////////////////////////////////////////////////////////////////
   void sectionStarting(Catch::SectionInfo const& info) override
   {
      Catch::ConsoleReporter::sectionStarting(info);
      CatchSink::ClearLogs();
   }

   ///////////////////////////////////////////////////////////////////////////////
   /// Invoked each time a test assertion macro is done evaluating. This is
   /// overridden for the purposes of printing logs captured by CatchSink only if
   /// the assertion in question fails (i.e. is not "ok").
   ///////////////////////////////////////////////////////////////////////////////
   bool assertionEnded(Catch::AssertionStats const& stats) override
   {
      // Allow catch to print the results of the assertion failure first.
      // We'll print our own logs below that.
      auto result = Catch::ConsoleReporter::assertionEnded(stats);

      auto const& logs = CatchSink::LogOutput();
      if (!stats.assertionResult.isOk() && !logs.empty())
      {
         Catch::cout()
            << "#######################################################################\n"
            << "## CAPTURED LOGS\n"
            << "#######################################################################\n"
            << logs << "\n"
            << "#######################################################################\n"
            << "\n"
            ;

         CatchSink::ClearLogs();
      }

      return result;
   }

};

First compiler issue is that it says Catch::ConsoleReporter is not available. And looking at the single header, that class is not within the ifdef block for CATCH_CONFIG_EXTERNAL_INTERFACES. So how can I properly implement this reporter?

Contributor

rcdailey commented Nov 3, 2017

So I finally got around to testing this, and I'm having some difficulty. I am using the official 2.0.1 release of Catch. Here is my reporter:

#define CATCH_CONFIG_EXTERNAL_INTERFACES
#include <catch.hpp>

class ErrorLogReporter : public Catch::ConsoleReporter
{
public:

   ///////////////////////////////////////////////////////////////////////////////
   ///
   ///////////////////////////////////////////////////////////////////////////////
   ErrorLogReporter(Catch::ReporterConfig const& config)
      : Catch::ConsoleReporter(config)
   {}

   ///////////////////////////////////////////////////////////////////////////////
   /// Describes this reporter to Catch. Required by the template functions used
   /// when the INTERNAL_CATCH_REGISTER_REPORTER macro is called.
   ///////////////////////////////////////////////////////////////////////////////
   static std::string getDescription()
   {
      return "ErrorLogReporter";
   }

   ///////////////////////////////////////////////////////////////////////////////
   /// Invoked when a catch test case (`TEST_CASE` macro) is about to start. Clear
   /// the buffered logs each time a test case starts.
   ///////////////////////////////////////////////////////////////////////////////
   void testCaseStarting(Catch::TestCaseInfo const& info) override
   {
      Catch::ConsoleReporter::testCaseStarting(info);
      CatchSink::ClearLogs();
   }

   ///////////////////////////////////////////////////////////////////////////////
   /// Invoked when a section (`SECTION` macro) is about to start. Clear the
   /// buffered logs each time a section starts.
   ///////////////////////////////////////////////////////////////////////////////
   void sectionStarting(Catch::SectionInfo const& info) override
   {
      Catch::ConsoleReporter::sectionStarting(info);
      CatchSink::ClearLogs();
   }

   ///////////////////////////////////////////////////////////////////////////////
   /// Invoked each time a test assertion macro is done evaluating. This is
   /// overridden for the purposes of printing logs captured by CatchSink only if
   /// the assertion in question fails (i.e. is not "ok").
   ///////////////////////////////////////////////////////////////////////////////
   bool assertionEnded(Catch::AssertionStats const& stats) override
   {
      // Allow catch to print the results of the assertion failure first.
      // We'll print our own logs below that.
      auto result = Catch::ConsoleReporter::assertionEnded(stats);

      auto const& logs = CatchSink::LogOutput();
      if (!stats.assertionResult.isOk() && !logs.empty())
      {
         Catch::cout()
            << "#######################################################################\n"
            << "## CAPTURED LOGS\n"
            << "#######################################################################\n"
            << logs << "\n"
            << "#######################################################################\n"
            << "\n"
            ;

         CatchSink::ClearLogs();
      }

      return result;
   }

};

First compiler issue is that it says Catch::ConsoleReporter is not available. And looking at the single header, that class is not within the ifdef block for CATCH_CONFIG_EXTERNAL_INTERFACES. So how can I properly implement this reporter?

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Nov 3, 2017

Member

@rcdailey CATCH_CONFIG_EXTERNAL_INTERFACES does not imply reporters, only reporter interfaces, bases and registrars (and also their transitive dependencies). This was done to keep the macro relatively cheap to enable.

If you want your own reporter to inherit from an existing reporter, it still has to go into the main file.

I could be convinced to include the full reporters in there, but it was mainly intended for codebases that need a lot of relatively simple listeners and not those that need/want to customize the provided reporters.

Member

horenmar commented Nov 3, 2017

@rcdailey CATCH_CONFIG_EXTERNAL_INTERFACES does not imply reporters, only reporter interfaces, bases and registrars (and also their transitive dependencies). This was done to keep the macro relatively cheap to enable.

If you want your own reporter to inherit from an existing reporter, it still has to go into the main file.

I could be convinced to include the full reporters in there, but it was mainly intended for codebases that need a lot of relatively simple listeners and not those that need/want to customize the provided reporters.

@horenmar horenmar added the Revisit label Nov 14, 2017

horenmar added a commit that referenced this issue Nov 14, 2017

horenmar added a commit that referenced this issue Nov 14, 2017

horenmar added a commit that referenced this issue Nov 14, 2017

horenmar added a commit that referenced this issue Nov 14, 2017

horenmar added a commit that referenced this issue Nov 14, 2017

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Nov 14, 2017

Member

@rcdailey I ended up separating the 4 provided reporters into headers + impl files and including them as part of external interfaces, so your use case should work now.

I will also be doing some slimming down of the includes, so unintended* interfaces might be gone soon.

* Intended interfaces are not quite well defined, but they are the reporter/listener bases, the default reporters and some more soon.

Member

horenmar commented Nov 14, 2017

@rcdailey I ended up separating the 4 provided reporters into headers + impl files and including them as part of external interfaces, so your use case should work now.

I will also be doing some slimming down of the includes, so unintended* interfaces might be gone soon.

* Intended interfaces are not quite well defined, but they are the reporter/listener bases, the default reporters and some more soon.

@rcdailey

This comment has been minimized.

Show comment
Hide comment
@rcdailey

rcdailey Nov 14, 2017

Contributor

One thing I learned through this is how to properly register reporters. What I was doing before was customizing the console reporter, which is harder. What I have now is a completely custom reporter, that I add in addition to the built-in console reporter, which allows me to append information in the output after the normal console output. This is the desired behavior in my case, and much better.

Having said that, one thing that is severely missing is documentation on reporters, examples of how to implement then, explanations of the interfaces a subclass can implement, and so forth. I think it's important to help teach people the proper way of doing these advanced integrations into Catch.

I get that reporters have been a bit of a moving target the past few months, but hopefully with 2.0 release line things stabilize enough that reliable documentation can be written. I'd offer to do this, but I'm still struggling to understand fundamentals, so I think someone more knowledgeable on the topic should do it. Either way this is outside the scope of this particular issue.

Contributor

rcdailey commented Nov 14, 2017

One thing I learned through this is how to properly register reporters. What I was doing before was customizing the console reporter, which is harder. What I have now is a completely custom reporter, that I add in addition to the built-in console reporter, which allows me to append information in the output after the normal console output. This is the desired behavior in my case, and much better.

Having said that, one thing that is severely missing is documentation on reporters, examples of how to implement then, explanations of the interfaces a subclass can implement, and so forth. I think it's important to help teach people the proper way of doing these advanced integrations into Catch.

I get that reporters have been a bit of a moving target the past few months, but hopefully with 2.0 release line things stabilize enough that reliable documentation can be written. I'd offer to do this, but I'm still struggling to understand fundamentals, so I think someone more knowledgeable on the topic should do it. Either way this is outside the scope of this particular issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment