approvals / approvaltests.cpp Goto Github PK

This came from feedback on the slides of my Paris CPPP conference talk in June:

Slide 113, two of the FileApprover::verify arguments are non-const references - if I do some quick poking around the codebase it looks like this might be able to be made const without too much trouble.

As our CMake scripts become more standard and more flexible, we should have a page of documentation describing the supported options.

This might be a good example to look at:
https://github.com/catchorg/Catch2/blob/master/docs/cmake-integration.md#top

See also this evolving project, in which I'm exploring, and testing, options for using 3rd-party dependencies:
https://github.com/claremacrae/ApprovalTests.cpp.Builds

In UTApprovals.h, the #include <ut.hpp> should be #include <boost/ut.hpp>. Users of Boost ut are expected to include the ut header using boost/ut.hpp as can be seen in the tutorial. The ut.hpp header will almost certainly not be found by the user's compiler without explicitly modifying the searched include directories.

In #37 it was really helpful to be able to write code that was able to detect the doctest version number, to be able to support more than one doctest version.

The relevant doctest code is:

// =================================================================================================
// == VERSION ======================================================================================
// =================================================================================================

#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)

Can we do the corresponding thing in Approval Tests too?

This would involve changing the release script, to set the values automatically.

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>"

It would be clearer if it were called third_party/

I'd also like to create sub-directories in - one for each included framework, e.g.

• catch1
• catch2
• doctest

This would allow us to make e.g. our doctest tests only pick up doctest headers, and would more quickly show up mistakes as we move most of our existing tests from catch2 to doctest.

It's fairly unlikely that any users are using a clone of this project directly, but just out of kindness, I would include this in the next release that allows for breaking changes, i.e. 5.0.0

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.

I've started a page to write an outline for a User Guide for this library.

https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/README.md

I'm struggling to come up with useful section headings and page titles, so this ticket is to propose a structure, and seek feedback on it, before much work is done on the actual writing.

Seeing #29 has made me wonder if we should have a CI build that tests our code with Clang's MSVC compatibility:

https://clang.llvm.org/docs/MSVCCompatibility.html

I've been experimenting with what compilers ApprovalTests.cpp builds successfully on.

I'm doing this in a branch on a fork https://github.com/claremacrae/ApprovalTests.cpp/commits/more_travis_builds - to avoid spamming followers of the main project with loads of notifications of failed builds.

I've found that the tests don't compile on g++-4.8 - see this failed build:
https://travis-ci.com/claremacrae/ApprovalTests.cpp/builds/118766632

Looking at the Catch2 Travis build configuration, g++-4.8 is the lowest version of g++ that Catch2 supports currently, so ideally I'd like to support this version for ApprovalTests.cpp too:

https://github.com/catchorg/Catch2/blob/b87caafd9117bf7004af13fe2c806cdb5957d87c/.travis.yml

We are about to introduce a namespace to all our code, so we must make sure that all our examples in docs are machine-generated, so that any docs are updated correctly to show the namespace.

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

I intend the structure to be as per #17 - probably with the addition of a documentation/ directory, which will contain any tests that exist only for the purpose of creating snippets for documentation.

I'll also need to move the corresponding files in approval_tests/ to the same sub-directory in the new location. I'll do this with git mv because moving non-source files in CLion doesn't retain that history of the move...

I believe this function should be marked 'inline' otherwise you get

error LNK2005: "void __cdecl initializeApprovalTestsForGoogleTests(void)" (?initializeApprovalTestsForGoogleTests@@yaxxz) already defined in ApprovalTests.obj

I really like the use of DisposableObjects throughout the project. However, one place where they're missing is custom comparators...

This creates an issue for the way I try to use them:
What I want to do is to verify JSON data, but for the contained floating point values have some sort of tolerance applied to the comparison. So I have an ApprovalComparator that takes the tolerance as a parameter.

But then I have to globally register this comparator for some file extension:

FileApprover::registerComparator(".json", std::make_shared<NumericJSONComparator>(Tolerance{1e-8}));

The problem is that this comparator is now used for all subsequent comparisons of JSON files. And that's not what I want (In fact forgetting to reset could lead to very hard to find false-positive tests...)

The workarounds I see are:

