Documentation is buried and lacking about a critical bug when using the Ninja generator. Any test framework integration, such as Catch2, that uses the __FILE__ macro to locate Approval Test source files will fail when using Ninja. This is not only for MSVC as referenced in the current documentation but for any time the Ninja generator is used. I recently started to use the Ninja generator for all of my builds, so was caught unaware of this issue when using LLVM's Clang with Ninja on macOS. It should also be noted that the Ninja generator is not a problem when using Boost ut because it uses std::source_location instead of the __FILE__ macro. I tested this successfully just to be sure. If this bug is documented before the troubleshooting page, perhaps in the pages on the effected test frameworks, user's may avoid this issue entirely.

CMake in VS2019 seems to default to using the Ninja generator.

If I build with this and run the tests I get exceptions thrown.

1: Catch1_Tests.exe is a Catch v1.12.1 host application. 1: Run with -? for options 1: 1: ------------------------------------------------------------------------------- 1: YouCanVerifyText 1: ------------------------------------------------------------------------------- 1: ..\..\..\tests\Catch1_Tests\ApprovalsTests.cpp(6) 1: ............................................................................... 1: 1: ..\..\..\tests\Catch1_Tests\ApprovalsTests.cpp(6): FAILED: 1: due to unexpected exception with message: 1: Unable to write file: ..\..\..\tests\Catch1_Tests\ApprovalsTests. 1: YouCanVerifyText.received.txt 1: 1: =============================================================================== 1: test cases: 1 | 1 failed 1: assertions: 1 | 1 failed

If I use the Visual Studio 2019 generator then all seems fine

I’d really like to remove the need to edit and recompile to change the reporter used, probably by providing a command-line argument to do so.

We could base the option names on the reporter classes, with the Reporter suffix removed.

Examples that spring to mind:

-r BeyondCompareReporter
--reporter Quiet
-r Clipboard
-r AutoApprove
-r ‘C:\path to my program\...exe’
-r ‘mydifftool -1 {received} -2 {approved} &’
-r ‘someconsolediff {received} {approved}’

We would want to control whether the tool runs in the foreground or background, if it launches another process.

Description

I came across this problem when writing an utility method, which registers the same comparator for multiple file extensions: link.

In linked code, Disposer class wraps a vector of ComparatorDisposer objects. Each ComparatorDisposer is created by FileApprover::registerComparatorForExtension, copied to a vector and destructed and at that point the comparator is immediately disposed.

Expected behavior is that the comparator is disposed once the copy in the vector is destructed. Destructor of the original ComparatorDisposer instance should not dispose the comparator.

The solution

I've added a flag isActive, which is initially set to true. The flag is set to false when a ComparatorDisposer is moved-from, to indicate that it's destructor should not restore previousComparator.

Additionally, I've declared copy constructor as deleted.

From the current maintainer of Catch2, after we announced we had changed from Catch.hpp to catch.cpp:

"I strongly recommend going with #include "catch2/catch.hpp"

Because if you use Catch2 via CMake or having it installed, the include path will be <catch2/catch.hpp>"

Currently, the number of collections in verifyAllCombinations is limited to 9, and is extensible by code by the user of the library. Some legacy code has functions with more than 9 parameters, sometimes many more. It would be more convenient if the user didn't have to write the extension themselves. This issue suggests to increase the number of collections that the library can support natively, ideally without limit, by using variadic templates. If I'm not mistaken, Clare already mentioned this could be an evolution for the library. This issue is then to log this request. Keep up the amazing work! :)

Feedback is painfully slow from our Travis and AppVeyor CI builds now:

• Travis: 40 - 45 minutes elapsed time
  • 13 configurations - each taking 3 to 4 minutes
• AppVeyor - 18 minutes
  • Cygwin: 11 minutes
  • Mingw 7.5 minutes
  • Some of the pain here is the queue time, which is probably outside our control

If the Catch2 project is included via CMake's add_subdirectory() or FetchContent, then the following targets are created, as far as I can tell:

• Catch2
• Catch2::Catch2

Unfortunately I didn't appreciate the significance of this when creating third_party/catch2/CMakeLists.txt - which creates the target catch

The name Catch2::Catch2 is preferred, as if that is missing, a warning is issued when CMake runs, making it easier to track down missing dependencies.

I think it should be possible to create aliases in the CMake files in third_party to retain the old naming, in case any users are already depending on the third_party target-names I created earlier.

Hi,

