approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
318 stars 51 forks source link

F Update to doctest 2.3.5 #37

Closed claremacrae closed 5 years ago

claremacrae commented 5 years ago

Description

Makes ApprovalTests.cpp work with the latest version of doctest - 2.3.5.

https://github.com/onqtam/doctest/releases/tag/2.3.5

The solution

There is a new virtual method test_case_reenter() added to doctest::IReporter, so I added an empty implementation of that, in order to get our code to work with the new version...

Comments

For consistency, I added this line:

        virtual void test_case_reenter(const doctest::TestCaseData&) override {}

By including the override, this requires users of Approval Tests to be building against doctest 2.3.5...

We could decide we wanted to keep the option to build against 2.3.4 - and so remove override.

claremacrae commented 5 years ago

I've reinstated support for Doctest v2.3.4, as our user guide says we support that or above, and it seems a shame to be more restrictive:

https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/UsingDoctest.md#top

claremacrae commented 5 years ago

Fixing this involves setting something up to disable the warning on Clang only:

https://travis-ci.com/claremacrae/ApprovalTests.cpp/jobs/243447365#L806

In file included from /home/travis/build/claremacrae/ApprovalTests.cpp/tests/DocTest_Tests/main.cpp:4:
In file included from /home/travis/build/claremacrae/ApprovalTests.cpp/tests/DocTest_Tests/ApprovalTests.hpp:6:
/home/travis/build/claremacrae/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/integrations/doctest/DocTestApprovals.h:29:22: error: 
      'test_case_reenter' overrides a member function but is not marked
      'override' [-Werror,-Winconsistent-missing-override]
        virtual void test_case_reenter(const doctest::TestCaseData&) /*o...
                     ^
/home/travis/build/claremacrae/ApprovalTests.cpp/third_party/doctest/doctest.2.3.5.h:1673:18: note: 
      overridden virtual function is here
    virtual void test_case_reenter(const TestCaseData&) = 0;
                 ^
1 error generated.

If I use CLion to disable that warning I get this:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Winconsistent-missing-override"
        // called when a test case is reentered because of unfinished subcases (safe to cache a pointer to the input)
        virtual void test_case_reenter(const doctest::TestCaseData&) /*override*/ {} // Not override, to support doctest v2.3.4
#pragma clang diagnostic pop

Which then doesn't compile in my cygwin gcc build:

In file included from /cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/tests/DocTest_Tests/ApprovalTests.hpp:6:0,
                 from /cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/tests/DocTest_Tests/main.cpp:4:
/cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/ApprovalTests/integrations/doctest/DocTestApprovals.h:28:0: error: ignoring #pragma clang diagnostic [-Werror=unknown-pragmas]
 #pragma clang diagnostic push

/cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/ApprovalTests/integrations/doctest/DocTestApprovals.h:29:0: error: ignoring #pragma clang diagnostic [-Werror=unknown-pragmas]
 #pragma clang diagnostic ignored "-Winconsistent-missing-override"

In file included from /cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/tests/DocTest_Tests/ApprovalTests.hpp:6:0,
                 from /cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/tests/DocTest_Tests/main.cpp:4:
/cygdrive/d/Users/Clare/Documents/Programming/github/ApprovalTests/ApprovalTests.cpp-claremacrae/ApprovalTests/integrations/doctest/DocTestApprovals.h:32:0: error: ignoring #pragma clang diagnostic [-Werror=unknown-pragmas]
 #pragma clang diagnostic pop

Options include:

  1. Finding a good example of how to suppress warnings for a range of compilers
  2. Stopping treating warnings as errors - but that will prevent user code from treating warnings as errors
  3. Giving up and dropping support for doctest 2.3.4
  4. Using the doctest version number to decide whether to add the new method - which is probably the right answer.
#define DOCTEST_VERSION_MAJOR 2
#define DOCTEST_VERSION_MINOR 3
#define DOCTEST_VERSION_PATCH 5
#define DOCTEST_VERSION_STR "2.3.5"

#define DOCTEST_VERSION                                                                            \
    (DOCTEST_VERSION_MAJOR * 10000 + DOCTEST_VERSION_MINOR * 100 + DOCTEST_VERSION_PATCH)
claremacrae commented 5 years ago

TODO Understand whether the code I added in ee66ca9 needs SingleHpp comments, like this:

// <SingleHpp unalterable>
#if DOCTEST_VERSION >= 20305
        // called when a test case is reentered because of unfinished subcases (safe to cache a pointer to the input)
        virtual void test_case_reenter(const doctest::TestCaseData&) override {}
#endif
// </SingleHpp>

My guess is that it doesn't, as I don't think <SingleHpp> looks at anything other than #include, but I'm not sure,

claremacrae commented 5 years ago

This all passes now. I have manually tested that our code still builds with the previous doctest version, so this PR should now be good to go.

claremacrae commented 5 years ago

TODO Understand whether the code I added in ee66ca9 needs SingleHpp comments, like this:

// <SingleHpp unalterable>
#if DOCTEST_VERSION >= 20305
        // called when a test case is reentered because of unfinished subcases (safe to cache a pointer to the input)
        virtual void test_case_reenter(const doctest::TestCaseData&) override {}
#endif
// </SingleHpp>

I've confirmed that we don't need those SingleHpp comments - without them, when I run the release script, the #ifdef and #endif lines are preserved, unaltered, which is what we need to happen.

So we really are good to go!