• Use a different extension for each tolerance (not at all nice)
• Write a RAII wrapper that manually resets the comparator to the default TextFileComparator (what I use for now)

    class RegisterComparator {
        public:
            RegisterComparator(std::string extensionWithDot, std::shared_ptr<ApprovalTests::ApprovalComparator> comparator)
            : ext_(extensionWithDot)
            {
                ApprovalTests::FileApprover::registerComparator(ext_, comparator);
            }

            ~RegisterComparator() {
                ApprovalTests::FileApprover::registerComparator(ext_, std::make_shared<ApprovalTests::TextFileComparator>());
            }
        
        private:
            std::string ext_;
    };

But I think it would be cleaner if ApprovalTests included the RAII wrapper out-of-the-box and also that the wrapper reset the state to the state before (i.e. remove from map, instead of overwriting map element with default.

What do you think? Maybe I'm also misusing the custom comparators. If so, is there a better way to accomplish what I want?

Trying to add support for Sublime Merge was very difficult because it requires the command-line argument "mergetool" before the two merged files and passing this in as the second argument to DiffInfo doesn't seem to end up in the command when it is sent to the system. Additionally, the merge tool has a "--no-wait" option which must be used instead of running it as a daemon with "&". Running the Sublime Merge mergetool as a daemon leads to diffs being opened twice.

The inheritance hierarchy was very difficult for me to figure out. An easier way to add a merge tool would be wonderful. Perhaps using a unique placeholder for each of the received and approved files in a string or list of string, the specific command-line call could be structured as needed. An option for whether to run as a daemon or not would also be handy. Thanks.

Below is the code I used to add support for Sublime Merge. I have only tested it on mac and Windows at the moment.

#pragma once
#include <string>
#include <utility>
#include <vector>
#include <ApprovalTests/ApprovalTests.hpp>

namespace ApprovalTests {

namespace DiffPrograms {

namespace Mac {
APPROVAL_TESTS_MACROS_ENTRY(SUBLIME_MERGE,
                            DiffInfo("/Applications/Sublime Merge.app/Contents/SharedSupport/bin/smerge", Type::TEXT))
}

namespace Linux {
APPROVAL_TESTS_MACROS_ENTRY(SUBLIME_MERGE, DiffInfo("smerge", Type::TEXT))
}

namespace Windows {
APPROVAL_TESTS_MACROS_ENTRY(SUBLIME_MERGE, DiffInfo("{ProgramFiles}Sublime Merge\\smerge.exe", Type::TEXT))
}

}

class SublimeMergeSystemLauncher
    :
        public CommandLauncher {
public:
  static bool exists(const std::string &command) {
    bool foundByWhich = false;
    if (!SystemUtils::isWindowsOs()) {
      std::string which = "which " + command + " > /dev/null 2>&1";
      int result = system(which.c_str());
      foundByWhich = (result == 0);
    }
    return foundByWhich || FileUtils::fileExists(command);
  }

  bool launch(std::vector<std::string> argv) override {
    if (!exists(argv.front())) {
      return false;
    }

    std::string command = std::accumulate(argv.begin(),
                                          argv.end(),
                                          std::string(""),
                                          [](const std::string &a, const std::string &b) {
                                            return a + " " + "\"" + b + "\"";
                                          });
    std::string launch = SystemUtils::isWindowsOs() ? ("start \"\" " + command) : command;
    system(launch.c_str());
    return true;
  }
};

class SublimeMergeReporter
    :
        public Reporter {
private:
  std::string m_command;
  mutable SublimeMergeSystemLauncher m_launcher;

public:
  explicit SublimeMergeReporter(std::string command)
      : m_command(std::move(command)) {}

  [[nodiscard]] bool report(std::string received, std::string approved) const override;
};

bool SublimeMergeReporter::report(std::string received, std::string approved) const {
  FileUtils::ensureFileExists(approved);
  return m_launcher.launch({m_command, "mergetool", "--no-wait", received, approved, "-o", approved});
}

As previously reported for Boost.ut (#65) and Catch2 (#62), our include paths are also non-standard for doctest.

When users pull in doctest via FetchContent or add_subdirectory, the header should be included via

#include <doctest/doctest.h>

We have a style guide of sorts:

https://github.com/approvals/ApprovalTests.cpp/blob/master/tests/Catch2_Tests/StyleGuide.h

But nothing to help us keep the formatting consistent...

In another project, I set up a .clang-format file to try to match the Style Guide..
https://github.com/claremacrae/ApprovalTests.cpp.Nursery/blob/master/.clang-format

In particular, as we increase the use of namespaces, e.g. in #34, I would like them to be indented so that namespace nesting can be spotted at a glance...

I know that @alastairUK has similarly been experimenting with this, and preferred not limiting the line length.

This is one of those things where any style definition is going to be better than having something inconsistent...

I would hold off implementing this until all the current PRs and open branches on forks have been resolved, though.

I put this project through Sourcetrail today - and it showed a surprisingly large number of functions - 684 - excluding anything in third_party and google test!

Here's a screenshot of all the ARAXIS_MERGE() functions, for example:

When I use a ClipboardReporter on Mac, and the test fails, I get this output:

sh: pbclip: command not found

The Mac equivalent of pbclip is apparently pbcopy

https://apple.stackexchange.com/questions/15318/using-terminal-to-copy-a-file-to-clipboard

We didn't consider that when writing the code, which says:

const std::string clipboardCommand = SystemUtils::isWindowsOs() ? "clip" : "pbclip";

Feedback on the docs:

Tutorial page, the syntax for passing an output stream overload lambda to the verify call requires you to explicitly pass in the template type Approvals::verify(stuff); is that also a place where you're aiming to not need to pass the template type explicitly?

I now know how to fix this, using the same SFINAE fix that I used for the Combinations tests.

Based on my initial checks, users won't need to update their code, though they could simplify it if they want.

I'd really like to do this - it will simplify slides talks, and the tutorial too.

This was mentioned in #75 - and will provide another nice way of making ApprovalTests available to users...

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

Ideally needs to check for links to:

• non-existent internal pages, that is, to Markdown pages that don't exist
• non-existent internal anchors, that is, to headings that don't exist
• non-existent external pages

Also, needs to check both .md and .source.md - to detect accidental use of relative links.

For details, see the really clear explanation by @athrun22 in: #69 (comment)

Boost ut removed the template parameter TLocation in a recent commit. This template parameter will need to be removed from functions in UTApprovals.h to maintain compatibility with the next Boost ut release.

My test cpp file starts with:

#include "ApprovalTests.hpp"
#include "Catch.hpp"
#include <opencv2/core.hpp>

which has the following error:

include\opencv2\core\persistence.hpp(506): error C2061: syntax error: identifier 'vector'

and at persistence.hpp(506) there is an enum element called EMPTY:

enum Type
{
    ...
    EMPTY     = 32, //!< empty structure (sequence or mapping)
    ...
};

and the error is caused by CombinationApprovals.h which contains:

#define EMPTY std::vector<Empty>{Empty()}

So, can we #undef EMPTY within CombinationApprovals.h?

In https://github.com/claremacrae/ApprovalTests.cpp.Nursery, I use ApprovalTests.cpp as a sub-module.

This has worked fine for some months...

Recently, the Git Desktop app on Windows has been saying that this sub-module is dirty, and I've only just investigated why.

cd D:\Users\Clare\Documents\Programming\github\ApprovalTests\ApprovalTests.cpp.Nursery\third_party\ApprovalTests.cpp
git reset  --hard
error: unable to create file
tests/DocTest_Tests/documentation/approval_tests/DocTestDocumentationSamples.ApprovalTests-MultipleOutputFiles-AutoApproving.American.approved.txt:
Filename too long
fatal: Could not reset index file to revision 'HEAD'.

Travis CI builds have started failing randomly now, due to time-outs. Each failure takes time to establish whether it was a Travis problem or a code problem...

For one recent example, see https://travis-ci.org/approvals/ApprovalTests.cpp/builds/578395889 - where build 9 out of 12 timed-out, meaning that the whole build failed.

Github has recently announced free CI/CD for open-source projects:
https://github.blog/2019-08-08-github-actions-now-supports-ci-cd/

I applied to be on the 'GitHub Actions beta' and have now got in...

I'd like to experiment with github's CI system, to see whether it's more reliable than Travis.

Also, I gather that understanding GitHub actions might make it easier to implement other triggers on commit... (for example, if we have pre-commit steps for running mdsnippets, then each user would have to install mdsnippets before committing, which could discourage participation a bit)

This is speculation, but I think that if we implement this with a GitHub action instead, it might be possible to run the process on a github server...

Most headers in ApprovalTests could be placed in sub-directories, to tidy up, and make it easier to see related files together.

Because our CMake file enables users to have this project as a sub-module, we should only do this when we are releasing other breaking changes, and so bumping the major version number.

We aren't taking advantage of compiler warnings to spot C++ language problems or areas for improvement in our code.

For example, see the changes in 93d97a8 - I spotted that after we did a release - it would have been nice to have one of our compilers point it out for us instead.

Lots has been written about the benefits of enabling warnings from C++ compilers, and how to enable this, for example:

https://arne-mertz.de/2016/02/compiler-warnings-treat-them-right/
https://arne-mertz.de/2016/02/compiler-warnings-part-2-tune-the-compiler/

I'm using this ticket to track a few changes in our code and CMake configuration, to see if we can enable treating warnings as errors, at least for the Catch2 tests, which exercise most of the ApprovalTests.cpp library.

So navigation of macros in Source Trail is more useful.

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.

The tests directory now contains:

ApprovalTests_Catch1_Tests
ApprovalTests_Catch2_Tests
ApprovalTests_DocTest_Tests
ApprovalTests_GoogleTest_Tests
CMakeLists.txt

I've found it would be really nice to be able to cd in to the GoogleTest or DocTest implementations, with filename completion...

The ApprovalTests_ prefix just gets in the way now, and feels a bit like clutter...

This would change the contents to:

Catch1_Tests
Catch2_Tests
DocTest_Tests
GoogleTest_Tests
CMakeLists.txt

I wanted this whilst working on some Qt test code, where I needed to set up a QApplication in main(), and use ApprovalTests, and this isn't yet possible, so I switched back to using Catch2 instead.

Documentation for how to implement this:
https://github.com/onqtam/doctest/blob/master/doc/markdown/main.md

The implementation for this may be quite similar to that in #36

For example, see
https://app.codacy.com/manual/claremacrae/ApprovalTests.cpp/pullRequest?prid=4752929

Some feedback from Neil Horlock to improve the Setup page:

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

At the bottom of the page offer a link to the next logical step "Go write some tests" as well as a return to the Contents page. I think that would make the workflow smoother

When I went to do this, I found that we didn't really have a good set of docs to point to...

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! :)

I think it would be worth-while to support the Boost Test framework out-of-the-box. Is this plausible?

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(...);

This stackoverflow article Points out that there is a non-declared function in MinGW that approvaltests tries to use because it is windows.

You can fix this, by adding a small header file infront of approvaltests

File: MinGW.Patch.h

#ifdef __cplusplus

extern "C" {

#endif

 

#include <sec_api/stdlib_s.h> /* errno_t, size_t */

errno_t getenv_s(

    size_t     *ret_required_buf_size,

    char       *buf,

    size_t      buf_size_in_bytes,

    const char *name

);

 

#ifdef __cplusplus

}

#endif

We would like to reduce the number of manual steps in How To Release

For example, there's a lot of faff around copying and renaming the release notes files.

People who download the single-header release of Approval Tests can use their own Catch header, so long as they name it Catch.hpp.

However, I was told at a recent C++ London Meetup that those download or fork this repo, and who include our top-level CMakeLists.txt file find that the Catch.hpp in our repo gets used in preference to theirs.

I was told that the workaround is to #include your own Catch header before pulling ours in...

Looking at that cmake file, I don't understand the cause of the probem, as I would have thought that our third_party directory was not added to the search path in that situation, due to these lines:

...
if(NOT DEFINED PROJECT_NAME)
    set(NOT_SUBPROJECT ON)
endif()

...

if (NOT_SUBPROJECT)
...
    add_subdirectory(third_party)
...
endif()

So I think we need more info about the problem in order to fix it...

• I have a pre-baked approval file on disk.
• I am generating the "received" contents into a std::string, which is terminated by a '\n'.
• I am passing that std::string into Approvals::verify, which fails due to a second(!) trailing '\n' now appearing on my "received" content.
• It appears that StringWriter::Write(std::ostream &) is determined to add a '\n':

    void Write( std::ostream &out ) const
    {
        out << s << "\n";
    }

This causes my call to verify to fail. So I wonder:

Should Write be modifying my content?

Benefits:

• Reduce the number of things linked from the home page, to move the README content higher up the home page
• Make it easy to search all our tests in one step, in IDEs
• Make the ApprovalTests directory stand out a bit more

From @lp55:

I found a bug in ApprovalTests. Clang provides the location information with unix directory separators, even on Windows (I get something like this: c/folder/main.cpp), which the Approval Tests code for handling filesystem doesn't account for.

I can patch that, but I think it would be better for Approval Tests to offload that responsibility for a specialized library. This seems like a good option: https://github.com/gulrak/filesystem. It raise Approval Tests C++ standard requirement, and makes uses of std::filesystem if available. What do you think?

https://github.com/onqtam/doctest