This is the initial implementation of the [Boost].UT test framework integration support, developed alongside @claremacrae I see that I still have to build the tests for the approval tests of the integration with the test framework :) But please take a look to see if anything else is missing and if there are changes needed for what's been done.

When I try to compile the ApprovalTests.cpp project with Clang, I receive this error message:

In file included from /Users/arh/code/ApprovalTests.cpp/src/ApprovalTests_Catch2_Tests/CombinationTests.cpp:8:
/Users/arh/code/ApprovalTests.cpp/src/ApprovalTests/../ApprovalTests/CombinationApprovals.h:102:58: error: call to function 'operator<<' that is neither visible in the template definition nor found by argument-dependent lookup
                                            s << ") => " << result << '\n';
                                                         ^

Clang is actually correct here, and the problem is that gcc and msvc accept incorrect code. The situation is described in detail at http://clang.llvm.org/compatibility.html#dep_lookup and the second example describes exactly the case in ApprovalTests.

There are two obvious ways to make the problem go away quickly:

1. Put the operator<< overload in CombinationTests.cpp inside the std namespace, so that it can be found by ADL in the verifyAllCombinations function template. Unfortunately this is illegal, as we're only allowed to put full specializations in to the std namespace.
2. Predeclare the operator<< overload before including the CombinationApprovals.h header.

This Godbolt example contains a minimal example: https://godbolt.org/z/nReFWZ

I'm probably going outside the scope of this Issue now, but I believe a better approach would be to provide a customization point that allows users to provide custom string conversions for their own types, or for types that do not easily support a stream insertion operator. Sometimes third-party types do provide a stream insertion operator, but its output isn't acceptable for some reason. In those cases, it's helpful to have a separate mechanism for stringifying the type.

The Catch2 library has a nice mechanism for this: https://github.com/catchorg/Catch2/blob/master/docs/tostring.md The Boost unit test library does it too: https://www.boost.org/doc/libs/1_68_0/libs/test/doc/html/boost_test/test_output/test_tools_support_for_logging/testing_tool_output_disable.html

Neither of those implementations could be described as 'trivial', but I think that some form of customization point that goes beyond requiring an operator<< overload would be advantageous for ApprovalTests.cpp.

Whilst looking in to this, I found a useful reference on best practices for providing customization points: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4381.html

All classes should be put inside namespace. And the class Approvals should be removed since it is just a collection of static functions. They should be free functions inside the namespace.

The name of the namespace should be ideally ApprovalTests. Downstream users would then use

ApprovalTests::verify(...);

Bumps egor-tensin/setup-cygwin from 3 to 4.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Bumps DoozyX/clang-format-lint-action from 0.14 to 0.15.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Bumps actions/checkout from 2 to 3.1.0.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Per Catch2 documentation:

GENERATE can be seen as an implicit SECTION, that goes from the place GENERATE is used, to the end of the scope. [...] The fact that GENERATE introduces a virtual SECTION can also be used to make a generator replay only some SECTIONs, without having to explicitly add a SECTION

I looked into the Catch2 integration and it seems like the Catch2 API does not include generator information as part of the sector:

// Approvals/integrations/catch/Cathc2Approvals.h

struct Catch2TestCommitRevert : Catch::TestEventListenerBase
{
    ...
    virtual void sectionStarting(Catch::SectionInfo const& sectionInfo) override
    {
        currentTest.sections.push_back(sectionInfo.name); // Name here does not include the generator value
    }
    ...
};

In fact, if you look at the Catch::SectionInfo type it only includes name and source location. I'm no expert on the Catch2 internals but at first glance, I would say Catch2 generators are handled more like part of the body of a section rather than variants of the parent section (Like generated tests cases in other frameworks).

The result of this behavior is that if you run an approval inside a section with a generator, the approval is uniquely registered for the section rather than each generated variant of the section, resulting in approval failures following the execution of the different variants of the section:

enum class Keyword
{
    Unknown, Foo, Bar, Quux
};

Keyword parseKeyword(const std::string_view word);

SECTION("Verify our parsing algorithm works with the most relevant words in the vocabulary of a programmer")
{
     const std::string_view word = GENERATE("foo", "bar", "quux");
     Approvals::verify(parseKeyword(word)); // If we approve "foo", it fails in the next iteration of the generator
}

see example here

the example shows a test file that has two failing tests - one of them is using approvals, the other is a non-approvals test.

expected

both failures should be shown

actual

the non-approvals test output is not seen in the terminal