approvals / ApprovalTests.cpp

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

verify(ApprovalWriter, Options) gives build failure #139

Closed claremacrae closed 4 years ago

claremacrae commented 4 years ago

I've been updating ApprovalTests.cpp.Qt to use the latest ApprovalTests.cpp.

This is the code that now does not compile, once I wrap the reporter in Options:

    inline void verifyQImage(
        const QImage& image,
        const ApprovalTests::Reporter& reporter = ApprovalTests::DiffReporter())
    {
        QImageApprovalWriter image_writer(image);
        ApprovalTests::Approvals::verify(image_writer, ApprovalTests::Options(reporter));
    }

where QImageApprovalWriter implements ApprovalTests::ApprovalWriter

The error output is:

In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/tests/Catch2_Tests/ApprovalsQtTests.cpp:4:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/ApprovalTestsQt/../ApprovalTestsQt/ApprovalsQt.h:4:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/ApprovalTestsQt/../ApprovalTestsQt/integrations/LoadApprovals.h:16:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/third_party/approval_tests_cpp/ApprovalTests.hpp:1:
/Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/third_party/approval_tests_cpp/ApprovalTests.v.8.7.0.hpp:3216:13: error: type 'ApprovalTests::Options' does not provide a call operator
            converter(contents, s);
            ^~~~~~~~~
/Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/ApprovalTestsQt/../ApprovalTestsQt/ApprovalsQt.h:15:35: 
note: in instantiation of function template specialization 'ApprovalTests::TApprovals<ApprovalTests::ToStringCompileTimeOptions<ApprovalTests::StringMaker> >::verify<ApprovalTestsQt::QImageApprovalWriter, ApprovalTests::Options, void>' requested here
        ApprovalTests::Approvals::verify(image_writer, ApprovalTests::Options(reporter));
                                  ^
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/tests/Catch2_Tests/ApprovalsQtTests.cpp:4:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/ApprovalTestsQt/../ApprovalTestsQt/ApprovalsQt.h:4:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/ApprovalTestsQt/../ApprovalTestsQt/integrations/LoadApprovals.h:16:
In file included from /Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/third_party/approval_tests_cpp/ApprovalTests.hpp:1:
/Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/third_party/approval_tests_cpp/ApprovalTests.v.8.7.0.hpp:3216:13: 
error: type 'ApprovalTests::Options' does not provide a call operator
            converter(contents, s);
            ^~~~~~~~~
/Users/clare/Documents/develop/ApprovalTests/ApprovalTests.cpp.Qt/ApprovalTestsQt/../ApprovalTestsQt/ApprovalsQt.h:23:35: 
note: in instantiation of function template specialization 'ApprovalTests::TApprovals<ApprovalTests::ToStringCompileTimeOptions<ApprovalTests::StringMaker> >::verify<ApprovalTestsQt::QTableViewWriter, ApprovalTests::Options, void>' requested here
        ApprovalTests::Approvals::verify(table_writer, ApprovalTests::Options(reporter));
                                  ^

This is supposed to call this method:

        // Note that this overload ignores any scrubber in options
        static void verify(const ApprovalWriter& writer,
                           const Options& options = Options())
        {
            FileApprover::verify(*getDefaultNamer(), writer, options.getReporter());
        }

But instead this method gets selected:

        template <typename T,
                  typename Function,
                  typename = Detail::EnableIfNotDerivedFromReporter<Function>>
        static void
        verify(const T& contents, Function converter, const Options& options = Options())
        {
            std::stringstream s;
            converter(contents, s);
            verify(s.str(), options);
        }

And the code attempts to call options(contents, s) which really doesn't have a call operator.

claremacrae commented 4 years ago

When building against 8.7.0, this change fixes the build:

        template <typename T,
                  typename Function,
+                 typename = IsNotDerivedFromWriter<T>,
                  typename = Detail::EnableIfNotDerivedFromReporter<Function>>
        static void
        verify(const T& contents, Function converter, const Options& options = Options())
        {
            std::stringstream s;
            converter(contents, s);
            verify(s.str(), options);
        }

In 10.0.0 we removed typename = Detail::EnableIfNotDerivedFromReporter<Function> - but the extra IsNotDerivedFromWriter is still required.

The problem only occurs once Reporter arguments are changed to Options.

If Reporter is passed in, there is no ambiguity. I think this is because the new custom ToString mechanism was only added to the new overloads that take Options, and not to the deprecated Reporter-based methods.

claremacrae commented 4 years ago

We do have tests for custom reporters, and we have an ExistingFileWriter (or similar), so I'm unsure what it is about the code in ApprovalTests.cpp.Qt that triggers this scenario...

claremacrae commented 4 years ago

I realise that I will in due course want to update this method so its Reporter argument becomes Options... For now, I wanted to make the smallest changes possible.

    inline void verifyQImage(
        const QImage& image,
        const ApprovalTests::Reporter& reporter = ApprovalTests::DiffReporter())
    {
        QImageApprovalWriter image_writer(image);
        ApprovalTests::Approvals::verify(image_writer, ApprovalTests::Options(reporter));
    }
claremacrae commented 4 years ago

An alternative fix is to say that the Function type is not Options - which matches the previous constraint that Function was not derived from Reporter...

claremacrae commented 4 years ago

This is now fixed - I'll close the issue when we release.

claremacrae commented 4 years ago

This is in the 10.0.1 release, about to be completed